Canonicalize chunked nested types through the builder [builders-child-stack] - #8967
Conversation
Merging this PR will degrade performance by 31.44%
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing Footnotes
|
afed52f to
79f350a
Compare
1145029 to
8f39b06
Compare
8f39b06 to
825ab2e
Compare
825ab2e to
ce2eb1d
Compare
| Canonical::FixedSizeList(swizzle_fixed_size_list_chunks( | ||
| DType::List(elem_dtype, _) => { | ||
| let owned_chunks: Vec<ArrayRef> = array.iter_chunks().cloned().collect(); | ||
| Canonical::List(swizzle_list_chunks( |
There was a problem hiding this comment.
this is unnecessary as well
There was a problem hiding this comment.
Done — swizzle_list_chunks is gone too, so Variant is the only dtype left with a hand-written pack (there is no variant builder). The generic builder path produces the same array: append_listview_array / extend_from_list hand each chunk’s elements to the ChildBuilder, which keeps them as chunks of the combined elements. Added a case::list to canonicalize_reuses_chunk_children covering it.
One consequence worth flagging: swizzle_list_chunks was the last canonicalization path that allocated through the session allocator (ctx.allocator().allocate_typed for offsets/sizes). builder_with_capacity_in still ignores the allocator it is handed, so chunked primitives, structs and FSLs already went around it — lists were the odd one out. I removed list_canonicalize_uses_memory_session_allocator with the swizzle rather than keep a bespoke path alive for it; restoring the property means teaching the builders to allocate through a HostAllocatorRef. Happy to file that as a follow-up if you want it tracked.
ce2eb1d to
cefbc8e
Compare
cefbc8e to
0d4eb9a
Compare
0d4eb9a to
1e8346a
Compare
1e8346a to
0431fa7
Compare
0431fa7 to
a9f7d31
Compare
`pack_struct_chunks`, `swizzle_fixed_size_list_chunks` and `swizzle_list_chunks` existed because `append_to_builder` used to decode a builder's children: without them, canonicalizing a `ChunkedArray` would concatenate every chunk's children instead of reusing them. Their doc comments describe what the builders now do on their own, so all three are dead weight - the generic builder path produces the same swizzled array, with each chunk's children kept as chunks of the combined child. Only `Variant` still needs a hand-written pack, because there is no variant builder. This drops the one canonicalization path that allocated its buffers through the session allocator: `swizzle_list_chunks` allocated its offsets and sizes with `ctx.allocator()`, whereas `builder_with_capacity_in` still ignores the allocator it is handed, so chunked primitives, structs and FSLs already went around it. `list_canonicalize_uses_memory_session_allocator` guarded that one path and goes with it; restoring the property means teaching the builders to allocate through a `HostAllocatorRef`, not keeping a bespoke list swizzle alive. Signed-off-by: Claude <noreply@anthropic.com> Signed-off-by: Robert Kruszewski <robert@spiraldb.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a9f7d31 to
9c8574c
Compare
Rationale for this change
Stacked on #8966 → #8964 → #9064 → #9046 — review those first. This is the payoff for the rest of the stack.
ChunkedArray's_canonicalizecarried four hand-written special cases plus a genericappend_to_builderfallback. Two of them existed purely becauseappend_to_builderused to decode a builder's children; their doc comments say so:pack_struct_chunks— "a singleStructArray, where the data for each field is aChunkedArray"swizzle_fixed_size_list_chunks— "reuse the chunks'elementschildren directly as the chunks of a combinedelementsarray without copying"That is what
ChildBuilderdoes now, so both are dead weight — an extra path to keep correct for a result the generic path already produces.What changes are included in this PR?
DType::StructandDType::FixedSizeListfall through to the builder, and both helpers are deleted. Nothing has to be configured for this to work: since #8964 a nested builder keeps every appended chunk however short, which is the behaviour the swizzles hand-rolled. An earlier revision of this PR calledbuilder.set_min_chunk_len(0)here to opt out of a length threshold that no longer exists.I verified the shapes match before deleting anything. A
tree_displayprobe over chunked struct / list / FSL inputs produces byte-identical trees on both paths for chunks of 200 rows — struct field isvortex.chunked(i32, len=400)with two chunks, FSLelementslikewise. Two newrstestcases lock that in using chunks of one row.What is not included
ListArraykeepsswizzle_list_chunks. The builder produces the same shape —is_zero_copy_to_list: trueand all — but the swizzle also allocates its offsets and sizes throughctx.allocator(), and builders are not allocator-aware:builder_with_capacity_intakes aHostAllocatorRefand discards it (let _allocator = allocator;).list_canonicalize_uses_memory_session_allocatorcatches exactly this. Making it work means threading an allocator throughBufferMutinvortex-buffer, which has none today — a bigger and separate piece of work than this stack, so the list swizzle stays until someone does it.pack_variant_chunksstays too:builder_with_capacityisunimplemented!()forDType::Variant.StructArray::try_concatis now unused in-tree. It ispub, and a useful helper in its own right, so I left it — removing public API seemed like a separate call for a reviewer to make rather than something to slip into this PR.What APIs are changed? Are there any user-facing changes?
No API changes, and no observable difference: the generic builder path preserves every chunk boundary, which is exactly what the swizzles did.
Checks
cargo nextest run --workspace— 7168 passed, 560 skippedcargo clippy --all-targets --all-features— cleancargo +nightly fmt --all --check— cleanGenerated by Claude Code
Stacked PR Chain: builders-child-stack
Generated by Claude Code