Skip to content

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

Open
gatesn wants to merge 5 commits into
developfrom
ngates/buffer-allocator-builders
Open

refactor(array): propagate allocators through builders#9670
gatesn wants to merge 5 commits into
developfrom
ngates/buffer-allocator-builders

Conversation

@gatesn

@gatesn gatesn commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Require every array builder caller to choose its buffer allocator explicitly.

Changes

  • Replace implicit and _in constructor pairs with one allocator-taking API.
  • Borrow allocator handles at API boundaries; clone only when a buffer or builder retains them.
  • 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 regress 1 benchmark

⚠️ 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.

⚡ 5 improved benchmarks
❌ 1 regressed benchmark
✅ 2166 untouched benchmarks
⏩ 206 skipped benchmarks1

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation chunked_bool_canonical_into[(1000, 10)] 27.1 µs 30.8 µs -12.25%
WallTime arrow_checked_add_u32_neon[16384] 20.5 µs 12.8 µs +59.85%
Simulation decompress[u64, (4000, 1024)] 85.6 µs 71.2 µs +20.3%
Simulation allocate_drop_arrow[0] 456.9 ns 402.7 ns +13.45%
Simulation compact_sliced[(1024, 90)] 1.5 µs 1.4 µs +12.03%
Simulation compact_sliced[(2048, 90)] 1.6 µs 1.4 µs +11.53%

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 (69a9f2e) with develop (792cef7)

Open in CodSpeed

Footnotes

  1. 206 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 0ce40e7 to 9076b5d Compare September 2, 2026 14:44
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from 9076b5d to 66cd9f7 Compare September 2, 2026 14:47
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from 66cd9f7 to 4e3ecb4 Compare September 2, 2026 15:35
@gatesn
gatesn force-pushed the ngates/buffer-allocator-builders branch from 4e3ecb4 to 4fe4b22 Compare September 2, 2026 16:10
@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

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