Replace ArrayData with direct Array construction in arrow-row - #10229
Merged
Conversation
Contributor
Author
|
run benchmark row_format |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing row-refactor-out-arraydata (033eb20) to da07bce (merge-base) diff File an issue against this benchmark runner |
Jefffrey
commented
Jun 28, 2026
| /// and all other values as a null | ||
| /// | ||
| /// Returns the null count and null buffer | ||
| pub fn decode_nulls(rows: &[&[u8]]) -> (usize, Buffer) { |
Contributor
Author
There was a problem hiding this comment.
Instead of returning a separate null count & null buffer (as Buffer), just return Option<NullBuffer> where None is if the resulting null buffer has 0 nulls
Contributor
There was a problem hiding this comment.
I think NullBuffers also contain a null count, however, they compute the null count with quite optimized code, so I think recomputing it in many cases is fine
Contributor
Author
There was a problem hiding this comment.
i assumed the way nullbuffer calculates its count might be as fast (or faster) than the manual count in the loop
Contributor
There was a problem hiding this comment.
Yes -- when we last checked LLVM was using the native hardware instruction to count set bits which was crazy fast
As I recall there were a few times it was faster to avoid a second pass through the data, but only when the bits were already being processed a word (u64) at a time, not individually
| /// # Safety | ||
| /// | ||
| /// `data_type` must be appropriate native type for `T` | ||
| unsafe fn decode_fixed<T: FixedLengthEncoding + ArrowNativeType>( |
Contributor
Author
There was a problem hiding this comment.
This has been inlined into decode_primitive
| } | ||
| } | ||
|
|
||
| pub(crate) fn decode_nulls_sentinel(rows: &[&[u8]], options: SortOptions) -> Option<NullBuffer> { |
Contributor
Author
There was a problem hiding this comment.
Similar to the one in fixed.rs but compares using null_sentinel()
Contributor
There was a problem hiding this comment.
perhaps some small comments explaining what it does would help future readers
Jefffrey
marked this pull request as ready for review
June 28, 2026 14:31
Contributor
Author
|
fyi @liamzwbao since you worked on similar PRs before |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
note to self: only convert_rows benchmarks should be affected, rest would be just noise |
Contributor
Author
|
run benchmark row_format |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing row-refactor-out-arraydata (033eb20) to da07bce (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
alamb
approved these changes
Jun 29, 2026
| .null_count(null_count) | ||
| .add_buffer(values.into()) | ||
| .null_bit_buffer(Some(nulls.into())); | ||
| let nulls = NullBuffer::new(BooleanBuffer::new(nulls.into(), 0, len)); |
Contributor
There was a problem hiding this comment.
I think new recomputes the null cound
We could use new_unchecked here to keep the exact same behavior
https://docs.rs/arrow/latest/arrow/buffer/struct.NullBuffer.html#method.new_unchecked
However it seems from the bencmarks it doesn't really matter (this formulation is as fast or faster as main)
ppend_rows 4096 bool(0, 0.5) 1.05 5.1±0.02µs ? ?/sec 1.00 4.9±0.00µs ? ?/sec
append_rows 4096 bool(0.3, 0.5) 1.00 5.8±0.00µs ? ?/sec 1.00 5.8±0.00µs ? ?/se
Contributor
Author
There was a problem hiding this comment.
yeah i wasnt sure if keeping the existing manual count was faster than however nullbuffer calculates its count; decided to keep it simpler with less unsafe blocks (and follow other changes i made in this PR to remove the manual count)
Contributor
There was a problem hiding this comment.
I agree -- sounds like a good call and it is reflected in the benchmarks as well
| /// and all other values as a null | ||
| /// | ||
| /// Returns the null count and null buffer | ||
| pub fn decode_nulls(rows: &[&[u8]]) -> (usize, Buffer) { |
Contributor
There was a problem hiding this comment.
I think NullBuffers also contain a null count, however, they compute the null count with quite optimized code, so I think recomputing it in many cases is fine
|
|
||
| let len = rows.len(); | ||
|
|
||
| let mut values = BufferBuilder::<T::Native>::new(len); |
Contributor
There was a problem hiding this comment.
We could potentially use Vec<T::Native> here which may be faster
Contributor
Author
There was a problem hiding this comment.
refactored to use vec; i wonder if we should also look for usages of [offset]bufferbuilder and replace them, like we're doing for arraydata 🤔
Contributor
Author
There was a problem hiding this comment.
locally i seem to get some nice speedups from switching to vec:
convert_rows 4096 u64(0)
time: [13.356 µs 13.413 µs 13.463 µs]
change: [−12.772% −12.336% −11.901%] (p = 0.00 < 0.05)
Performance has improved.
convert_rows 4096 u64(0.3)
time: [13.653 µs 13.692 µs 13.730 µs]
change: [−11.773% −11.227% −10.677%] (p = 0.00 < 0.05)
Performance has improved.
Found 5 outliers among 100 measurements (5.00%)
1 (1.00%) low mild
4 (4.00%) high mild
convert_rows 4096 i64(0)
time: [13.174 µs 13.209 µs 13.241 µs]
change: [−11.255% −10.789% −10.340%] (p = 0.00 < 0.05)
Performance has improved.
convert_rows 4096 i64(0.3)
time: [13.996 µs 14.038 µs 14.079 µs]
change: [−8.9987% −8.5290% −8.0439%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
2 (2.00%) low mild
2 (2.00%) high mild
Contributor
There was a problem hiding this comment.
locally i seem to get some nice speedups from switching to vec:
Yeah, the Rust team has optimized Vec a lot
i wonder if we should also look for usages of [offset]bufferbuilder and replace them, like we're doing for arraydata 🤔
Yes, I think that would be a nice idea. I filed a ticket
| } | ||
| Codec::Struct(converter, _) => { | ||
| let (null_count, nulls) = fixed::decode_nulls(rows); | ||
| let nulls = fixed::decode_nulls(rows); |
|
|
||
| Arc::new(StructArray::from(unsafe { builder.build_unchecked() })) | ||
|
|
||
| Arc::new(unsafe { |
Contributor
There was a problem hiding this comment.
I think it would be nice to add a small safety justification here, though I see the previous code didn't have one either
Contributor
Author
There was a problem hiding this comment.
i guess something like
/// SAFETY: valid by construction
which does feel a little weak 😅
i was looking at the invariants and i find it difficult to justify all the invariants were upheld (from looking at this code alone) 🤔
| } | ||
| } | ||
|
|
||
| pub(crate) fn decode_nulls_sentinel(rows: &[&[u8]], options: SortOptions) -> Option<NullBuffer> { |
Contributor
There was a problem hiding this comment.
perhaps some small comments explaining what it does would help future readers
| } | ||
|
|
||
| pub(crate) fn decode_nulls_sentinel(rows: &[&[u8]], options: SortOptions) -> Option<NullBuffer> { | ||
| let nulls = BooleanBuffer::collect_bool(rows.len(), |x| rows[x][0] != null_sentinel(options)); |
Contributor
There was a problem hiding this comment.
Given there are only two null values for null sentinel, I wonder if we could make two separate loops here for the two values and generate even more efficient code
As a follow PR
Contributor
Author
There was a problem hiding this comment.
could be a wider refactor too, since null_sentinel() is used in other loops too. for now ive hoisted the function outside the loop (though maybe LLVM already would have optimized it like this)
Contributor
There was a problem hiding this comment.
Another thought is we could potentially make a null sentinel type and then template the relevant functions (to have rust do the code copy for us)
Contributor
Author
There was a problem hiding this comment.
Contributor
Author
|
run benchmark row_format |
This comment was marked as outdated.
This comment was marked as outdated.
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark row_format |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing row-refactor-out-arraydata (d5d0b99) to 033eb20 diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
Author
|
run benchmark row_format |
|
🤖 Arrow criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing row-refactor-out-arraydata (d5d0b99) to da07bce (merge-base) diff File an issue against this benchmark runner |
|
🤖 Arrow criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
Contributor
|
Thank you @Jefffrey |
This was referenced Jun 30, 2026
alamb
pushed a commit
that referenced
this pull request
Jul 2, 2026
…irect construction (#10261) # Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. --> - Part of #9298 # Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> Missed by #10229 as it used the `into_data()` API (I was grepping just for `ArrayData`). Instead of roundtripping through arraydata to convert from a binary to string, just directly use `into_parts()` and `new_unchecked()` for optimization. # What changes are included in this PR? <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> Remove arraydata roundtrip in favour of deconstructing with `into_parts()` and constructing with `new_unchecked()` # Are these changes tested? <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? If this PR claims a performance improvement, please include evidence such as benchmark results. --> Existing tests # Are there any user-facing changes? <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. If there are any breaking changes to public APIs, please call them out. --> No
Member
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?
ArrayDatawith direct Array construction, when possible #9298Rationale for this change
Avoid indirection via
ArrayDataconstruction for potentially some small performance benefits.What changes are included in this PR?
Switch all usages of
ArrayDatato directArrayconstruction inarrow-row.Are these changes tested?
Existing tests
Are there any user-facing changes?
No