fix: Streaming aggregation produces wrong result with grouping sets - #24422
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #24422 +/- ##
==========================================
+ Coverage 81.17% 81.23% +0.05%
==========================================
Files 1109 1112 +3
Lines 388117 390652 +2535
Branches 388117 390652 +2535
==========================================
+ Hits 315071 317352 +2281
- Misses 54504 54660 +156
- Partials 18542 18640 +98 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| InputOrderMode::Linear | ||
| }; | ||
|
|
||
| // Ordering optimization is not compatible with grouping sets |
There was a problem hiding this comment.
Do we need to add some comments or todo for this?
There was a problem hiding this comment.
Updated, thanks!
4ddfc45 to
5673f3e
Compare
I don't think so - as there is no one ordering I think that will satisfy all the groupings |
alamb
left a comment
There was a problem hiding this comment.
Thanks @2010YOUY01 and @zhuqi-lucas
Is this fixing a regression in df 55 (aka there are potentially wrong results in df 55 that were correct in df 54)? If so, I'll add it to the df 55.1 tracking ticket
@alamb No, I've checked this bug can also be reproduced in Thank you for the review @zhuqi-lucas and @alamb , let's merge it. |
Which issue does this PR close?
Rationale for this change
The aggregation planning will enable ordering optimization when the grouping sets is available, however they're not compatible now.
I'm also not sure if we want to enable ordering optimization for grouping sets in the future, since this seems requires extra implementation complexity.
This PR disables ordering/streaming aggregation optimization when grouping set is present.
Reference for ordering optimization in aggregation:
datafusion/datafusion/physical-plan/src/aggregates/ordered_partial_stream.rs
Line 62 in 710e74e
What changes are included in this PR?
Are these changes tested?
slt
Are there any user-facing changes?
no