Support wide decimals in DecimalByteParts with 64-bit lower parts - #9119
Draft
joseph-isaacs wants to merge 11 commits into
Draft
Support wide decimals in DecimalByteParts with 64-bit lower parts#9119joseph-isaacs wants to merge 11 commits into
joseph-isaacs wants to merge 11 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>
Contributor
|
the funky part with this that I was debating at some point is that we can support i192 decimals, not sure how often it happens though |
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>
Merging this PR will improve performance by 12.27%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | decompress[u64, (10000, 256)] |
62.1 µs | 55.3 µs | +12.27% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/decimal-byte-parts-pr-p0ugog (8c5cd41) with develop (3239a5c)
Footnotes
-
1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports. ↩
…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>
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
3 participants
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 extends
DecimalBytePartsto support decimals wider than 64 bits by splitting them into a signed most significant part (MSP) plus unsigned 64-bit lower parts. Previously, the encoding only supported single-part decimals that fit in a signed integer.The change enables efficient compression of
i128andi256decimal values by:This is part of the broader decimal compression effort and allows the
DecimalSchemecompressor to handle the full range of Vortex decimal types.What changes are included in this PR?
Core changes:
New
limbs.rsmodule: Implements splitting/reassembling logic fori128andi256decimals into 64-bit partssplit_decimal(): Decomposes canonical decimals into MSP + lower partsassemble_decimal(): Reconstructs canonical decimals from partscombine_i128()/combine_i256(): Single-row reassembly helpersassembled_values_type(): Determines output decimal type from part configurationUpdated
DecimalBytePartsSlots: Now carries multiple lower parts via aVec<ArrayRef>instead of being emptyu64, non-nullable)Updated
DecimalBytePartsData: Simplified to a unit struct (all data lives in slots)validate()now checks all parts match expected types and lengthstry_new()in favor oftry_new_with_lower_parts()Updated serialization:
lower_part_countmetadata field now properly populated and validated during encode/decodeCompute kernels updated:
filter,take,slice,cast,masknow handle lower parts correctlyScalar extraction:
execute_scalar()now reassembles multi-part values usingcombine_i128()/combine_i256()Canonical conversion:
to_canonical_decimal()now callsassemble_decimal()to handle all part countsTesting:
testing.rsmodule with helpers:encode(),i128_parts(),i256_parts(),i256_of()i128andi256vortex-fileandvortex-btrblocksverifying compression of wide decimalsAPI changes:
split_decimal,DecimalParts,LOWER_PART_DTYPE,MAX_LOWER_PARTS,assembled_values_typeDecimalByteParts::try_new_with_lower_parts(msp, lower_parts, decimal_dtype)DecimalByteParts::try_new()now delegates to the new constructor with empty lower partsWhat APIs are changed? Are there any user-facing changes?
Public API additions:
split_decimal(decimal: &DecimalArray) -> VortexResult<DecimalParts>— splits canonical decimals into partsDecimalPartsstruct — holds MSP and lower partsDecimalByteParts::try_new_with_lower_parts()— constructor accepting lower partsMAX_LOWER_PARTS,LOWER_PART_DTYPEhttps://claude.ai/code/session_01JfoQz1AGVNidakVfEDQ2BS