Rewrite AVG(expr) --> SUM(expr) / COUNT(expr) when components can be shared - #25536
wudidapaopao wants to merge 3 commits into
Conversation
add0efd to
9acbb37
Compare
|
run benchmark tpch |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing avg-simplify-sum-count (9acbb37) to c149764 (merge-base) diff Run configurationrun benchmark tpchResults will be posted here when complete File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: Comparing avg-simplify-sum-count (9acbb37) to c149764 (merge-base) diff Run configurationrun benchmark tpchCPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
# Conflicts: # datafusion/sqllogictest/test_files/aggregates_simplify.slt
# Conflicts: # datafusion/expr/src/udaf.rs
|
Thanks for running the benchmark. The result shows no improvement because the standard TPC-H schema uses Decimal columns, while this PR only decomposes Float64 AVG, so the new optimization is not triggered. In a local release benchmark, I materialized the four Q1 Decimal input columns as Float64 Parquet. Q1 improved from a 29.137 ms median to 25.978 ms, or |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #25536 +/- ##
========================================
Coverage 82.39% 82.40%
========================================
Files 1138 1139 +1
Lines 434623 434914 +291
Branches 434623 434914 +291
========================================
+ Hits 358100 358371 +271
- Misses 54856 54862 +6
- Partials 21667 21681 +14 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AVG(expr) --> SUM(expr) / COUNT(expr) when components can be shared
Which issue does this PR close?
Rationale for this change
AVGmaintains both sum and count state. When the same aggregate node already computes a matchingSUMorCOUNT, those states are redundant.What changes are included in this PR?
AVGexpressions into sharedSUM/COUNTcomponents.AVGaccumulator when no component can be shared.NameTrackerfor internal aggregate name conflicts.For example, given a Float64 column:
the optimized plan is equivalent to:
The existing
SUM(x)is computed once and reused byAVG(x). In contrast,SELECT AVG(x)alone keeps the original combined AVG accumulator because decomposition would add an aggregate.Decimal AVG is not included because its accumulation and result types differ from regular Decimal SUM. For example, for
Decimal128(15, 2), regular SUM returnsDecimal128(25, 2), while AVG uses aDecimal128(38, 2)internal sum and returnsDecimal128(19, 6). Reusing the regular SUM directly could change scale and overflow behavior. There is not yet a satisfactory way to share these states while preserving those semantics, so this PR does not optimize Decimal AVG. As a result, the standardTPC-H Q1, which uses Decimal columns, does not benefit from this PR as initially expected.What is the testing strategy for this PR?
avg_to_sum_count.sltcovering shared SUM/COUNT, no-share cases, NULL/empty input, grouping sets, unsupported AVG forms, Decimal, and naming conflicts.cargo fmt --all, full workspace Clippy with-D warnings, and the extended workspace test suite.Release benchmark using TPC-H SF1 with the four Q1 Decimal inputs materialized as Float64 Parquet:
Are there any user-facing changes?
Adds an optional
AggregateUDFImpl::decomposehook with a default no-op implementation. Existing UDAFs do not need changes.Eligible Float64 AVG aggregates may use shared SUM/COUNT states. Query results and output schemas are unchanged.