feat(array): let callers choose a nested builder's chunk threshold [builders-child-stack] - #8965
Conversation
2e987a3 to
b4cb8fb
Compare
cc36904 to
6f3ef13
Compare
Merging this PR will not alter performance
Comparing Footnotes
|
`append_listview_array` rebuilt every incoming `ListViewArray` into an exact layout before appending it. Rebasing offsets by the number of elements already in the builder is correct whatever layout they have, so the rebuild bought nothing except an unconditional promise that the finished array is zero-copyable to a `ListArray` - and it cost the caller any sharing the source expressed. A constant list array is the case that matters: canonicalizing one already points every view at a single copy of the value, and flattening it materialized one copy per row. Appending a 10,000-row constant list of three elements produced 30,000 elements; it now produces 3. Keep trimming unreferenced elements, but otherwise append the views as they arrived and track whether the result is still zero-copyable to a `ListArray` instead of asserting it. The flag is per-array and consumers already branch on it, so callers that need an exact layout can rebuild. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <robert@spiraldb.com>
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>
`typos` splits `mis-sliced` on the hyphen and flags `mis`. Signed-off-by: Claude <noreply@anthropic.com>
`ChildBuilder` copies appended arrays shorter than 64 values instead of giving them a chunk, which is right for the nested builders that append the elements of a single list, and wrong for a caller whose appended arrays are themselves chunks worth preserving. Add `ArrayBuilder::set_min_chunk_len`, a no-op for builders without array children, so such a caller can lower the threshold — to zero to keep every chunk boundary. The threshold is read when an array is appended and applies transitively to the children of a builder's children. Signed-off-by: Claude <noreply@anthropic.com>
b4cb8fb to
c78ed3c
Compare
6f3ef13 to
20dd597
Compare
c78ed3c to
e45996f
Compare
|
Superseded: The looping callers that motivated the threshold — the sparse fill loops and |
Rationale for this change
Stacked on #8964 — review that one first.
ChildBuildercopies appended arrays shorter than 64 values instead of giving each one a chunk. That default is right for the callers it was written for: nested builders routinely append the elements of a single list, and a chunk per list would produce aChunkedArraywith more chunks than values.It is wrong for a caller whose appended arrays are already chunks whose identity is the point. The motivating one is
ChunkedArray's canonicalization, which today hand-rollsswizzle_list_chunksand friends precisely to reuse each chunk's children without copying.The argument for the knob is compatibility rather than measured performance. Canonicalizing a
ChunkedArraycurrently preserves every chunk boundary in the children, at any chunk size; letting the default threshold apply would silently coalesce chunks under 64 rows into a copy. Passing0reproduces today's behaviour exactly, which is what a refactor should do. Whether sub-64-row chunks are common enough at canonicalization time for that to matter is unmeasured — I instrumented the threshold across eleven crates' test suites and every production-shaped append (7232 and 8192 rows, file scan batches) chunked under either setting; the only sub-64 chunks observed came from tests.What changes are included in this PR?
ArrayBuilder::set_min_chunk_len, defaulting to a no-op so builders without array children ignore it.StructBuilder,ListBuilder,ListViewBuilder,FixedSizeListBuilderandExtensionBuilderforward it to theirChildBuilders, which hold the threshold as a field instead of reading the const.Two semantics worth stating, both documented on the trait method:
ChildBuilder's scalar builder is itself a nested builder with children of its own, so it gets the setting too.Three tests: a threshold of zero keeps every chunk boundary at the
ChildBuilderlevel and through the publicbuilder_with_capacity, and the transitive case is checked with a list-typed child short enough to be materialized whose elements would otherwise have earned a chunk.What APIs are changed? Are there any user-facing changes?
One new
ArrayBuildertrait method with a default implementation, so external implementors are unaffected. No behaviour change for anyone who does not call it:DEFAULT_MIN_CHUNK_LENis the previous constant.Checks
cargo test -p vortex-array— 3167 lib testscargo +nightly fmt --all,cargo clippy -p vortex-array --all-targets --all-featuresGenerated by Claude Code
Stacked PR Chain: builders-child-stack
Generated by Claude Code