perf: improve performance of vectorized_equal_to for PrimitiveGroupValueBuilder in multi group by aggregation
#17977
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
rluvaton
commented
Oct 8, 2025
Comment on lines
+112
to
+135
| let iter = izip!( | ||
| lhs_rows.iter(), | ||
| rhs_rows.iter(), | ||
| equal_to_results.iter_mut(), | ||
| ); | ||
|
|
||
| for (&lhs_row, &rhs_row, equal_to_result) in iter { | ||
| // Has found not equal to in previous column, don't need to check | ||
| if !*equal_to_result { | ||
| continue; | ||
| } | ||
|
|
||
| // Perf: skip null check (by short circuit) if input is not nullable | ||
| let exist_null = self.nulls.is_null(lhs_row); | ||
| let input_null = array.is_null(rhs_row); | ||
| if let Some(result) = nulls_equal_to(exist_null, input_null) { | ||
| *equal_to_result = result; | ||
| continue; | ||
| } | ||
|
|
||
| // Otherwise, we need to check their values | ||
| *equal_to_result = self.group_values[lhs_row].is_eq(array.value(rhs_row)); | ||
| } | ||
| } |
Member
Author
There was a problem hiding this comment.
moved the code from vectorized_equal_to and removed the if NULLABLE as we will always get here if nullable
Member
Author
|
@alamb can you please run |
Dandandan
reviewed
Oct 9, 2025
| self.group_values[lhs_row] | ||
| } else { | ||
| // SAFETY: indices are guaranteed to be in bounds | ||
| unsafe { *self.group_values.get_unchecked(lhs_row) } |
Contributor
There was a problem hiding this comment.
As lhs_row is not checked here te be in bounds, this method would need to be marked unsafe as well.
Contributor
|
I could run the benchmarks locally so that we can see the performance gains. Can you provide two git hashes at which to run the criterion benchmark and compare? |
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
53 tasks
Contributor
|
🤖 |
Contributor
|
🤖: Benchmark completed Details
|
Contributor
|
The results look promising and consistent -- thank you @rluvaton -- I plan to review this PR over the next few days |
Contributor
|
🤖 |
alamb
approved these changes
Nov 13, 2025
| unsafe { *array_values.get_unchecked(rhs_row) } | ||
| }; | ||
|
|
||
| // Always evaluate, to allow for auto-vectorization |
Contributor
There was a problem hiding this comment.
this makes sense for primitive values -- namely that the cost of checking if we should compare dominated just always comparing
Contributor
This is a good idea. I filed a ticket to track it |
Contributor
|
🤖: Benchmark completed Details
|
Member
Author
|
As suspected up to 6 times faster:
all below are for The good thing you will notice below is that regardless of the number of true count the results are consistent - meaning it really not using branches and also, the null case which I did not optimize is roughly the same Table
|
Member
Author
|
once we have mutable bit packed buffer, we will also evaluate the nulls separately and do bit mask operation on the compare result to make that case faster as well |
Contributor
|
Thanks again @rluvaton |
logan-keede
pushed a commit
to logan-keede/datafusion
that referenced
this pull request
Nov 23, 2025
…pValueBuilder` in multi group by aggregation (apache#17977) ## Which issue does this PR close? N/A ## Rationale for this change Making multi column aggregation even faster ## What changes are included in this PR? In `PrimitiveGroupValueBuilder.vectorized_equal_to` always evaluate and use unchecked as both of these changes are what making the code compile to SIMD. ## Are these changes tested? Existing tests ## Are there any user-facing changes? Nope ----- I tried a LOT of variations [GodBolt](https://godbolt.org/z/Kc8ze6E9n) from splitting to fixed size chunks and trying to get auto-vectorization to use gather and creating bitmask to even testing portable SIMD (just to see what it will generate). this version only optimize the non null path for the moment as it is the easiest. once and if we change from `&mut [bool]` to mutable packed bits we could: 1. evaluate in chunks of `64` items (I tried different variations to see what is the best - you can tweak in the godbolt above with different type and size to check for yourself), 64 is not necessarily the best but it will be the fastest I think for doing AND with the `equal_to_results` boolean buffer 2. add optimization for nullable as well by just doing bitwise operation at 64 items at a time and avoid the cost of getting each bit manually 3. skip 64 items right away if the the `equal_to_results` equal to `0x00` (i.e. all false) --------- Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>
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?
N/A
Rationale for this change
Making multi column aggregation even faster
What changes are included in this PR?
In
PrimitiveGroupValueBuilder.vectorized_equal_toalways evaluate and use unchecked as both of these changes are what making the code compile to SIMD.Are these changes tested?
Existing tests
Are there any user-facing changes?
Nope
I tried a LOT of variations GodBolt
from splitting to fixed size chunks and trying to get auto-vectorization to use gather and creating bitmask to even testing portable SIMD (just to see what it will generate).
this version only optimize the non null path for the moment as it is the easiest.
once and if we change from
&mut [bool]to mutable packed bits we could:64items (I tried different variations to see what is the best - you can tweak in the godbolt above with different type and size to check for yourself), 64 is not necessarily the best but it will be the fastest I think for doing AND with theequal_to_resultsboolean bufferequal_to_resultsequal to0x00(i.e. all false)