feat(array): builders no longer canonicalize their children [builders-child-stack] - #8964
Conversation
Merging this PR will improve performance by 59.28%
Performance Changes
Tip Curious why this is faster? Comment Comparing Footnotes
|
2e987a3 to
b4cb8fb
Compare
b4cb8fb to
c78ed3c
Compare
c78ed3c to
e45996f
Compare
e45996f to
df75205
Compare
df75205 to
c30fa2a
Compare
e8186e0 to
fe7132d
Compare
fe7132d to
9b0fd4b
Compare
|
Force-pushed: the branch did not compile (which is why every job on this PR and the two above it was red). The rebase kept two call sites that hand
|
9b0fd4b to
bc03a46
Compare
bc03a46 to
79dc1ec
Compare
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. However short the appended array, it becomes a chunk. Deciding on the caller's behalf that its values are cheaper copied than referenced would be guessing at a boundary only the caller can see, and a caller that wants them copied has `append_scalar`. A child is therefore chunked on exactly the boundaries it was appended on. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <robert@spiraldb.com> Signed-off-by: Robert Kruszewski <github@robertk.io> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
79dc1ec to
c9bacae
Compare
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.Third PR in the stack, after #9046 and #9064. 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.However short the appended array, it becomes a chunk. An earlier revision of this PR kept a
MIN_CHUNK_LEN(64) below which arrays were copied into the scalar builder instead, because some callers append one list at a time — the load-bearing case beingSparseArraycanonicalization, whose fill loop calledappend_array_as_list(fill_elements, ctx)once per filled row and would have produced a chunk per row.That threshold is gone. Deciding on the caller's behalf that its values are cheaper copied than referenced is guessing at a boundary only the caller can see, and a caller that wants them copied already has
append_scalar. #9064 converts the looping callers that motivated the threshold — the sparse fill loops, the sparse patch loops, andListBuilder::extend_from_listview— to bulk appends, so a child is now chunked on exactly the boundaries it was appended on.A test in
encodings/sparsepins that down where it is observable: 100 patches on consecutive rows of a 200-row sparse list array produce an elements child of exactly 2 chunks — one for the patch run, one for the trailing gap. Without the run coalescing in #9064 it is 101.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 nextest run --workspace— 7168 passed, 560 skippedcargo clippy --all-targets --all-features— cleancargo +nightly fmt --all --check— cleancargo test --doc -p vortex-array -p vortex-sparse— passedOne existing test needed a fix rather than an update to match:
uncompressed_size_in_bytes::list_matches_materialized_sizecompares the aggregate againstbuilder.finish().nbytes(), and a builder that keeps a dict-encoded child no longer measures anything "materialized". Its helper now finishes the round-trip withRecursiveCanonicalbefore takingnbytes, which is what the name always promised.Generated by Claude Code
Stacked PR Chain: builders-child-stack
Generated by Claude Code