Replace OffsetBufferBuilder with Vec in byte-array zip - #10913
Open
yashkuceriya wants to merge 1 commit into
Open
Replace OffsetBufferBuilder with Vec in byte-array zip#10913yashkuceriya wants to merge 1 commit into
yashkuceriya wants to merge 1 commit into
Conversation
Contributor
|
run benchmark zip_kernels |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing optimize-zip-offsets (4829c8f) to cbbb56b (merge-base) diff Run configurationrun benchmark zip_kernelsBENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental,object_store --bench zip_kernels File an issue against this benchmark runner |
Rich-T-kid
suggested changes
Aug 29, 2026
Rich-T-kid
left a comment
Rich-T-kid
left a comment
Contributor
There was a problem hiding this comment.
This PR looks good.
we should return errors instead of panicking on overflows. I outlined one place but the same idea applies to all call sites in the PR
| offset_buffer_builder.push_length(falsy_len) | ||
| } | ||
| let start_offset = current_offset; | ||
| let added = falsy_len.checked_mul(false_repeat_count).expect("overflow"); |
Contributor
There was a problem hiding this comment.
we should avoid panicking here. return an error
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
|
zip generally takes too long, you'll need to run with specific bench filters @Rich-T-kid |
Contributor
|
run benchmark zip_kernels env: |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing optimize-zip-offsets (4829c8f) to cbbb56b (merge-base) diff Run configurationrun benchmark zip_kernels
env:
BENCH_FILTER: "non_nulls_scalars"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
run benchmark zip_kernels |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: Comparing optimize-zip-offsets (4829c8f) to cbbb56b (merge-base) diff Run configurationrun benchmark zip_kernels
env:
BENCH_FILTER: "zip_8192_from_(long|short).+non_nulls_scalars"CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
@Jefffrey I wouldn't expect there to be a 4x speed up from this PR. are the zip benchmarks usually this noisy? |
Contributor
|
im able to reproduce the results locally. maybe because its a scalar path the optimization has a larger effect |
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
4 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?
OffsetBufferBuilder/BufferBuilderusage withVec, when possible #10245.Rationale for this change
The byte-array scalar zip path is one of the remaining
OffsetBufferBuildercallsites listed in #10245. Constructing the offsets directly in aVecalso allows each truthy or falsy run to validate its total length once before extending the cumulative offsets.A temporary release-mode microbenchmark exercised a mixed 4,096-row mask with two UTF-8 scalars for 10,000 iterations. In three alternating runs, the median was 3.18 ns/row for this change and 3.38 ns/row for current
main, an improvement of about 5.9%. The temporary benchmark was not committed.What changes are included in this PR?
BytesScalarImpl::create_output_on_non_nullsnow builds offsets withVec<T::Offset>. It preserves the existing capacity, cumulative-offset, overflow, and monotonicity behavior.Are these changes tested?
Yes:
cargo +stable fmt --all -- --checkcargo test -p arrow-select- 417 unit tests and 17 documentation tests passedcargo clippy -p arrow-select --all-targets --all-features -- -D warningsExisting zip tests cover regular and large byte arrays, mixed masks, fragmented masks, nulls, and one-sided scalar paths.
Are there any user-facing changes?
No.
Automated assistance
Automated assistance was used to identify the requested callsite and draft the initial refactor. I reviewed the implementation, fixed compiler findings, rejected a slower first version after benchmarking, verified the final offset and overflow behavior against
OffsetBufferBuilder, and ran all checks listed above.