feat(array): builders no longer canonicalize their children [builders-child-stack] - #8964
feat(array): builders no longer canonicalize their children [builders-child-stack]#8964robert3005 wants to merge 3 commits into
Conversation
Nested builders used to push every appended child array through `append_to_builder`, which decoded it into the child's canonical builder. That work is wasted: `Canonical` only promises a canonical *top level*, so struct fields, list elements and extension storage are free to stay compressed. Introduce `ChildBuilder`, which accumulates a child as a `Vec<ArrayRef>` of chunks plus a scalar builder for the values that cannot come from an array, and stitches them into a `ChunkedArray` on `finish` when more than one chunk accumulated. `StructBuilder`, `ListBuilder`, `ListViewBuilder`, `FixedSizeListBuilder` and `ExtensionBuilder` now hold their children this way. Arrays shorter than `MIN_CHUNK_LEN` are still materialized into the scalar builder: nested builders routinely append the elements of a single list, and a chunk per list would cost far more than the copy. Signed-off-by: Claude <noreply@anthropic.com>
The first pass only checked that children survive undecoded. Add coverage for the arithmetic and validity handling that the chunk path changes: - `ChildBuilder`: empty arrays never become chunks, dtype mismatches are rejected on both sides of the length threshold, and scalars, zeros and nulls appended around a chunk keep their order. - `ChildBuilder::set_validity_unchecked`: the override is sliced per chunk, covers values still pending in the scalar builder, is dropped for a non-nullable child, and panics on a chunk that already contains nulls. - List builders: offsets are rebased onto the running element count once the elements live in chunks, for `ListViewArray` and `ListArray` inputs and for `ListBuilder::append_array_as_list`. - Nested builders keep their own validity buffer alongside a chunked child, extension storage receives a validity override through its chunks, and a built array with chunked children still canonicalizes recursively. Each test was checked against a mutated implementation: dropping the per-chunk validity slice, dropping the pending flush, zeroing the listview offset base, and taking the `ListBuilder` offset from the appended array instead of the running total all fail the intended tests. Signed-off-by: Claude <noreply@anthropic.com>
Merging this PR will regress 1 benchmark
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | canonicalize_sparse_fixed_size_list[(1024, 17, 8)] |
327.3 µs | 363.9 µs | -10.06% |
| ⚡ | Simulation | extend_from_array_zctl[(1000, 64)] |
1,193.7 µs | 518.1 µs | ×2.3 |
| ⚡ | Simulation | extend_from_array_zctl[(10000, 8)] |
2.2 ms | 1.4 ms | +58.97% |
| ⚡ | Simulation | extend_from_array_zctl[(1000, 8)] |
401.9 µs | 312.6 µs | +28.55% |
| ⚡ | Simulation | decode_varbin[(1000, 8)] |
34.1 µs | 27 µs | +26.12% |
| ⚡ | Simulation | extend_from_array_non_zctl_overlapping[(1000, 32)] |
893.7 µs | 719.2 µs | +24.26% |
| ⚡ | Simulation | extend_from_array_non_zctl_overlapping[(10000, 8)] |
2.5 ms | 2 ms | +23.83% |
| ⚡ | Simulation | extend_from_array_non_zctl_overlapping[(1000, 8)] |
437.8 µs | 379 µs | +15.5% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing claude/builders-canonical-children-9ze0t6 (b4cb8fb) with develop (a12c310)
Footnotes
-
46 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. ↩
`typos` splits `mis-sliced` on the hyphen and flags `mis`. Signed-off-by: Claude <noreply@anthropic.com>
2e987a3 to
b4cb8fb
Compare
| /// example), and giving each one its own chunk would produce a [`ChunkedArray`] with more chunks | ||
| /// than values. Below this length, copying the values costs less than the indirection that every | ||
| /// later access to the chunk would pay. | ||
| pub(super) const MIN_CHUNK_LEN: usize = 64; |
There was a problem hiding this comment.
I am not sure this is real, need to see which benchmarks regress
Rationale for this change
Canonical form is not recursive —
Canonicalonly promises a canonical top level. Nested builders did not honour that: every appended child array was pushed throughappend_to_builder, which decoded it into the child's canonical builder. Struct fields, list elements and extension storage were all decompressed on the way through a builder that had no reason to look at them.This is the first PR in a stack. The eventual goal is to delete the hand-written
pack_struct_chunks/swizzle_list_chunks/swizzle_fixed_size_list_chunksspecial cases inChunkedArray's canonicalization and route every dtype throughappend_to_builder— those helpers exist only because the builders used to decode children, and their doc comments describe exactly what this PR makes the builders do.What changes are included in this PR?
ChildBuilder(vortex-array/src/builders/child.rs) accumulates a nested builder's child from the two sources such a builder receives:finishstitches the two back together, returning aChunkedArrayonly when more than one chunk accumulated.StructBuilder,ListBuilder,ListViewBuilder,FixedSizeListBuilderandExtensionBuildernow hold their children this way. Those are all the builders with array children;bool/primitive/decimal/varbinview/nullhave none, andUnion/Variantbuilders do not exist yet.Arrays shorter than
MIN_CHUNK_LEN(64) are still materialized. Nested builders routinely append the elements of a single list, and a chunk per list would produce aChunkedArraywith more chunks than values.ChildBuilder::set_validity_uncheckedpushes the override into each chunk as aMaskedArrayrather than decoding. That path panics if a chunk already contains nulls, since replacing its validity would mean decoding it; it is documented on the method.ExtensionBuilderis the only builder that forwardsset_validityinto a child — the others keep their own validity buffer — and the fast path (nothing chunked yet) is unchanged.Tests
22 new tests. Beyond "children survive undecoded" for each nested builder, they cover the arithmetic and validity handling that the chunk path changes:
set_validity_uncheckedslices the override per chunk, covers values still pending in the scalar builder, is dropped for a non-nullable child, and panics on a chunk that already contains nullsListViewArrayandListArrayinputs and forListBuilder::append_array_as_listEach test was checked against a mutated implementation. Dropping the per-chunk validity slice, dropping the pending flush, zeroing the listview offset base, and taking the
ListBuilderoffset from the appended array instead of the running total each fail the intended tests. Reverting to the old always-materialize behaviour fails 17 of them.What APIs are changed? Are there any user-facing changes?
No public API changes —
ChildBuilderispub(crate). The user-facing change is the shape of what builders return:ArrayBuilder::finishis now documented as canonical at the top level only, and callers that need a fully decoded tree should ask for one viaRecursiveCanonical(arrow export and the DuckDB exporter already recurse per child, so both are unaffected).Checks
cargo test -p vortex-array— 3164 lib + 72 doc testscargo test --lib --testsgreen forvortex-arrow,vortex-row,vortex-btrblocks,vortex-compressor,vortex-layout,vortex-scan,vortex-file,vortex-ipc,vortex,vortex-datafusion,vortex-json,vortex-tensor, and all encodingscargo check --all-targetsgreen forvortex-ffi,vortex-jni,vortex-tui,vortex-python-abi,vortex-fuzzcargo +nightly fmt --all,cargo clippy -p vortex-array --all-targets --all-featuresMIN_CHUNK_LENforced to 1 (every append becomes a chunk): 3141/3142 lib tests pass. The one failure,uncompressed_size_in_bytes::list_matches_materialized_size, compares againstbuilder.finish().nbytes()— it measures the builder's compaction, which is what this PR removes. It passes at the real threshold.Not run in this environment:
vortex-duckdbandvortex-sqllogictest(DuckDB source download is blocked), CUDA crates,vortex-bench(lance-encodingneedsprotoc), and the Python suite.Generated by Claude Code
Stacked PR Chain: builders-child-stack
Generated by Claude Code