feat: Reduce allocations for aggregating Statistics#20768
Open
jonathanc-n wants to merge 12 commits intoapache:mainfrom
Open
feat: Reduce allocations for aggregating Statistics#20768jonathanc-n wants to merge 12 commits intoapache:mainfrom
Statistics#20768jonathanc-n wants to merge 12 commits intoapache:mainfrom
Conversation
jonathanc-n
commented
Mar 7, 2026
Contributor
Author
|
I verified this has a 5x speed up for numeric primitive values using small benchmark. felt unnecssary to add the benchmark since it is jsut a regular vectorization optimization |
Dandandan
reviewed
Mar 7, 2026
jonathanc-n
changed the title
StatisticsStatistics
jonathanc-n
force-pushed
the
speed-up-stats-aggregation
branch
from
06cd380 to
e51023e
Compare
…hanc-n/datafusion into speed-up-stats-aggregation
…hanc-n/datafusion into speed-up-stats-aggregation
Contributor
Author
|
@Dandandan cleaned up the implementation. The performance hit came from using |
Dandandan
approved these changes
Mar 8, 2026
Contributor
Dandandan
left a comment
Dandandan
left a comment
There was a problem hiding this comment.
I think it looks good, it could benefit though from a benchmark somewhere.
Contributor
Author
Main benchmarkSpeed up benchmarkadded bench. Looking at around a 10-12x improvement |
asolimando
reviewed
Mar 9, 2026
Member
asolimando
left a comment
asolimando
left a comment
There was a problem hiding this comment.
LGTM, left a couple of minor comments and questions
| ($lhs:expr, $rhs:expr, $VARIANT:ident) => { | ||
| match ($lhs, $rhs) { | ||
| (ScalarValue::$VARIANT(Some(a)), ScalarValue::$VARIANT(Some(b))) => { | ||
| Ok(ScalarValue::$VARIANT(Some(a.wrapping_add(*b)))) |
Member
There was a problem hiding this comment.
Correct me if I am wrong, but it seems that wrapping_add would wrap the sum upon overflow, producing a negative number, I am not sure this is what we want for statistics, I propose we either mark it Absent or at the latest mark it as Inexact (which doesn't seem to be happening) as done in Precision::add.
Not in scope for this PR, but on the subject I also think pub sum_value: Precision<ScalarValue>, unlike min and max should not have matched the data type of the column, but a wide type like Int64 to minimize the chances of overflow.
Contributor
Author
There was a problem hiding this comment.
Not in scope for this PR, but on the subject I also think
pub sum_value: Precision<ScalarValue>, unlikeminandmaxshould not have matched the data type of the column, but a wide type likeInt64to minimize the chances of overflow.
Good point, filed in #20826
Contributor
Author
There was a problem hiding this comment.
@asolimando Added the fix! 256 needs to be dealt with differently, so set up a small function for that
Member
There was a problem hiding this comment.
Checked the new commit, thanks for addressing the comment and filing the issue, marking as resolved!
EDIT: it looks I lack privileges, I will let you do that if you want
asolimando
approved these changes
Mar 9, 2026
Member
asolimando
left a comment
asolimando
left a comment
There was a problem hiding this comment.
LGTM (modulo fixing the cargo doc failure)
xudong963
reviewed
Mar 11, 2026
| ScalarValue::Float64(_) => add_float!(lhs, rhs, Float64), | ||
| ScalarValue::Decimal32(_, _, _) => add_decimal!(lhs, rhs, Decimal32), | ||
| ScalarValue::Decimal64(_, _, _) => add_decimal!(lhs, rhs, Decimal64), | ||
| ScalarValue::Decimal128(_, _, _) => add_decimal!(lhs, rhs, Decimal128), |
Member
There was a problem hiding this comment.
For Decimal128(Some(a), p, s), saturating to i128::MAX doesn't respect the precision constraint. A Decimal128 with precision 5 can't hold i128::MAX.
For example:
let a = Decimal128(Some(99999), 5, 2); // 999.99
let b = Decimal128(Some(99999), 5, 2); // 999.99
| @@ -0,0 +1,149 @@ | |||
| // Licensed to the Apache Software Foundation (ASF) under one | |||
Member
There was a problem hiding this comment.
Not sure if we can put it into a higher level, is it possible that other nodes may use this code in the future, not only agg.
Contributor
|
run benchmark sql_planner |
alamb
reviewed
Mar 12, 2026
| /// converts the result back — 3 heap allocations per call. | ||
| /// | ||
| /// For non-primitive types, falls back to `ScalarValue::add`. | ||
| pub(crate) fn scalar_add(lhs: &ScalarValue, rhs: &ScalarValue) -> Result<ScalarValue> { |
Contributor
There was a problem hiding this comment.
I bet you can make this even faster by making it mutate lhs rather than make a new one
pub(crate) fn scalar_add(lhs: &mut ScalarValue, rhs: &ScalarValue) -> Result<()> {| pub(crate) fn precision_add( | ||
| lhs: &Precision<ScalarValue>, | ||
| rhs: &Precision<ScalarValue>, | ||
| ) -> Precision<ScalarValue> { |
Contributor
There was a problem hiding this comment.
Ditto here -- reusing lhs is probably even faster
alamb
reviewed
Mar 12, 2026
| //! | ||
| //! Provides a cheap pairwise [`ScalarValue`] addition that directly | ||
| //! extracts inner primitive values, avoiding the expensive | ||
| //! `ScalarValue::add` path (which round-trips through Arrow arrays). |
Contributor
There was a problem hiding this comment.
Why not just add this special case to ScalarValue::add ?
|
🤖 |
|
🤖: Benchmark completed Details
|
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
6 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
What changes are included in this PR?
Vectorize aggregations for combining statistics by gathering all values then calling kernels once
Are these changes tested?
Unit tests + existing tests
Are there any user-facing changes?
Removed
merge_iter