perf: Optimize array_has_any() with scalar arg#20385
Merged
alamb merged 23 commits intoapache:mainfrom Feb 24, 2026
Merged
Conversation
github-actions
Bot
added
the
documentation
Contributor
Author
|
Benchmark results: |
neilconway
force-pushed
the
neilc/optimize-array-has-any-scalar
branch
from
cc2d735 to
ef696bd
Compare
The previous implementation tested the cost of building an array_has() `Expr` (!), not actually evaluating the array_has() operation itself. Refactor things along the way.
neilconway
force-pushed
the
neilc/optimize-array-has-any-scalar
branch
from
83484af to
b3bbf3a
Compare
Contributor
Author
|
Note that the commit fixing up the benchmarks is shared with #20374 -- I can also pull that out into a separate PR, because it's a prerequisite for any performance work on these functions |
martin-g
reviewed
Feb 19, 2026
Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com>
Jefffrey
reviewed
Feb 20, 2026
| use crate::utils::make_scalar_function; | ||
|
|
||
| use std::any::Any; | ||
| use std::collections::HashSet; |
Contributor
There was a problem hiding this comment.
Do we get performance gains if we use hashbrown instead?
- Related issue: Consider disallowing std hashmap/hashset via clippy #19869
Contributor
Author
There was a problem hiding this comment.
I'm happy to benchmark, although std HashSet uses hashbrown internally these days; have we found that using hashbrown directly leads to be better performance in other circumstances?
Contributor
There was a problem hiding this comment.
Main point of reference is this reply:
Though its a few years old at this point so don't know if things have changed since
|
|
||
| let col_list: ArrayWrapper = col_arr.as_ref().try_into()?; | ||
| let all_col_strings = string_array_to_vec(col_list.values().as_ref()); | ||
| let col_offsets: Vec<usize> = col_list.offsets().collect(); |
Contributor
There was a problem hiding this comment.
We could probably avoid this collect of offsets if we take advantage of peeking the iter
Contributor
Author
There was a problem hiding this comment.
Interestingly, this turned out to be much slower:
array_has_any_scalar/string_no_match/1
time: [97.474 µs 98.334 µs 99.302 µs]
change: [−15.448% −14.766% −14.081%] (p = 0.00 < 0.05)
Performance has improved.
array_has_any_scalar/string_no_match/10
time: [298.73 µs 317.71 µs 343.54 µs]
change: [+32.141% +60.866% +85.741%] (p = 0.00 < 0.05)
Performance has regressed.
Found 20 outliers among 100 measurements (20.00%)
3 (3.00%) low mild
17 (17.00%) high severe
array_has_any_scalar/string_no_match/100
time: [437.14 µs 455.34 µs 480.03 µs]
change: [+22.766% +39.379% +59.120%] (p = 0.00 < 0.05)
Performance has regressed.
Found 18 outliers among 100 measurements (18.00%)
1 (1.00%) low mild
17 (17.00%) high severe
array_has_any_scalar/string_no_match/1000
time: [332.13 µs 351.25 µs 376.77 µs]
change: [+28.480% +54.273% +78.992%] (p = 0.00 < 0.05)
Performance has regressed.
I didn't dig into why; maybe dynamic dispatch because of the iterator adds a bunch of overhead? I'll leave this as-is for now.
| columnar_arg: &ColumnarValue, | ||
| scalar_values: &ArrayRef, | ||
| ) -> Result<ColumnarValue> { | ||
| let scalar_strings = string_array_to_vec(scalar_values.as_ref()); |
Contributor
There was a problem hiding this comment.
I wonder if we should defer creating this vec, since if we take the hashing path we effectively allocate a vec to allocate a hashset, which seems redundant
Contributor
Author
There was a problem hiding this comment.
I tried this but it didn't seem to help the benchmarks, so I'll keep things as they were for now.
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
Contributor
Author
|
@Jefffrey Thank you for the detailed code review! 🙏 I addressed all of your comments; please let me know if you have more feedback. |
Jefffrey
approved these changes
Feb 23, 2026
Contributor
Jefffrey
left a comment
Jefffrey
left a comment
There was a problem hiding this comment.
How do the benchmarks look now?
| use crate::utils::make_scalar_function; | ||
|
|
||
| use std::any::Any; | ||
| use std::collections::HashSet; |
Contributor
There was a problem hiding this comment.
Main point of reference is this reply:
Though its a few years old at this point so don't know if things have changed since
Contributor
Author
|
Benchmarks comparing the latest version (with hashbrown) versus main: |
Contributor
Author
|
@Jefffrey Got it; the default |
Contributor
|
Thanks @Jefffrey @martin-g and @neilconway |
de-bgunter
pushed a commit
to de-bgunter/datafusion
that referenced
this pull request
Mar 24, 2026
## Which issue does this PR close? - Closes apache#20384. - See apache#18181 for related context. ## Rationale for this change When `array_has_any` is passed a scalar for either of its arguments, we can use a much faster algorithm: rather than doing O(N*M) comparisons for each row of the columnar arg, we can build a hash table on the scalar argument and probe it instead. ## What changes are included in this PR? * Add benchmark to cover the one-scalar-arg case * Implement optimization as described above Note that we fallback to a linear scan when the scalar arg is smaller than a threshold (<= 8 elements), because benchmarks suggested probing a HashSet is not profitable for very small arrays. ## Are these changes tested? Yes. Tests pass and benchmarked. ## Are there any user-facing changes? No. --------- Co-authored-by: Martin Grigorov <martin-g@users.noreply.github.com> Co-authored-by: Jeffrey Vo <jeffrey.vo.australia@gmail.com>
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?
array_has_any()for scalar arg #20384.array_has#18181 for related context.Rationale for this change
When
array_has_anyis passed a scalar for either of its arguments, we can use a much faster algorithm: rather than doing O(N*M) comparisons for each row of the columnar arg, we can build a hash table on the scalar argument and probe it instead.What changes are included in this PR?
Note that we fallback to a linear scan when the scalar arg is smaller than a threshold (<= 8 elements), because benchmarks suggested probing a HashSet is not profitable for very small arrays.
Are these changes tested?
Yes. Tests pass and benchmarked.
Are there any user-facing changes?
No.