Skip to content

feat(array): let callers choose a nested builder's chunk threshold [builders-child-stack]#8965

Open
robert3005 wants to merge 1 commit into
claude/builders-canonical-children-9ze0t6from
claude/builders-min-chunk-len-9ze0t6
Open

feat(array): let callers choose a nested builder's chunk threshold [builders-child-stack]#8965
robert3005 wants to merge 1 commit into
claude/builders-canonical-children-9ze0t6from
claude/builders-min-chunk-len-9ze0t6

Conversation

@robert3005

@robert3005 robert3005 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Rationale for this change

Stacked on #8964 — review that one first.

ChildBuilder copies 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 a ChunkedArray with 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-rolls swizzle_list_chunks and friends precisely to reuse each chunk's children without copying. A filtered scan can easily produce chunks of well under 64 rows, and routing that through a builder with the default threshold would copy every child.

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, FixedSizeListBuilder and ExtensionBuilder forward it to their ChildBuilders, which hold the threshold as a field instead of reading the const.

Two semantics worth stating, both documented on the trait method:

  • The threshold is read when an array is appended, so it only affects subsequent appends — no "call this before anything else" ordering trap.
  • It applies transitively. A 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 ChildBuilder level and through the public builder_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 ArrayBuilder trait method with a default implementation, so external implementors are unaffected. No behaviour change for anyone who does not call it: DEFAULT_MIN_CHUNK_LEN is the previous constant.

Checks

  • cargo test -p vortex-array — 3167 lib tests
  • cargo +nightly fmt --all, cargo clippy -p vortex-array --all-targets --all-features

Generated by Claude Code

Stacked PR Chain: builders-child-stack

PR Title Merges Into
#8964 feat(array): builders no longer canonicalize their children [builders-child-stack] N/A
#8965 feat(array): let callers choose a nested builder's chunk threshold [builders-child-stack] #8964
#8966 perf(array): accumulate nested builder validity without a null buffer [builders-child-stack] #8965
#8967 refactor(array): canonicalize chunked structs and FSLs through the builder [builders-child-stack] #8966

Generated by Claude Code

@claude claude Bot changed the title feat(array): let callers choose a nested builder's chunk threshold feat(array): let callers choose a nested builder's chunk threshold [builders-child-stack] Jul 25, 2026
`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>
@robert3005
robert3005 force-pushed the claude/builders-canonical-children-9ze0t6 branch from 2e987a3 to b4cb8fb Compare July 25, 2026 21:07
@robert3005
robert3005 force-pushed the claude/builders-min-chunk-len-9ze0t6 branch from cc36904 to 6f3ef13 Compare July 25, 2026 21:07
@codspeed-hq

codspeed-hq Bot commented Jul 25, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚡ 8 improved benchmarks
❌ 1 regressed benchmark
✅ 1840 untouched benchmarks
⏩ 46 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation canonicalize_sparse_fixed_size_list[(1024, 17, 8)] 327.3 µs 365.2 µs -10.4%
Simulation extend_from_array_zctl[(1000, 64)] 1,193.7 µs 513.7 µs ×2.3
Simulation extend_from_array_zctl[(10000, 8)] 2.2 ms 1.4 ms +59.5%
Simulation extend_from_array_zctl[(1000, 8)] 401.9 µs 307.8 µs +30.55%
Simulation extend_from_array_non_zctl_overlapping[(1000, 32)] 893.7 µs 704.6 µs +26.84%
Simulation decode_varbin[(1000, 8)] 34.1 µs 27 µs +26.08%
Simulation extend_from_array_non_zctl_overlapping[(10000, 8)] 2.5 ms 2 ms +24.09%
Simulation extend_from_array_non_zctl_overlapping[(1000, 8)] 437.8 µs 382.6 µs +14.41%
Simulation fsl_large 99.2 µs 88.5 µs +12.16%

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-min-chunk-len-9ze0t6 (6f3ef13) with develop (a12c310)2

Open in CodSpeed

Footnotes

  1. 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.

  2. No successful run was found on claude/builders-canonical-children-9ze0t6 (b4cb8fb) during the generation of this report, so develop (a12c310) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants