Skip to content

refactor(array): propagate allocators through builders - #9670

Merged
gatesn merged 8 commits into
developfrom
ngates/buffer-allocator-builders
Sep 9, 2026
Merged

refactor(array): propagate allocators through builders#9670
gatesn merged 8 commits into
developfrom
ngates/buffer-allocator-builders

Conversation

@gatesn

@gatesn gatesn commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Require array builder call sites to choose a buffer allocator explicitly while keeping the old APIs as deprecated compatibility shims.

Changes

  • Add allocator-taking _in APIs and deprecate the allocator-free builder constructors.
  • Take &BufferAllocatorRef at API boundaries; clone only when a buffer or builder retains it.
  • Propagate the allocator through nested, dictionary, and offset-based string builders.
  • Use ctx.allocator() in execution paths; use the static allocator only where no execution context exists.
  • Add custom allocator coverage.

@gatesn gatesn mentioned this pull request Aug 27, 2026
5 tasks
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch 2 times, most recently from 9f76879 to cb223eb Compare August 27, 2026 20:42
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from cb223eb to dd4f747 Compare August 27, 2026 21:37
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from dd4f747 to fd018db Compare August 27, 2026 21:38
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from fd018db to 737f66c Compare August 28, 2026 14:40
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from 737f66c to 08a209a Compare August 28, 2026 14:56
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch 2 times, most recently from 56d0be5 to 4af7bf3 Compare August 28, 2026 17:03
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch 2 times, most recently from 01b019a to de08d68 Compare August 28, 2026 18:02
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from de08d68 to b773833 Compare August 28, 2026 18:23
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from b773833 to 4b92e40 Compare August 28, 2026 18:24
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch 2 times, most recently from 62670ea to f494e5a Compare August 28, 2026 20:50
@gatesn gatesn added the changelog/feature A new feature label Aug 28, 2026
@codspeed-hq

codspeed-hq Bot commented Aug 28, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 8.22%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 3 regressed benchmarks
✅ 2183 untouched benchmarks
⏩ 218 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation decompress[u64, (4000, 1024)] 70.8 µs 86 µs -17.63%
WallTime filtered_owned_i64_avx512[OneNullInEight] 22.4 µs 26.1 µs -14.24%
Simulation chunked_bool_canonical_into[(1000, 10)] 27.1 µs 30.6 µs -11.47%
Simulation allocate_drop_arrow[0] 456.9 ns 402.7 ns +13.45%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ngates/buffer-allocator-builders (542aed9) with develop (bffdca1)

Open in CodSpeed

Footnotes

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

@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch 2 times, most recently from d5d1c39 to fd3a728 Compare September 2, 2026 16:59
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from fd3a728 to cfcc653 Compare September 2, 2026 17:02
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from cfcc653 to f497774 Compare September 2, 2026 17:27
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from f497774 to a8608ae Compare September 2, 2026 21:41
Base automatically changed from ngates/buffer-allocator-context to develop September 2, 2026 23:01
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from a8608ae to f627f7c Compare September 2, 2026 23:02
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
Signed-off-by: Nicholas Gates <nick@nickgates.com>
@gatesn
gatesn marked this pull request as ready for review September 3, 2026 00:19
@robert3005

Copy link
Copy Markdown
Contributor

No breaks please, can you add back old api and mark it deprecated. Users don't want the churn just because you need a new feature.

gatesn commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

I dunno… most callers will have an ExecutionCtx in scope and not pass it. In fact, shouldn't ArrayBuilders hold ExecutionCtx anyway so that you can extend_from_array?

@robert3005

Copy link
Copy Markdown
Contributor

I am purely arguing from the churn POV, it would be ideal if we can avoid forcing people to update their builder usage when they upgrade vortex. I think we can remove it in 1-2 releases

@gatesn
gatesn requested a review from robert3005 September 9, 2026 12:40
Comment thread vortex-array/src/builders/mod.rs Outdated
///
/// Prefer [`builder_with_capacity_in_ref`] when borrowing an allocator already in scope.
#[deprecated(note = "use `builder_with_capacity_in_ref` with a borrowed allocator")]
pub fn builder_with_capacity_in(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think since this method wasn't widely used before we can break this ONE method so that we don't have builder_with_capacity_in_ref

Signed-off-by: Nicholas Gates <nick@nickgates.com>
@gatesn
gatesn merged commit 5c92e4e into develop Sep 9, 2026
84 of 85 checks passed
@gatesn
gatesn deleted the ngates/buffer-allocator-builders branch September 9, 2026 15:48
robert3005 pushed a commit that referenced this pull request Sep 9, 2026
`VarBinBuilder::with_capacity` was deprecated when allocators were
propagated through builders (#9670), after the btrblocks binary scheme
(#9576) was written against the old API. CI denies warnings, so pass the
execution context's allocator instead.

Signed-off-by: "Claude" <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BAC54whRD3iHLBZfM4TCvd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/feature A new feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants