Gate the writer's array context and compressors by the enabled editions - #9019
Gate the writer's array context and compressors by the enabled editions#9019joseph-isaacs wants to merge 8 commits into
Conversation
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
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. |
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. |
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. |
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. |
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. |
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. |
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. |
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↓)
|
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:
|
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:
|
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↓)
|
Benchmarks: Random Access 📖Vortex (geomean): 1.036x ➖ How to read Verdict and Engines
unknown / unknown (1.030x ➖, 0↑ 2↓)
|
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:
|
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↓)
|
Benchmarks: Compression 📖Vortex (geomean): 1.005x ➖ How to read Verdict and Engines
unknown / unknown (0.983x ➖, 6↑ 3↓)
|
| // `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"); |
There was a problem hiding this comment.
@AdamGS @robert3005 what shall we do here?
There was a problem hiding this comment.
I think by adding it here we are effectively commiting to it being stable. We can remove it here for now and work on stabilising
There was a problem hiding this comment.
Agreed, removed in 1288325.
Worth noting it costs less than it would have when the opt-in was written. The gate now normalizes a gated encoding back to an allowed representation instead of rejecting it, so a JNI session without the unstable edition writes vortex.parquet.variant as its canonical form rather than failing the write. The cost is compression ratio on that path, not a broken writer — which makes it easy to leave removed while stabilisation is worked out.
Generated by Claude Code
There was a problem hiding this comment.
Dropping the opt-in in 1288325 breaks the JNI variant write path, and the reason is structural rather than a missing test setup.
write_strategy_for_schema adds ParquetVariant to the allow-list whenever the schema has variant fields, so the gate is active on that path and tries to normalize vortex.parquet.variant away. It cannot: ParquetVariant::execute returns a VariantArray whose core-storage slot is another ParquetVariant (core_storage_without_typed_value, encodings/parquet-variant/src/array.rs:149, which rebuilds a ParquetVariant with typed_value: None). normalize_with_execution executes the root, recurses into the slots, finds the same forbidden encoding, and repeats — the stack overflows. That is the SIGSEGV in the Java job and the SIGABRT in test_file_roundtrip_typed_value_variant_with_zoned_strategy.
The with_strategy path fails differently but for the same reason: no gate is installed there, so the array context rejects it outright with Array encoding vortex.parquet.variant not permitted by ctx.
So "removing it now costs compression ratio rather than failing writes, because the gate normalizes instead of rejecting" does not hold for this encoding — there is no allowed representation for the gate to fall back to.
Options, roughly in order of how well they fit the design in this PR:
- Let
vortex_parquet_variant::initializedeclare its own edition and inclusion, the wayvortex-file/tests/commondoes. Registering the encoding then also declares what it belongs to, which is the "positive admission" model applied consistently — and it puts the stability commitment on the encoding's own edition rather than onunstable. - Restore the
vortex-jniopt-in. Smallest diff, but it is the thing the review objected to. - Declare
vortex.parquet.variantin acoreedition — only if we do mean to commit to its stability.
Worth doing regardless of which we pick: normalize_with_execution should bail with an error when executing an array leaves the same forbidden encoding in the tree, so a gated encoding that cannot be normalized away is a clear write error rather than a stack overflow.
2c0e14d fixes the two vortex-parquet-variant failures by declaring an edition in their test fixture, which is the same migration 1288325 applied to the vortex-file tests. It does not address the JNI path — that needs one of the choices above.
Generated by Claude Code
64a0334 to
965957b
Compare
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
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
The gate stopped filtering the compressor when it moved to write time; it normalizes a gated chunk back to an allowed encoding instead. Two test docs still described the old design, and one test name claimed the write "falls back to another scheme" when what it exercises is the normalization. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
d2447b3 to
c95fee2
Compare
Merging this PR will improve performance by 13.58%
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
`retain_writable_encodings` let an encoding through when *no* registered edition
declared it, so the gate only ever subtracted from a set it did not otherwise
control. That escape hatch is gone, along with `EditionSession::declared_encodings`
which existed only to feed it. The writer now reads the enabled editions and
filters by that:
let enabled_encoding_ids = self.session.enabled_encoding_ids();
let ctx = ArrayContext::new(enabled_encoding_ids.clone())
.with_allowed_ids(enabled_encoding_ids.into_iter().collect());
(`with_allowed_ids` is the method's actual name.) The context no longer intersects
`ALLOWED_ENCODINGS`, nor reads the array registry: the enabled editions are the
whole answer. `writable_encodings` and the write-time gate keep intersecting the
static allow-list, since that is the set the strategy is permitted to emit.
The consequence is that admission is now positive. A session that enables no
edition writes nothing — not even `vortex.primitive`. That is a real behaviour
change for any session assembled below the `vortex` facade, because the
first-party declarations live in the facade and nothing else declares them. It
took 63 of this crate's tests with it, all of the same shape, so the test
sessions now declare an edition covering what they register, mirroring what such
a caller has to do. The `strategytest` edition also gained the structural
encodings (`struct`, `chunked`, `constant`); an edition without them cannot write
a file at all, gated compression or not.
Also drops the `vortex-jni` unstable opt-in, per review: registering
`vortex.parquet.variant` there reads as a commitment to its stability. Removing it
now costs compression ratio on that path rather than failing writes, because the
gate normalizes instead of rejecting.
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
Gating the writer on the enabled editions alone left these two file-roundtrip
tests unable to write: their session is assembled from `array_session()`, so it
enables no edition and the writer admits nothing. One failed on the array
context ("vortex.parquet.variant not permitted by ctx"), the other overflowed
its stack normalizing an encoding it could not admit.
The fixture now declares an edition covering everything it registers and enables
it, mirroring `vortex-file/tests/common` and what any caller below the `vortex`
facade has to do.
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
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. Admission is positive: an encoding may be written only when an enabled edition includes it.
VortexWriteOptions::write_internalbuilds itsArrayContextfromenabled_encoding_ids()alone — both the pre-populated table and thewith_allowed_idspermit set — so an encoding no enabled edition covers cannot be interned, and therefore cannot be serialized, even under a custom write strategy.The consequence is deliberate and worth reviewing: a session that enables no edition writes nothing, not even
vortex.primitive. The first-party declarations live in thevortexfacade, so any session assembled below it (array_session()plusregister_default_encodings) must declare an edition covering what it registers.vortex-file/tests/commonand thevortex-parquet-varianttest fixtures show the shape that takes.Normalization at write time.
EditionGatedStrategy(vortex-file/src/strategy.rs) wraps the flat leaf strategy and normalizes each chunk down to the writable set — the enabled encodings intersected with the strategy's static allow-list — before it reaches the writer. A gated encoding is executed back to an allowed representation rather than rejected, so a compressor scheme the enabled editions do not cover costs compression ratio instead of failing the file.The writable set is deliberately not resolved when the strategy is built. Enabling an edition mutates the session, so a set captured at construction time can disagree with the one the writer's
ArrayContextenforces, and the write then fails on an encoding the compressor was still allowed to produce.LayoutStrategy::write_streamreceives the session, which is where the gate is enforced, so it is resolved there. That also meanswith_strategycallers are gated without opting in — a strategy built with no knowledge of any session is still gated (a_session_agnostic_strategy_is_still_gated).One opt-in that preserves existing behaviour.
enable_default_editionsalso 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.Open issue: the
vortex-jnivariant write pathDropping the
vortex-jniunstable opt-in breaks writing variant data through JNI, and not for a reason more test setup can fix.ParquetVariant::executereturns aVariantArraywhose core-storage slot is anotherParquetVariant, so the gate can never normalizevortex.parquet.variantaway: normalization executes the root, recurses into the slots, meets the same forbidden encoding, and overflows the stack. That is theSIGSEGVin the Java job. See the thread onvortex-jni/src/session.rsfor the options — this needs a decision before merge.Tests
Coverage in
vortex-edition(gate semantics),vortex-file(a gated write that normalizes rather than failing, and a session-agnostic strategy that is still gated), andvortex(what a default session gates out, an end-to-end write and read-back on a deliberately narrowed baseline edition, and narrowing the session after the write options are built — which used to fail withArray encoding vortex.sequence not permitted by ctx).a_gated_compressor_never_emits_a_gated_encodingchecks a related invariant the writer does not depend on: thatBtrBlocksCompressorBuilder::retain_allowed_encodingsreally does bound the compressor's output, i.e. no scheme under-declares itsproduced_encodings.Checks run on the rebased branch:
cargo nextest runforvortex-file,vortex-edition,vortexandvortex-parquet-variant, with default features, withunstable_encodings, and withVORTEX_EXPERIMENTAL_PATCHED_ARRAY=1;cargo clippy --all-targetson the changed crates;cargo +nightly fmt --all --check; and the Python doctests (make -C docs doctest, 271 tests, 0 failures), which pin the two file sizes changed invortex-python/src/io.rs.Not run:
cargo build --workspacefails onlance-encodingneedingprotocin this environment — confirmed pre-existing against a stashed tree. The Java-side JNI tests were not exercised locally; the failure above was reproduced in Rust instead.What APIs are changed? Are there any user-facing changes?
New public API, all additive:
vortex_file::writable_encodings(&VortexSession) -> HashSet<ArrayId>WriteStrategyBuilder's gate is applied automatically; no new builder method is required.Behavioural change for writers: a session whose enabled editions do not cover an encoding will no longer write that encoding; chunks carrying one are normalized back to an allowed representation where one exists. Default sessions built through the
vortexfacade are unaffected —enable_default_editionscovers the core encodings, and the patched-array opt-in preserves that path. The only encoding a default (non-unstable_encodings) session loses isfastlanes.delta, whose scheme is alreadycfg'd out in that configuration, so no compression scheme is affected;a_default_session_gates_out_only_fastlanes_deltapins exactly that.Sessions assembled below the facade are affected: they must now declare and enable an edition covering the encodings they register, or they cannot write.
The two file sizes in the
VortexWriteOptionsdocstrings drop by 40 bytes each, from the gated-out ids no longer being listed in the footer.