Add special implementation for zip for Utf8View/BinaryView scalars #8963
+341
−4
Conversation
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
mkleen
changed the title
mkleen
force-pushed
the
zip-string-view-improv
branch
from
70809ca to
d64eaf5
Compare
mkleen
changed the title
mkleen
force-pushed
the
zip-string-view-improv
branch
from
d64eaf5 to
1e05651
Compare
mkleen
changed the title
mkleen
changed the title
mkleen
changed the title
mkleen
force-pushed
the
zip-string-view-improv
branch
2 times, most recently
from
2006eff to
724157f
Compare
mkleen
changed the title
mkleen
changed the title
mkleen
changed the title
mkleen
commented
Dec 8, 2025
Author
|
I have some decent speed-ups, see benchmarks. It would be great if somone could provide an initial review of this. |
mkleen
force-pushed
the
zip-string-view-improv
branch
from
724157f to
ba3c71a
Compare
mkleen
changed the title
Author
|
@rluvaton Maybe you could have a look since you were the original author of this optimization? Thank you! |
Member
|
Great job! Will try to review it today |
mkleen
force-pushed
the
zip-string-view-improv
branch
from
94fd9a6 to
310a81a
Compare
mkleen
force-pushed
the
zip-string-view-improv
branch
from
310a81a to
918e2d0
Compare
Jefffrey
reviewed
Dec 25, 2025
Comment on lines
+669
to
+670
| truthy: Option<GenericByteViewArray<T>>, | ||
| falsy: Option<GenericByteViewArray<T>>, |
Contributor
There was a problem hiding this comment.
I question this choice of holding a GenericByteViewArray itself here 🤔
Can we decompose it to just the buffers and view u128 perhaps?
Comment on lines
+683
to
+689
| fn get_value_from_scalar(scalar: &dyn Array) -> Option<GenericByteViewArray<T>> { | ||
| if scalar.is_null(0) { | ||
| None | ||
| } else { | ||
| Some(scalar.as_byte_view().clone()) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Personally I would inline this; there's a lot of functions here so we would benefit from removing simple ones like this
|
|
||
| let bytes = Buffer::from(mutable); | ||
|
|
||
| (bytes.into(), buffers, Some(NullBuffer::new_valid(length))) |
Contributor
There was a problem hiding this comment.
Suggested change
| (bytes.into(), buffers, Some(NullBuffer::new_valid(length))) | |
| (vec![views[0]; length].into(), buffers, None) |
No need for null buffer if all are valid
Also can simplify buffer creation
| // otherwise, we simply use the view. | ||
| let view_falsy = if falsy.total_buffer_bytes_used() > 0 { | ||
| let byte_view_falsy = ByteView::from(falsy.views()[0]); | ||
| let new_index_falsy_buffers = buffers.len() as u32; |
Contributor
There was a problem hiding this comment.
Suggested change
| let new_index_falsy_buffers = buffers.len() as u32; | |
| let new_index_falsy_buffers = buffers.len() as u32 + byte_view_falsy.buffer_index; |
We can't assume falsy only has 1 buffer
| // All values are null | ||
| return Self::get_scalar_buffers_and_nulls_for_all_values_null(number_of_values); | ||
| } | ||
| let view = value.views()[0].to_byte_slice(); |
Contributor
There was a problem hiding this comment.
Does this view have the right buffer offset?
Comment on lines
+746
to
+747
| let total_number_of_bytes = true_count * view_truthy.len() | ||
| + (predicate.len() - true_count) * view_falsy.to_byte_slice().len(); |
Contributor
There was a problem hiding this comment.
view_truthy and view_falsy are u128s which we can know the constant size for; no need to call len() on their byte slices
| ( | ||
| bytes.into(), | ||
| buffers, | ||
| Some(NullBuffer::new_valid(predicate.len())), |
Contributor
There was a problem hiding this comment.
Suggested change
| Some(NullBuffer::new_valid(predicate.len())), | |
| None, |
All values are non-null so we don't need a null buffer
Comment on lines
+694
to
+697
| let mut mutable = MutableBuffer::with_capacity(0); | ||
| mutable.repeat_slice_n_times((0u128).to_byte_slice(), len); | ||
|
|
||
| (mutable.into(), vec![], Some(NullBuffer::new_null(len))) |
Contributor
There was a problem hiding this comment.
Suggested change
| let mut mutable = MutableBuffer::with_capacity(0); | |
| mutable.repeat_slice_n_times((0u128).to_byte_slice(), len); | |
| (mutable.into(), vec![], Some(NullBuffer::new_null(len))) | |
| (vec![0; len].into(), vec![], Some(NullBuffer::new_null(len))) |
At this point it would be better to inline this instead of having this thin wrapper function
| (mutable.into(), vec![], Some(NullBuffer::new_null(len))) | ||
| } | ||
|
|
||
| fn get_scalar_buffers_and_nulls_for_single_non_nullable( |
Contributor
There was a problem hiding this comment.
Suggested change
| fn get_scalar_buffers_and_nulls_for_single_non_nullable( | |
| fn get_view_parts_single_value( |
These function names can do with improving, they aren't very readable
Comment on lines
+713
to
+716
| let mut bytes = MutableBuffer::with_capacity(0); | ||
| bytes.repeat_slice_n_times(view, number_of_values); | ||
|
|
||
| let bytes = Buffer::from(bytes); |
Contributor
There was a problem hiding this comment.
Suggested change
| let mut bytes = MutableBuffer::with_capacity(0); | |
| bytes.repeat_slice_n_times(view, number_of_values); | |
| let bytes = Buffer::from(bytes); | |
| let bytes = vec![value.views()[0]; number_of_values]; |
No need to use MutableBuffer since our values (views) are simple u128s
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?
zipwith scalar for Utf8View #8724Rationale for this change
It's explained in the issue.
What changes are included in this PR?
This adds a special implementation for Utf8View/BinaryView scalars for zip based on the design from #8653. It also includes tests. Benchmarks are available here:
Are these changes tested?
Yes.
Are there any user-facing changes?
There is a new struct
ByteViewScalarImpl.Benchmarks
System: Apple M1 Max with 10 cores on macOS 26.1