Gate DecimalByteParts lower parts through editions via a v2 serialized format - #9276
Draft
joseph-isaacs wants to merge 25 commits into
Draft
Gate DecimalByteParts lower parts through editions via a v2 serialized format#9276joseph-isaacs wants to merge 25 commits into
joseph-isaacs wants to merge 25 commits into
Conversation
`DecimalByteParts` reserved a `lower_parts` field but never populated it:
the encoding only ever held a single signed most significant part, so
decimals wider than 64 bits after narrowing were left uncompressed as raw
`i128`/`i256` buffers, and `deserialize` asserted `lower_part_count == 0`.
The encoding now stores the reserved lower parts. A value is a signed MSP
plus `k` non-nullable `u64` parts ordered most significant first, which is
the value's two's complement bit pattern cut on 64-bit boundaries:
msp * 2^(64k) + Σ lower[i] * 2^(64 * (k - 1 - i))
`i128` splits into an `i64` MSP and one lower part, `i256` into an `i64`
MSP and three. `split_decimal` / `assemble_decimal` in the new `limbs`
module are the single definition of that layout, used by the encoding's
canonicalization and by the compressor.
Encoding changes:
- `lower_parts` becomes a variadic slot tail, so parts are ordinary
children: written and read by serde, with the child count checked
against `lower_part_count` rather than asserted to be zero.
- Canonicalization and `scalar_at` reassemble the parts, widening to
`i128` or `i256` depending on the MSP width and part count.
- `filter`, `take`, `slice` and the parent filter push-down apply to every
part; `mask` and nullability `cast` touch only the MSP, which carries
validity; `is_constant` requires every part to be constant, except for
an all-null array whose lower parts hold undefined bits.
- The `compare` push-down against a constant now bails when lower parts
are present — the MSP alone no longer determines the ordering — and
falls back to the canonical comparison.
- The CUDA executor bails for arrays with lower parts instead of decoding
the MSP as the whole value.
Compressor changes:
- `DecimalScheme` splits post-narrowing `i128`/`i256` arrays and cascades
into each part instead of returning the decimal uncompressed.
Tests:
- Split/assemble round trips over both limb boundaries and both signs, at
`i128::MIN/MAX` and `i256::MIN/MAX`.
- Consistency, filter, cast and binary-numeric conformance suites over
arrays with one and three lower parts, nullable and non-nullable.
- Serde round trips for 0, 1 and 3 lower parts, asserting the part count
survives, plus `deserialize` rejecting child-count and bound violations.
- Construction rejects signed, nullable, mis-sized and too-many lower
parts.
- Compressor tests pinning one lower part for `i128`, three for `i256`,
and the canonical storage width of the result.
- Compression ratio: 16k wide values with 24 bits of noise compress 5.3x
(`i128`) and 10.7x (`i256`) at the array level, and 7.4x through a
Vortex file end to end, where before splitting they were stored raw.
- A wide-decimal column added to the compat fixture so future readers
must decode today's lower parts.
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Reassembling byte parts filled a stack array of 64-bit words per row at indices derived from a runtime part count, so every word placement was a dynamic index with a bounds check and nothing about the loop was known to the compiler. `benches/decimal_assemble.rs` benchmarks the candidate shapes over 65,536 rows, each spelled out in the bench so the comparison can be re-run from any revision: | shape | i128 (1 part) | i256 (3 parts) | | --------------------------- | ------------- | -------------- | | row, runtime part count | 114.4 µs | 307.5 µs | | row, constant part count | 91.7 µs | 178.2 µs | | column, lane writes | - | 401.3 µs | | column, lane writes blocked | - | 289.3 µs | | column, whole-value shifts | 88.0 µs | 2.03 ms | Row-at-a-time is not what costs — the runtime part count is. Columnar is worse for `i256`: the output word for a given part is strided by 32 bytes, so each pass scatters, and expressing the pass as whole-value shifts pays 256-bit arithmetic per row. Only for `i128`, at 16 bytes per row, does a two-pass column shape match the specialized row loop, and there both are memory bound. So the assembly loops now take the part count as a const parameter, with `assemble_decimal` dispatching 1/2/3 parts into monomorphized bodies, and the `i128` path — where a signed MSP can only ever share 128 bits with one lower part — is specialized outright. Parts are sliced to the MSP's length up front so the per-row bounds checks fall away. Through the public API, on the same 65,536 rows: | benchmark | before | after | speedup | | -------------------------------- | -------- | -------- | ------- | | `i128_assemble_shipped` | 114.7 µs | 93.1 µs | 1.23x | | `i256_assemble_shipped` | 358.4 µs | 201.3 µs | 1.78x | | `canonicalize_byte_parts` 1 part | 118.6 µs | 92.0 µs | 1.29x | | `canonicalize_byte_parts` 3 part | 361.0 µs | 201.8 µs | 1.79x | `assemble_decimal` is now public, matching `split_decimal`, so the benchmark can call the shipped path directly. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…cision Two defects in the lower-parts support, both found by review of the preceding commits. `take` with a nullable indices array failed outright on any array carrying lower parts. Taking builds a `Dict`, and `Array<Dict>::try_new` unions the codes' nullability into the values' dtype, so a non-nullable `u64` lower part came back as `u64?` — which `validate` rejects, because lower parts must be non-nullable with validity held by the MSP alone. The error propagated out of the kernel instead of falling back, so the whole scan failed with "lower part 0 must have dtype u64, got u64?". Arrays without lower parts were unaffected, so this arrived with the lower-parts work. The kernel now returns `Ok(None)` for nullable indices when lower parts are present, deferring to the canonical path, the same way `compare` already declines the MSP-only pushdown. Separately, nothing cross-checked the width the parts assemble into against the declared precision. `validate` bounded the part count and checked each part's dtype, and `assemble_decimal` dispatched purely on `(msp ptype, part count)`, so a file declaring `Decimal(38, 2)` with two lower parts deserialized happily, canonicalized to `i256` values of 39 digits, and then panicked in `Scalar::decimal`'s `vortex_expect` on scalar access. `validate` now requires the assembled type to be no wider than the precision needs, which rejects the crafted array at deserialization. The redundant `MAX_LOWER_PARTS` check goes away with it: `assembled_values_type` already performs it with the same message. The four one-line rejection tests become one `rstest` with the new over-wide case as a fifth, and `take` gains an `rstest` covering nullable indices against one and three lower parts, checked against the canonical take rather than just for absence of an error. Both new cases fail without their fix. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…nels Re-running `benches/decimal_assemble.rs` after the review corrected a claim the previous commit made. Specializing the part count is worth 1.85x on `i256`, as reported, but on `i128` it is worth only ~1.04x — the 1.25x figure did not reproduce. What actually costs on `i128` is the write: pushing into a reserved buffer instead of storing into a pre-sized one is the whole difference at 16 bytes per row. A new `i128_row_write` variant isolates it, holding the loop shape fixed and changing only the output buffer. Over 65,536 rows, `fastest` of three runs each: | shape | i128 | i256 | | ---------------------------- | ------- | ------- | | row, runtime part count | 143 µs | 351 µs | | row, const part count, push | 138 µs | 190 µs | | row, const part count, write | 83 µs | 196 µs | | column, lane writes | 103 µs | 438 µs | So the columnar shape was never the interesting axis: it beats the *pushing* row loop on `i128` but still loses to the single-pass write, and the second pass buys nothing once the push is gone. On `i256` the write shape ties the push shape, because 32 bytes of stores per row dominate either way, so only `assemble_i128` changes. Through the array API, one lower part goes 138 µs -> 83 µs (1.6x); three parts is unchanged at ~209 µs. The rest is cleanup from the same review. Seven kernels open-coded "map every part, rebuild the array", and two of them had already been fixed in this branch for dropping the lower parts on the floor. `map_parts`, `with_msp` and `decimal_dtype` replace all seven, so a part-wise op cannot silently lose a part, and the argument for why an MSP-only rebuild is sound lives in one doc comment instead of being restated or omitted per site. Dead code: `DecimalBytePartsDataParts` had exactly one reference in the repository — its own definition — and this branch had been growing it a field and doc comments. The `[first]` arm of the `i256` dispatch is unreachable, since one lower part under a <=64-bit MSP always lands in an `i128`; a comment now says so where the arm was. Visibility: `assemble_decimal`, `assembled_values_type` and `LOWER_PART_DTYPE` had no callers outside the crate and are now crate-private. `assemble_decimal` was public only so the benchmark could call it, but `canonicalize_byte_parts` already measures the same assembly through the array API, so the two `*_assemble_shipped` benches go with it. As public API it could also panic rather than error on an unsigned MSP, since signedness is only checked on the zero-parts path. The metadata accessor `lower_parts()` returned a count while the generated slots accessor of the same name returns the arrays, both in scope in the same module; it is now `lower_part_count()`. The btrblocks scheme spelled the child layout as `1 + MAX_LOWER_PARTS` and `idx + 1` where the encoding crate has named slot constants; it now uses them. Three hand-rolled LCGs become `StdRng::seed_from_u64`, matching the rest of the repo. Four one-line rejection tests became one `rstest` in the previous commit; the two removed columnar bench variants are recorded in the module doc with their numbers rather than kept as dead code. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Auditing each compute function against the reduce/execute contract — `*Reduce` operates "purely on array metadata and structure without needing to read or execute on the underlying buffers", `*Kernel`/ `*Execute` may read buffers and take an `ExecutionCtx` — turned up two kernels on the wrong side of it. `take` was implemented as `TakeExecute` and registered as an execute parent kernel, but its body ignores the context entirely: `ArrayRef::take` wraps each part in a `Dict` and optimizes, which is a lazy rewrite, and the only other work is the `validate` call rebuilding the array. It is now `TakeReduce`, registered through `TakeReduceAdaptor` alongside the other parent reduce rules, so the push-down happens during optimization rather than being deferred to execution. `TakeReduceAdaptor` also applies the empty-indices and empty-array preconditions and propagates take statistics, neither of which the execute path was doing. The nullable indices guard keeps its meaning: `Ok(None)` now means "cannot do this without buffers", which is exactly the fallback it was asking for. `DecimalBytePartsFilterPushDownRule` was byte-for-byte what `FilterReduceAdaptor(DecimalByteParts)` already does via `FilterReduce`, and was listed first so it shadowed the adaptor — which meant filtering also skipped the adaptor's empty-mask preconditions. Removed; the adaptor that was already registered covers it. The other kernels are on the correct side and stay put. `filter`, `slice`, `cast` and `mask` build lazy wrappers only. `compare` needs `all_valid` to decide whether an uncoercible constant can be answered without null checks, and `is_constant` reads its children, so both legitimately take a context. `take_pushes_down_without_executing` pins the new behavior: it asserts that `take` on a wide array reduces to the encoding rather than being left as a `vortex.dict`, and fails with "got vortex.dict" if the rule is unregistered. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
The wide `DecimalByteParts` columns were added to the existing `decimal_byte_parts.vortex` fixture, which breaks the compat contract. `DESIGN.md` states it directly under "Fixture evolution": a fixture's `build()` is immutable once published, because `check` compares files written by older releases against what `build()` produces today. Adding a column changes the schema the generator emits, so the check fails against every previously published version — exactly the regression the fixture exists to catch, reported against unrelated releases. `decimal_byte_parts.vortex` is restored to its published definition, and the wide cases move to a new `decimal_byte_parts_wide.vortex` with a comment recording why the split exists rather than leaving the next person to rediscover the rule. The new fixture gains a negative `i128` column so sign extension above the MSP is exercised on read back, alongside the one-lower-part and nullable three-lower-part cases. Verified with `generate` followed by `check --mode exact`: 36 fixtures pass, and `decimal_byte_parts.rs` is byte-identical to its pre-branch state. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
`i256::from_parts` takes a `u128` and an `i128`, so each row of the assembly loop ends in `u128::from(w0) | (u128::from(w1) << 64)`. The reasonable suspicion is that this is worse than storing four `u64`s by hand, since 128-bit integers have a reputation for lowering badly. `i256_row_words` is that hand-written version: it builds a `u64` lane buffer and reinterprets it as `i256` at the end, so no 128-bit value is ever written. Over 65,536 rows it ties the shipped shape across four runs (`fastest` 224-228 µs against 227-236 µs), which is inside the noise on this host. Disassembly explains the tie and is the more durable evidence. Neither shape emits a single `shld`/`shrd`, and both compile to four plain 64-bit stores per row at offsets 0x0/0x8/0x10/0x18. The `i128` loop is the same: `(i128::from(msp) << 64) | i128::from(part)` becomes two 64-bit stores with no shift at all. A shift by a constant multiple of 64 followed by an or is pure data movement and LLVM recognizes it as such; the 128-bit codegen actually worth avoiding is division and remainder, which call into compiler-rt, and shifts by a runtime amount. Neither appears in this code. So no change to the assembly loops. The variant and the reasoning stay in the benchmark, because "avoid the u128" is a rewrite someone will propose again and this is the answer. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
The hand-written shape variants have served their purpose: the design questions they were written to answer are settled, and the answers are recorded in the module docs. Keeping them means maintaining a second copy of the assembly loop that no test covers and that silently stops representing the shipped code the moment that loop changes. `canonicalize_byte_parts` stays. It goes through the array API rather than duplicating the loop, so it tracks whatever shape the crate ships and works as a regression guard. The module docs keep the measured conclusions — const part count is 1.85x on `i256`, the pre-sized write is 1.6x on `i128`, columnar loses on both, and hand-written 64-bit words tie the `u128` packing because neither emits a shift — with a note that the variants are recoverable from history if a future change needs to re-run the comparison rather than trust the numbers. 346 lines to 105. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
An `i256` is exactly `{_0: u64, _1: u64, _2: u64, _3: i64}`: three
unsigned words beneath a single signed one. That is the same shape this
encoding stores — unsigned lower parts under a signed most significant
part — and it is why splitting and reassembling are pure reinterpretation
rather than arithmetic. No carry crosses a word boundary, so each word
compresses independently and goes back verbatim.
The code did not say so. Three sites open-coded the same word math with
`to_parts`/`from_parts` and shifts: `split_i256` unpacking, and
`combine_i256` and `assemble_i256` packing, the latter two character for
character identical. A reader had to re-derive the layout at each one, and
`split_i256` carried a `cast_possible_truncation`/`cast_sign_loss` expect
that hid where the truncation was meant to happen.
`i256_to_words` and `i256_from_words` now name the reinterpretation, and
`sign_extended_words` names the other half of the invariant: the words
above the most significant part are its sign. `split_i256` reads as the
inverse of `assemble_i256` at `K == MAX_LOWER_PARTS`, and says so.
Codegen is unchanged. `assemble_i256` still compiles to four plain 64-bit
stores per row at offsets 0x0/0x8/0x10/0x18 with no `shld`/`shrd`, and the
only shifts in the function are index scaling and a single `sar $0x3f` —
the branchless sign broadcast, which is the ideal lowering of the fill.
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
A reader that predates lower parts expects this encoding to have exactly one child, so a file containing a multi-child `DecimalByteParts` is one it cannot open. Introducing lower parts is now gated behind `unstable_encodings` at both places that can introduce them. `DecimalByteParts::try_new_with_lower_parts` rejects a non-empty lower-parts list without the feature, and names the feature in the error. `try_new` builds a single child and is unaffected. In the compressor, the decimal scheme leaves values too wide for one signed part as the canonical decimal instead of splitting them, and reports `num_children` as 1 — restoring exactly the pre-lower-parts behaviour, which was to return the narrowed array uncompressed. The gate is on *introducing* lower parts, not on having them. Rebuilding an array whose parts already exist goes through a new crate-private `rebuild_with_lower_parts`, which every compute kernel uses via `map_parts`/`with_msp`, and `deserialize` is untouched. Gating those too would mean a build without the feature could not read a file written by a build with it — strictly worse than not being able to write one. An earlier revision of this change did gate them, and `compute_over_existing_lower_parts_is_not_gated` fails without the split: reverting `map_parts` to the public constructor breaks filter, take, slice and the consistency suite on every wide array. That test also drove the gate's shape. Letting the crate's own unit tests through the gate via `cfg!(test)` would have hidden exactly that bug, since unit tests would no longer run the configuration they ship. The gate is therefore purely `cfg!(feature = ...)`, and the test helpers that build wide arrays call `rebuild_with_lower_parts` explicitly, so a default `cargo test` still covers the multi-part paths while running the same gate production does. `tests/lower_parts_gate.rs` covers the gate itself. Tests that assert lower parts are *produced* — the btrblocks split and compression-ratio tests, and the vortex-file round trip — are gated on the feature, since without it the compressor deliberately declines. The benchmark declares `required-features` for the same reason. The wide compat fixture is gated too: it is a written file, so generating it by default would emit precisely what the gate exists to prevent. A default `generate` produces 35 fixtures, and 36 with the feature; `check --mode exact` passes in both. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Gating construction and the compressor was not enough. An array read from a file can be handed straight back to a writer without passing through either: `deserialize` is deliberately ungated so a build without the feature can still read such files, and the write allow-list checks only the encoding id, not how many children it carries — `ALLOWED_ENCODINGS` inserts `DecimalByteParts.id()` unconditionally. A build that could never have constructed a multi-child array could therefore still emit one. This was demonstrable, not theoretical: `test_serde_round_trip` with three lower parts passed on default features before this change. `VTable::serialize` now refuses an array carrying lower parts unless the feature is on. That is the last point before bytes reach a file, so it covers the pass-through path as well as anything else that reaches the writer. Reading stays untouched, and so does compute over an array that already has lower parts. `serializing_read_lower_parts_is_gated` pins it, going through `deserialize` to obtain the array exactly as opening a file would, and asserting the write is refused with an error naming the feature. The three wide `test_serde_round_trip` cases move to a feature-gated variant, since without the feature serializing them is now the refusal being tested. Note this makes a stable build unable to rewrite a wide array it just read, so copying or compacting such a file fails loudly rather than producing something old readers cannot open. That is the intended trade-off while the format is unstable, but it is a behaviour change for read-modify-write on files written with the feature. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Uses `hegeltest`, the Hypothesis-based property testing crate that `spiraldb/fastlanes` already depends on, so the two repositories share a generator vocabulary and shrinking behaviour. Every property has the same shape: the encoding must be indistinguishable from doing the same thing to the canonical `DecimalArray`. Seven of them cover split/assemble round tripping, a serialize/decode round trip through the file path, per-row `scalar_at` against bulk canonicalization, and filter, slice and take. The properties were checked against deliberate mutations rather than assumed to be load bearing. Six real mutations, all caught: reversing the lower-part order in assembly, placing the MSP one word too low, swapping the `i256` word-pair packing, dropping the lower part on the `i128` path, reversing the order in `split_i256`, and dropping the sign fill. A seventh — logical instead of arithmetic shift in `split_i128` — is an equivalent mutant, since truncating to `i64` makes the shift kind unobservable, and is correctly not flagged. Dropping the sign fill initially survived, which is why `msp_below_the_top_word_sign_extends` exists. `split_decimal` always emits three lower parts for an `i256`, and at three parts every word is written, so the fill is dead on that path — the round-trip properties structurally cannot reach it. Only a directly constructed array with a most significant part below the top word does. That property computes its expectation independently of the assembly loop: with two lower parts the MSP occupies bits 191..128, exactly the low half of an `i256`'s signed `i128` half, so `i128::from` performs the sign extension the encoding is supposed to. The test target declares `required-features = ["unstable_encodings"]`, since building multi-part arrays is what the write gate restricts. Running the properties writes an example database to `.hegel/`, which is generated state rather than source — the same role `.hypothesis/` plays for Python, and ignored alongside it. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Two properties remain, one starting from each side. Generating a decoded decimal and checking encode-then-decode reproduces it covers what `split_decimal` emits. Generating an encoded array directly and checking decode-then-encode preserves the values it decodes to reaches layouts `split_decimal` never produces — it only ever emits 0, 1 or 3 lower parts under an `i64` most significant part, so drawing the part count is the only way to reach the two-part shape. The second compares decoded values rather than the arrays, because re-encoding normalizes the part count: splitting an `i256` always yields three lower parts whatever the original carried. The removed properties are recorded as a TODO rather than dropped silently, since some of them caught mutations these two do not. Verified rather than assumed: reversing the lower-part order and placing the MSP one word too low are still caught, but dropping the sign fill in `sign_extended_words` now survives both. A round trip compares decode against decode, so a decode-side sign-extension bug is invisible to it — catching that needs an oracle computed independently of the assembly loop, which is what the removed property had. The TODO says so explicitly. Also pins the one-limb invariant against the last way of reaching it. `ArrayParts` is public and `DecimalBytePartsData` is a public unit struct, so slots can be assembled by hand and passed to `Array::try_from_parts`, bypassing the gated constructor. That path stays open deliberately — it is the shape a file read produces, and closing it would stop a build without the feature reading a file written by one with it — but `hand_assembled_lower_parts_cannot_be_serialized` pins that such an array can never be turned back into bytes. So without `unstable_encodings`: `try_new` builds one limb, `try_new_with_lower_parts` refuses more, the compressor declines to split, and anything holding more than one limb — however it was obtained — cannot be serialized. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Three changes that move the lower-parts restriction to where it belongs. The compressor now only handles decimals that fit a single signed part. Anything still wider than 64 bits after narrowing is left as the canonical decimal, unconditionally — this is a property of the width, not of a feature, so `num_children` is back to 1 and the lower-part compression loop is gone. That restores the pre-lower-parts behaviour permanently rather than behind a flag. `serialize` keeps refusing an array carrying lower parts without `unstable_encodings`, and is now the only gate. It is the right place for it: the write allow-list checks the encoding id rather than the child count, so this is the single point every path to a file passes through, whether the array came from a constructor, a compute kernel, hand-assembled slots, or a previous read. The constructor limb check is removed. Building lower parts in memory is allowed again, which is what reading a file needs anyway, so `rebuild_with_lower_parts` — which existed only to bypass that check — disappears with it; `map_parts`, `with_msp` and the test helpers go back to the public constructor. Everything else `validate` enforces is untouched: signedness, dtypes, lengths, the part-count bound, and the width-against- precision check all still apply. Because the compressor no longer splits, the btrblocks tests asserting one and three lower parts are replaced by one asserting wide values are left canonical, and the compression-ratio test is dropped — it measured the win from splitting, which no longer happens. The vortex-file round trip loses its ratio assertion for the same reason but is now ungated, so wide decimals are covered by a default `cargo test` instead of only under the feature. The property tests and the benchmark no longer need the feature either, since neither serializes. `unstable_encodings` now reaches the encoding through `vortex` and `vortex-file` rather than `vortex-btrblocks`, which no longer has an opinion about lower parts. Verified end to end: a default `generate` produces 35 fixtures, and 36 with the feature, `check --mode exact` passing in both. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
`taplo fmt --check` failed on the workspace manifest and on `vortex-btrblocks`: the `hegeltest` workspace dependency was inserted out of alphabetical order, and the `unstable_encodings` feature array stayed expanded across lines after an entry was removed from it, where taplo collapses a short array onto one line. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…e-parts-pr-p0ugog
Merging develop brought `trace_tests`, which snapshots the optimizer trace for a compressed lineitem scan. Two entries move, both because of changes in this branch, and both are the intended behaviour rather than a drift in output. The filter trace names `FilterReduceAdaptor(DecimalByteParts)` where it named `DecimalBytePartsFilterPushDownRule`. That rule was byte-for-byte what the adaptor already did and was registered ahead of it, so removing it leaves the adaptor to do the same work under its own name. Parent, child and result in the trace line are unchanged. The take trace gains three lines: the decimal column now reduces during optimization rather than being deferred to execution, which is exactly what moving `take` from `TakeExecute` to `TakeReduce` was for. The trace is the first end-to-end evidence of that change — the encoding's own tests can only observe it one array at a time. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
Two independent mechanisms keep a multi-limb array out of a file, and only one of them was covered. `serialize` refusing outright was tested in the encoding crate; this adds the file-level check. Handing the writer an already-encoded wide array — bypassing both the compressor's width limit and any constructor — the column lands as a canonical decimal with no children, because the writer recompresses its input and the decimal scheme declines to split values too wide for a single signed part. Verified by inspecting the encodings read back, not just by the write succeeding. Worth recording which mechanism applies where: through the file writer the array is recompressed, so `serialize` is never reached with lower parts present. The refusal in `serialize` is the backstop for a write strategy that does not recompress, where a pre-encoded array would otherwise pass through the allow-list, which checks the encoding id and not the child count. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
…e-parts-editions-uhm1sw Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk>
The unstable_encodings gate inside `serialize` refused to turn lower parts into bytes because the frozen `vortex.decimal_byte_parts` format promises a single child. That put the compatibility decision in the wrong layer: a compile-time feature of one crate, invisible to the editions system that exists to answer exactly this question, and enforced by content inspection where every other write restriction is an id check. Editions gate serialized formats — the id in the file plus its metadata and child semantics — so give the wide layout its own format id instead. `VTable::serialized_id` (with an `ArrayPlugin` counterpart and a session resolver) lets an encoding pick the format written for a given array, defaulting to its encoding id; the flatbuffer writer now interns that id, so the writer's permitted-encoding check gates the bytes a reader will meet rather than the in-memory representation. `DecimalByteParts` keeps one in-memory encoding. Single-part arrays serialize under the frozen id, byte-identical to every file written since core2025.05. Arrays carrying lower parts serialize under `vortex.decimal_byte_parts_wide`, which joins the new unstable2026.08 draft edition — enabled by the default session exactly when the unstable_encodings feature is, so the facade behaviour is unchanged while the mechanism moves into editions. Both ids deserialize into the same encoding through a registered wide plugin; a reader that predates lower parts now fails with the unknown-encoding error editions promise instead of a gate error at write time on someone else's machine. The compressor is untouched: the decimal scheme still declines to split values too wide for one signed part, so the default writer never produces the wide format. Tests move accordingly — the encoding crate pins that the serialized id tracks the parts and that a context restricted to the frozen format refuses a multi-limb array however it was assembled, and the vortex editions tests pin end to end that a flat (non-recompressing) write is refused without the unstable edition and round-trips with it, lower parts intact, in-memory id unchanged. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5gep1Ah7rjHkDuyfrgKCD
… Debug Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5gep1Ah7rjHkDuyfrgKCD
The unstable arm of the wide-format editions test uses assert_arrays_eq, which vortex-array only exports with the _test-harness feature; mirror vortex-file's dev-dependency. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5gep1Ah7rjHkDuyfrgKCD
Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5gep1Ah7rjHkDuyfrgKCD
Serialized format revisions form a version chain on one base name — vortex.decimal_byte_parts, then vortex.decimal_byte_parts_v2 — so each frozen format has exactly one successor and an encoding's serialized history reads as a list. A descriptive name like "wide" invites sibling variants; the next incompatible revision would fork the naming rather than extend it. Rename the plugin to DecimalBytePartsV2, the id to vortex.decimal_byte_parts_v2, and the tests and comments to match. No behavioural change; the id has never shipped. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5gep1Ah7rjHkDuyfrgKCD
joseph-isaacs
changed the title
A fixture's name freezes on first publish, and this one has not shipped yet; name it after the v2 serialized format it covers. Signed-off-by: Joe Isaacs <joe.isaacs@live.co.uk> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01C5gep1Ah7rjHkDuyfrgKCD
Merging this PR will degrade performance by 14.93%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | decompress[u64, (1000, 16)] |
72.5 µs | 133.9 µs | -45.85% |
| ❌ | Simulation | search_index_in_range_chunked |
5.4 ms | 6.3 ms | -14.14% |
| ❌ | Simulation | search_index_above_max_chunked |
4.8 ms | 5.5 ms | -13.1% |
| ❌ | Simulation | search_index_mixed_out_of_range_chunked |
3.9 ms | 4.3 ms | -10.9% |
| ❌ | Simulation | search_index_below_min_chunked |
3.8 ms | 4.3 ms | -10.9% |
| ❌ | Simulation | search_index_full_range_random_chunked |
4 ms | 4.5 ms | -10.52% |
| ⚡ | Simulation | compress_fsst[(1000, 64, 8)] |
1.2 ms | 1 ms | +12.31% |
| 🆕 | Simulation | canonicalize_byte_parts[1] |
N/A | 2.4 ms | N/A |
| 🆕 | Simulation | canonicalize_byte_parts[3] |
N/A | 6.9 ms | N/A |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/decimal-byte-parts-editions-uhm1sw (d9c6f94) with develop (d443eab)
Footnotes
-
8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
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
This PR carries all of #9119 (wide decimals in
DecimalBytePartsvia 64-bit lower parts) and replaces its write gate. There, serializing an array with lower parts was refused by acfg!(unstable_encodings)check insideVTable::serialize— a compile-time feature of one crate, enforced by content inspection, invisible to the editions system that exists to answer exactly this compatibility question.The replacement applies the model documented in #9275: editions gate serialized formats, and an incompatible extension of a frozen format is a new format id, not a mutation of the frozen one.
vortex.decimal_byte_partsfroze incore2025.05.0promising a single child, so an array carrying lower parts serializes under the next link in the format's version chain,vortex.decimal_byte_parts_v2, staged in a newunstable2026.08.0draft edition. Revisions take sequential version suffixes on one base name so each format has exactly one successor — a list of revisions, never a tree of named variants. The in-memory encoding does not fork — both ids deserialize into the sameDecimalBytePartsArray.Facade behavior is unchanged: the default session enables the newest unstable edition exactly when the
unstable_encodingsfeature is on, so who can write lower parts is the same as in #9119 — but the mechanism is now an id in an edition, visible to the registry, the spec, and third-party readers, and the failure mode for an old reader is the unknown-encoding error editions promise instead of a decode-time surprise.What changes are included in this PR?
Core hook (
vortex-array).VTable::serialized_id(default: the encoding id) lets an encoding pick the serialized format for a given array, with anArrayPlugin::serialized_idcounterpart and a session-levelarray_serialized_idresolver. The flatbuffer writer interns the serialized id instead of the in-memory encoding id, so the writer's permitted-encoding check gates the bytes a reader will meet. Every existing encoding is unaffected — the default keeps serialized id == encoding id.Encoding (
vortex-decimal-byte-parts). Theunstable_encodingsfeature and the refusal insideserializeare gone.DecimalByteParts::serialized_idreturns the frozen id for single-part arrays (byte-identical output to today) and the v2 id when lower parts are present. A newDecimalBytePartsV2plugin registers the v2 id for reading and deserializes it into the same in-memory encoding — the same pattern as the ALP-patches plugin.Editions (
vortex). Newunstable2026.08.0draft declaringvortex.decimal_byte_parts_v2;DEFAULT_UNSTABLE_EDITIONmoves to it.Compressor. Untouched from #9119: the decimal scheme still declines to split values too wide for one signed part, so the default writer never produces the v2 format.
Tests.
tests/format_v2.rs(renamed fromlower_parts_gate.rs): construction is never gated; the serialized id tracks the parts; a context restricted to the frozen format refuses a hand-assembled multi-limb array, and permitting the v2 id is exactly what lets the same array through.vortexeditions tests: the v2 id joins the family at 2026.08 and not before; end-to-end, a flat (non-recompressing) write of a multi-limb array is refused by the default session withnot permitted by ctx, and with the unstable edition enabled it round-trips — lower parts intact, in-memory id stillvortex.decimal_byte_parts.vortex-file: the recompression test is no longer feature-gated (the behavior never depended on the feature).Checks run:
cargo buildfor the five touched crates;cargo test -p vortex-decimal-byte-parts(all suites),cargo test -p vortex editionswith and without--features unstable_encodings(20 passed each), targetedvortex-fileandvortex-btrblocksdecimal tests;cargo +nightly fmt --all;cargo clippy --all-targetson the touched crates (clean). Not run: workspace-wide clippy with--all-features(pulls CUDA targets unavailable in this environment) and the fullvortex-file/vortex-arraysuites beyond the targeted tests.What APIs are changed? Are there any user-facing changes?
New public API:
VTable::serialized_id/ArrayPlugin::serialized_id(defaulted — no action needed for existing encodings)ArraySessionExt::array_serialized_idvortex_decimal_byte_parts::DecimalBytePartsV2vortex::editions::UNSTABLE_2026_08_0Removed: the
unstable_encodingscargo feature ofvortex-decimal-byte-parts(and its propagation lines invortex/vortex-file). Anyone enablingvortex/unstable_encodingsgets the same write capability as before, now expressed through editions.File-format impact: single-part byte-parts arrays serialize byte-identically to today under the frozen id. The v2 id only ever appears in files written with an edition that contains it.
Supersedes the gating approach in #9119; the encoding work is unchanged and carried here. Documentation for the underlying model is in #9275.
AI Assistance
Written with Claude Code (agentic), on top of #9119. All commits are signed off under the DCO.