Skip to content

fix(physical-plan): count empty grouping sets in the aggregate row estimate for an empty input - #24039

Open
asolimando wants to merge 3 commits into
apache:mainfrom
asolimando:asolimando/aggregate-empty-grouping-sets-stats
Open

fix(physical-plan): count empty grouping sets in the aggregate row estimate for an empty input#24039
asolimando wants to merge 3 commits into
apache:mainfrom
asolimando:asolimando/aggregate-empty-grouping-sets-stats

Conversation

@asolimando

Copy link
Copy Markdown
Member

Which issue does this PR close?

Rationale for this change

AggregateExec reported a row count of Exact(0) whenever its input had no rows, even when the
group-by contained an empty grouping set. GROUPING SETS(()), ROLLUP and CUBE each emit a
grand-total row for such an input, so the row count contradicted execution. The count was claimed
as Exact, so rules that answer a query from statistics rather than by executing it returned a
value that no row of the result holds:

SELECT COUNT(*) FROM (SELECT SUM(v1) FROM t WHERE false GROUP BY ROLLUP(v1));
-- 0, while the inner aggregate emits one row

This is a correctness fix, not an estimation improvement.

What changes are included in this PR?

  • estimate_num_rows counts the empty grouping sets instead of reusing the child's row count of
    zero. Partial aggregation emits the grand-total row from every output partition, so the count
    goes through the per-partition scaling already applied to aggregates without grouping
    expressions, now factored into scale_logical_rows.
  • The grouping columns of such an output are reported as NULL: typed null bounds, a distinct
    count of zero, and a null count equal to the row count.

Are these changes tested?

Yes.

  • SQL logic tests in grouping.slt for both folds: an outer COUNT(*) over ROLLUP, CUBE and
    GROUPING SETS, and an outer MIN/MAX over the grand-total row.
  • Unit tests for the reported statistics: zero rows for a plain GROUP BY, one row for
    GROUPING SETS((a), ()), two rows for GROUPING SETS((a), (), ()), the partition scaling of a
    partial aggregate, and the NULL grouping-column statistics.

Each test was checked to fail without the corresponding change.

Are there any user-facing changes?

Yes. Queries such as the one above now return the correct result. There are no API changes.


Disclaimer: I used AI to assist in the code generation, I have manually reviewed the output and it matches my intention and understanding.

@github-actions github-actions Bot added sqllogictest SQL Logic Tests (.slt) physical-plan Changes to the physical-plan crate labels Jul 31, 2026
@codecov-commenter

codecov-commenter commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 89.41176% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.86%. Comparing base (dbcb5c0) to head (42161dc).
⚠️ Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
datafusion/physical-plan/src/aggregates/mod.rs 89.41% 5 Missing and 13 partials ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #24039    +/-   ##
========================================
  Coverage   80.85%   80.86%            
========================================
  Files        1101     1101            
  Lines      374933   375633   +700     
  Branches   374933   375633   +700     
========================================
+ Hits       303166   303746   +580     
- Misses      53671    53775   +104     
- Partials    18096    18112    +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

COUNT(*) over an empty grouping set with empty input returns 0 (should return 1)

2 participants