chore: use Vec instead of OffsetBuilder#23195
Merged
Merged
Conversation
Contributor
Author
|
@alamb FYI, for |
Contributor
|
run benchmark array_remove |
alamb
approved these changes
Jun 25, 2026
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing arrays (4303be4) to a0e9887 (merge-base) diff using: array_remove File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_remove — base (merge-base)
array_remove — branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark array_remove |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing arrays (4303be4) to a0e9887 (merge-base) diff using: array_remove File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagearray_remove — base (merge-base)
array_remove — branch
File an issue against this benchmark runner |
Contributor
Author
|
the benchmark is confusing, the same metric from 2 different runs, reports I'll check again my local benchmarks, if its reported as faster I'll follow local recommendations, however even now the PR brings some purpose in terms of unification |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
array_compacthandle edge case with NULLs #23192 (comment)Rationale for this change
arrow::buffer::OffsetBufferBuilderis a thin wrapper aroundVec<O>plus alast_offset: usizerunning counter; everypush_length(n)does achecked_addonusizeand ausize_as(O)conversion. Forper-row loops with a known upfront row count, a direct
Vec<O>that stores the running offset viaoffsets[row] + O::usize_as(len)can save measurable work in tight per-row loops — provided the offset pushis a meaningful fraction of per-row cost.
I swapped the pattern in all eight
OffsetBufferBuildercall sites in the repo (array_normalize,array_filter,remove,replace,array_add,utils::general_array_zip_with,array_scale,encoding::delegated_decode), benchmarked the three sites that have criterion benches, and found the win is not uniform.What changes are included in this PR?
Replace
OffsetBufferBuilder<O>withVec<O>(preinitialized withO::zero()and finalized withOffsetBuffer::new(v.into())) only indatafusion/functions-nested/src/remove.rs, where benches showclean wins with no regressions.
The other seven sites are left on
OffsetBufferBuilder— benches showed flat-to-regressing results, see below.Are these changes tested?
Existing unit tests, doctests, and sqllogictests (
array_remove*) pass unchanged. No new tests — refactor is functionally equivalent.Are there any user-facing changes?
No.
Benchmark results
The biggest win is
array_removearray_removeint64n_int64all_int64stringsbooleanfixed_size_binaryint64_nestedFor others its more like noise