Optimize Nullstate / accumulators
#19625
Open
+186
−69
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
Contributor
Author
|
run benchmark tpch |
Contributor
Author
|
run benchmarks |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
Looks like it is a nice win. |
Contributor
Author
|
run benchmarks |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
run benchmarks |
|
🤖 |
|
Benchmark script failed with exit code 101. Last 10 lines of output: Click to expand |
Contributor
Author
|
run benchmarks |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
run benchmarks |
|
🤖 |
Dandandan
force-pushed
the
speedup_accumulate2
branch
from
fb8f6ac to
05414e8
Compare
alamb
reviewed
Jan 5, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
this is looking pretty cool
| /// If `seen_values[i]` is false, have not seen any values that | ||
| /// pass the filter yet for group `i` | ||
| seen_values: BooleanBufferBuilder, | ||
| /// If true, all groups seen so far have seen at least one non-null value |
Contributor
There was a problem hiding this comment.
Maybe we could encode this as an state enum so it is clearer how things are related to BooleamBufferBuilder
Something like
enum SeenValues {
/// All groups seen so far have seen at least one non-null value
All {
num_values: usize,
}
// some groups have not yet seen a non-null value
Some {
values: BooleanBufferBuilder,
}
}
Contributor
Author
|
run benchmarks |
|
🤖 |
|
🤖: Benchmark completed Details
|
Contributor
Author
|
run benchmark aggregate |
|
🤖 Hi @Dandandan, thanks for the request (#19625 (comment)).
Please choose one or more of these with |
Contributor
Author
|
run benchmark aggregate_query_sql aggregate_vectorized |
|
🤖 |
|
🤖: Benchmark completed Details
|
|
🤖 |
|
Benchmark script failed with exit code 101. Last 10 lines of output: Click to expand |
Contributor
Author
|
run benchmark tpch_mem |
|
🤖 |
|
🤖: Benchmark completed Details
|
alamb
approved these changes
Jan 8, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @Dandandan -- this looks good to me. I have some comment / structure comments but I don't think anything is needed prior to merge (we could do it as follow on PRs or never)
| // "not seen" valid) | ||
| let seen_values = | ||
| initialize_builder(&mut self.seen_values, total_num_groups, false); | ||
| if let SeenValues::All { num_values } = &mut self.seen_values |
Contributor
There was a problem hiding this comment.
One possibility is to make this another another function to reduce some duplication and have a place it could be explained
Maybe something like
if let Some(num_values) = self.all_values_mut() && opt_filter.is_none && values.null_count == 0 {
...
}?
Though maybe an extra level of indirection would make it harder to follow.
| // "not seen" valid) | ||
| let seen_values = | ||
| initialize_builder(&mut self.seen_values, total_num_groups, false); | ||
| if let SeenValues::All { num_values } = &mut self.seen_values |
Contributor
There was a problem hiding this comment.
Suggested change
| if let SeenValues::All { num_values } = &mut self.seen_values | |
| // skip null handling if no nulls in input or accumulator | |
| if let SeenValues::All { num_values } = &mut self.seen_values |
| // "not seen" valid) | ||
| let seen_values = | ||
| initialize_builder(&mut self.seen_values, total_num_groups, false); | ||
| if let SeenValues::All { num_values } = &mut self.seen_values |
Contributor
There was a problem hiding this comment.
Suggested change
| if let SeenValues::All { num_values } = &mut self.seen_values | |
| // skip null handling if no nulls in input or accumulator | |
| if let SeenValues::All { num_values } = &mut self.seen_values |
| SeenValues::Some { .. } => { | ||
| let mut old_values = match std::mem::replace( | ||
| &mut self.seen_values, | ||
| SeenValues::All { num_values: 0 }, |
Contributor
There was a problem hiding this comment.
It might be nicer if you implemented SeenValues::new() or maybe even SeenValues::default
SO this couls look like
let mut old_values = match std::mem::take(&mut self.seen_values);| total_num_groups, | ||
| |group_index, new_value| { | ||
| let value = &mut self.values[group_index]; | ||
| // SAFETY: group_index is guaranteed to be in bounds |
Contributor
There was a problem hiding this comment.
I recommend adding safety notes to the docs of GroupsAccumulator in https://github.com/apache/datafusion/blob/36ec9f1de0aeabca60b8f7ebe07d650b8ef03506/datafusion/expr-common/src/groups_accumulator.rs#L114-L113
That explains that all group indexes are guaranteed to be <= total_num_groups and that can be relied on for safety
Contributor
|
run benchmark tpch_mem |
|
🤖 |
Contributor
|
(will see if we can see a reproducable speedup) |
|
🤖: Benchmark completed Details
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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?
Rationale for this change
Speedup accumulator code (sum, avg, count) by specializing on non-null cases.
What changes are included in this PR?
Nullstateto non-null values.Are these changes tested?
Are there any user-facing changes?