Gate the writer's array context and compressors by the enabled editions - #9019
Draft
joseph-isaacs wants to merge 5 commits into
Draft
Gate the writer's array context and compressors by the enabled editions#9019joseph-isaacs wants to merge 5 commits into
joseph-isaacs wants to merge 5 commits into
Conversation
joseph-isaacs
added
action/benchmark
Claude
github-actions
Bot
removed
the
action/benchmark
Contributor
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Contributor
Benchmarks: PolarSignals Profiling 📖Vortex (geomean): 0.965x ➖ How to read Verdict and Engines
datafusion / vortex-file-compressed (0.965x ➖, 1↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.039x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.024x ➖, 0↑ 0↓)
datafusion / parquet (1.055x ➖, 0↑ 4↓)
duckdb / vortex-file-compressed (1.042x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.030x ➖, 0↑ 0↓)
duckdb / parquet (1.031x ➖, 0↑ 2↓)
duckdb / duckdb (1.029x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb NVMe 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.975x ➖, 0↑ 0↓)
datafusion / vortex-compact (0.986x ➖, 0↑ 0↓)
datafusion / parquet (0.973x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.993x ➖, 0↑ 1↓)
duckdb / vortex-compact (0.986x ➖, 0↑ 0↓)
duckdb / parquet (0.962x ➖, 1↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: Vortex queries 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.996x ➖, 0↑ 0↓)
datafusion / parquet (1.001x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.017x ➖, 0↑ 0↓)
duckdb / parquet (1.002x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-DS SF=1 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.002x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.008x ➖, 0↑ 1↓)
datafusion / parquet (1.013x ➖, 1↑ 3↓)
duckdb / vortex-file-compressed (1.008x ➖, 0↑ 4↓)
duckdb / vortex-compact (1.009x ➖, 0↑ 4↓)
duckdb / parquet (1.013x ➖, 0↑ 1↓)
duckdb / duckdb (1.000x ➖, 1↑ 1↓)
No file size changes detected. |
Contributor
Benchmarks: Statistical and Population Genetics 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
duckdb / vortex-file-compressed (0.992x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.971x ➖, 0↑ 0↓)
duckdb / parquet (0.979x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: TPC-H SF=10 on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.908x ➖, 8↑ 0↓)
datafusion / vortex-compact (0.924x ➖, 3↑ 0↓)
datafusion / parquet (0.928x ➖, 4↑ 0↓)
duckdb / vortex-file-compressed (0.941x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.952x ➖, 0↑ 0↓)
duckdb / parquet (0.971x ➖, 0↑ 0↓)
duckdb / duckdb (0.961x ➖, 0↑ 0↓)
No file size changes detected. |
Contributor
Benchmarks: FineWeb S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.976x ➖, 0↑ 0↓)
datafusion / vortex-compact (1.029x ➖, 0↑ 1↓)
datafusion / parquet (0.899x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.063x ➖, 0↑ 1↓)
duckdb / vortex-compact (1.154x ➖, 0↑ 1↓)
duckdb / parquet (1.028x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Clickbench Sorted on NVME 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.065x ➖, 1↑ 6↓)
datafusion / parquet (1.496x ❌, 0↑ 9↓)
duckdb / vortex-file-compressed (1.050x ➖, 0↑ 2↓)
duckdb / parquet (1.000x ➖, 0↑ 0↓)
duckdb / duckdb (1.006x ➖, 0↑ 0↓)
File Size Changes (201 files changed, +0.0% overall, 109↑ 92↓)
Totals:
|
Contributor
Benchmarks: Clickbench on NVME 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.049x ➖, 2↑ 6↓)
datafusion / parquet (1.559x ❌, 0↑ 29↓)
duckdb / vortex-file-compressed (0.986x ➖, 4↑ 2↓)
duckdb / parquet (1.000x ➖, 0↑ 1↓)
duckdb / duckdb (1.003x ➖, 0↑ 1↓)
File Size Changes (1 files changed, -0.0% overall, 0↑ 1↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=1 on S3 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.068x ➖, 0↑ 3↓)
datafusion / vortex-compact (1.020x ➖, 0↑ 0↓)
datafusion / parquet (0.952x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.006x ➖, 0↑ 0↓)
duckdb / vortex-compact (1.020x ➖, 0↑ 1↓)
duckdb / parquet (1.029x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Random Access 📖Vortex (geomean): 1.036x ➖ How to read Verdict and Engines
unknown / unknown (1.030x ➖, 0↑ 2↓)
|
Contributor
Benchmarks: Appian on NVME 📖Verdict: No clear signal (low confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (1.018x ➖, 0↑ 0↓)
datafusion / parquet (0.995x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (0.995x ➖, 0↑ 0↓)
duckdb / parquet (1.001x ➖, 0↑ 0↓)
duckdb / duckdb (0.990x ➖, 0↑ 0↓)
File Size Changes (1 files changed, -0.0% overall, 0↑ 1↓)
Totals:
|
Contributor
Benchmarks: TPC-H SF=10 on S3 📖Verdict: No clear signal (environment too noisy confidence) How to read Verdict and Engines
datafusion / vortex-file-compressed (0.979x ➖, 0↑ 1↓)
datafusion / vortex-compact (0.968x ➖, 0↑ 0↓)
datafusion / parquet (1.021x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed (1.003x ➖, 0↑ 0↓)
duckdb / vortex-compact (0.993x ➖, 0↑ 0↓)
duckdb / parquet (1.048x ➖, 0↑ 0↓)
|
Contributor
Benchmarks: Compression 📖Vortex (geomean): 1.005x ➖ How to read Verdict and Engines
unknown / unknown (0.983x ➖, 6↑ 3↓)
|
joseph-isaacs
commented
Jul 28, 2026
Comment on lines
+26
to
+31
| // `vortex.parquet.variant` is declared by the `unstable` family, so the writer would gate it | ||
| // out of a core-only session. Registering the encoding here is the opt-in to writing it. | ||
| session | ||
| .enable_edition(DEFAULT_UNSTABLE_EDITION) | ||
| .map_err(|e| vortex_err!("{e}")) | ||
| .vortex_expect("default unstable edition is registered"); |
Contributor
Author
There was a problem hiding this comment.
@AdamGS @robert3005 what shall we do here?
The file writer could emit any encoding in `ALLOWED_ENCODINGS` regardless of which editions a session had enabled for writing, so a core-only session could still put an unstable-family encoding into a file whose compatibility guarantee does not cover it. Add `EditionSessionExt::retain_writable_encodings`, which drops every encoding a registered edition declares but the session has not enabled. Encodings no registered edition declares are left alone, so sessions outside the edition system — and third-party encodings without an inclusion — are not gated. Apply it in two places: - `VortexWriteOptions::write_internal` builds its `ArrayContext` from the gated ids, for both the pre-populated table and the permitted set, so a gated-out encoding cannot be interned even under a custom write strategy. - `WriteStrategyBuilder::with_session_editions` narrows the allow-list, and `build` now filters the BtrBlocks compressor's schemes to those whose output the allow-list covers. Filtering happens on the finished builder, so schemes added later — including the compact Zstd/Pco ones — are filtered too. Otherwise the compressor would produce an encoding the validator then rejects, failing the write instead of picking the next best scheme. Two sessions opt into the unstable edition to keep behaviour they already had: the default session when experimental patched arrays are switched on, since the compressor then emits `vortex.patched`; and the JNI session, which registers `vortex.parquet.variant`. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013prrhMhUESQfFucbDPjYAd
…tests Gating the writer by the enabled editions makes the default session stop listing `fastlanes.delta` in a file's array context. Two doctests in `vortex-python/src/io.rs` assert exact file sizes, and both shrank by exactly 40 bytes as a result. Reproduced locally: `chonky.vortex` is 215900 bytes and `tiny.vortex` is 55028. The gate filters compression schemes by their declared `produced_encodings`, so a scheme that under-declares would survive the filter, compress into a gated encoding, and make the writer fail the file rather than fall back — `LayoutStrategyEncodingValidator` normalizes with `Operation::Error`, and nothing in the writer uses the `Operation::Execute` fallback. Add a test that checks those declarations against what the schemes actually emit: it narrows a real session to the baseline `core2025.05.0` edition, which predates Zstd, Pco, FastLanes RLE and `vortex.masked`, then compresses primitive, float, string and boolean data with the widest scheme set the writer ever uses and asserts nothing outside the allow-list reaches the output. Removing the `retain_allowed_encodings` call makes it fail on `vortex.pco`, so it bites. Pin the gate's effect in both feature configurations, since neither is otherwise covered: - Without `unstable_encodings`, `fastlanes.delta` is the only allow-list entry no core edition declares, and its scheme is compiled out in that build, so no compression scheme is affected. - With `unstable_encodings` — the configuration every benchmark builds with — the gate is a no-op, so it cannot change a byte the benchmarks write. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013prrhMhUESQfFucbDPjYAd
joseph-isaacs
force-pushed
the
claude/writer-conntext-array-encoding-gate-wipyib
branch
from
July 28, 2026 09:10
64a0334 to
965957b
Compare
Extend `a_gated_compressor_never_emits_a_gated_encoding` to decimal, opaque binary and a struct that mixes column types, so the check covers cascading across schemes rather than a single scheme in isolation. Add an end-to-end test that writes and reads back a file with the narrowed edition, exercising the compressor filter, the array context and the leaf validator together. Add an ignored test for a bug this PR introduces. `VortexWriteOptions::new` resolves the writable set when the write options are built, but the writer's array context resolves it again at write time. Narrowing the session between those two points leaves the compressor filtered against the old, wider set while the context enforces the new, narrower one, and the write fails with `Array encoding vortex.sequence not permitted by ctx`. Deriving the set at strategy-construction time is the wrong place for it: `LayoutStrategy::write_stream` already receives the session, so the gate belongs there, where it is enforced. Moving it would also remove the need for `WriteStrategyBuilder::with_session_editions` and cover custom strategies automatically. Left failing and ignored so the fix lands with a test that already reproduces the problem. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013prrhMhUESQfFucbDPjYAd
…s built `WriteStrategyBuilder::with_session_editions` resolved the writable set when the strategy was built, but the writer's array context resolved it again at write time. Enabling an edition mutates the session, so anything that narrowed it between those two points left the compressor filtered against the old, wider set while the context enforced the new, narrower one, and the write died with `Array encoding vortex.sequence not permitted by ctx`. Deriving the set from mutable session state at construction time is the wrong place for it. `LayoutStrategy::write_stream` receives the session, which is where the gate is actually enforced, so resolve it there: - Drop `with_session_editions`. The builder no longer knows about sessions, and `VortexWriteOptions::new` goes back to a plain `build()`. - Drop the build-time scheme filtering. Filtering by the static allow-list was a no-op, and filtering by the session's is what created the ordering hazard. - Add `EditionGatedStrategy`, which wraps the flat leaf, resolves the writable set from the session inside `write_stream`, and normalizes each chunk with `Operation::Execute`. When the session's editions already cover the allow-list — every session by default — it skips the traversal entirely. Two consequences worth noting. Strategies built through `WriteStrategyBuilder` are now gated whether or not the caller opts in, so the `with_strategy` callers that previously slipped past the compressor filter are covered; the test that asserted a session-agnostic strategy fails now asserts it writes and round-trips. And because gated encodings are executed back to an allowed representation rather than rejected, a scheme whose output an edition does not cover costs compression ratio on a narrowed session instead of failing the file, which no longer makes the gate depend on `produced_encodings` being exhaustive. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013prrhMhUESQfFucbDPjYAd
`cargo doc --document-private-items` runs with `-D warnings`, and the label
already resolves to the same path:
error: redundant explicit link target
--> vortex-file/src/strategy.rs:171:22
| /// [`ArrayContext`](vortex_array::ArrayContext) enforces, ...
| -------------- ^^^^^^^^^^^^^^^^^^^^^^^^^^ explicit target is redundant
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013prrhMhUESQfFucbDPjYAd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
The file writer could emit any encoding in
ALLOWED_ENCODINGSregardless of which editions a session had enabled for writing. A core-only session could therefore put an unstable-family encoding into a file whose read-compatibility guarantee does not cover it — the edition machinery existed but nothing on the write path consulted it.What changes are included in this PR?
The gate.
EditionSessionExt::retain_writable_encodings(vortex-edition/src/session.rs), backed by a newEditionSession::declared_encodings. An encoding survives if an enabled edition includes it, or if no registered edition declares it at all.That second clause is the design decision worth reviewing. A strict "enabled editions only" gate would break every session that registers an encoding outside the first-party declarations:
vortex-geodeclares no inclusion anywhere, and sessions built fromarray_session()register no editions at all and would be left unable to write anything. Undeclared means ungated; declared-but-not-enabled is removed.Two application points:
VortexWriteOptions::write_internalbuilds itsArrayContextfrom the gated ids — both the pre-populated table and thewith_allowed_idspermit set — so a gated-out encoding cannot be interned, and therefore cannot be serialized, even under a custom write strategy. Written files also stop listing gated-out ids in the footer.WriteStrategyBuilder::with_session_editionsnarrows the allow-list, andbuildfilters the BtrBlocks compressor's schemes to those whose output the allow-list covers. The filter runs on the finished builder, so schemes added later — includingwith_compact()'s Zstd/Pco — are covered too. Without this the compressor would produce an encoding the context validator then rejects, failing the write outright instead of falling back to the next-best scheme.Two opt-ins that preserve existing behaviour. Both are places where the gate would otherwise silently remove capability:
enable_default_editionsnow also enables the unstable edition when experimental patched arrays are switched on. The compressor emitsvortex.patchedin that mode, and only theunstablefamily declares it — without the opt-in, gating it out would take BitPacking and ALP with it, since those schemes declare Patched output when the flag is on. That would be a silent compression regression.vortex-jnisession enables the unstable edition, as it registersvortex.parquet.variant(declared inunstable2026.04.0).Tests
New coverage in
vortex-edition(gate semantics, including the ungated-session case),vortex-file(allow-list narrowing plus two end-to-end write tests), andvortex. I verified the key regression test bites: with theretain_allowed_encodingscall removed frombuild,gated_editions_filter_the_compressor_rather_than_failing_the_writefails with a validation error rather than falling back to another scheme.Checks run:
cargo testforvortex-file,vortex-edition,vortex,vortex-btrblocks,vortex-parquet-variant; the same with--features unstable_encodingsand withVORTEX_EXPERIMENTAL_PATCHED_ARRAY=1; doc tests;cargo +nightly fmt --all;cargo clippy --all-targetson the changed crates; and builds ofvortex-jni,vortex-ffi,vortex-tui,vortex-compat.Not run:
cargo build --workspacefails onlance-encodingneedingprotocin this environment — confirmed pre-existing against a stashed tree. The Java-side JNI tests were not exercised, so the parquet-variant write path there rests on the declaration plus the added edition opt-in rather than on execution.What APIs are changed? Are there any user-facing changes?
New public API, all additive:
EditionSession::declared_encodings() -> Vec<Id>EditionSessionExt::retain_writable_encodings(impl IntoIterator<Item = Id>) -> Vec<Id>WriteStrategyBuilder::with_session_editions(&VortexSession)Behavioural change for writers: a session whose enabled editions do not cover an encoding will no longer write that encoding, and the default write strategy's compressor will not select schemes producing it. Default sessions are unaffected —
enable_default_editionscovers the core encodings, and the two opt-ins above preserve the unstable paths that previously worked.Callers that build a custom strategy and pass it to
with_strategy(vortex-python,vortex-tui,vortex-bench,vortex-ffi,fuzz, compat-gen) get the context gate but not the compressor filtering. On a default session the only affected scheme isfastlanes.delta, which is alreadycfg'd off withoutunstable_encodings, so those sites are safe today. Adding.with_session_editions(&session)there would make that robust rather than incidental; I left it out as beyond the scope of this change.Generated by Claude Code