asolimando commented on code in PR #24039:
URL: https://github.com/apache/datafusion/pull/24039#discussion_r3715701066
##########
datafusion/physical-plan/src/aggregates/mod.rs:
##########
@@ -1596,7 +1671,13 @@ impl AggregateExec {
}
num_rows
} else if value == 0 {
- child_statistics.num_rows
+ // `limit_options` becomes a soft limit on the groups built
Review Comment:
I didn't manage to write such a regression test as an SLT test, as empty
grouping sets are part of partial aggregations, and, to my best understanding,
limits are not pushed below the final aggregation when grouping sets are
involved (as the partial and final groupbys differ, see
[this](https://github.com/apache/datafusion/blob/179b32c9b60103d9c4e6a4364f10f6286c963904/datafusion/physical-optimizer/src/limited_distinct_aggregation.rs#L105-L113)).
I could only build it as a unit test, but it would encode a subplan that can't
be reached in practice, so it didn't feel worth adding.
Re-reading that comment, I felt it could be rephrased at a higher level, in
terms of expected behavior, instead of describing how the execution side
respects that behavior, so I have reworded it in
[3dd5e18](https://github.com/apache/datafusion/pull/24039/commits/3dd5e18ebaaac674536d735af8cae72625c256a8).
While experimenting, I have found other SLT tests worth adding to
`grouping.slt`, all of which fail if the row-count expression is reverted:
1. `... GROUP BY GROUPING SETS((v1), (), ()) LIMIT 1` returns two rows
instead of one, since
`Exact(0)` makes the `GlobalLimitExec` look redundant and it is dropped
2. an outer `COUNT(*)` over that query returns 0 instead of 1, and 0 instead
of 2 with `LIMIT 5`
1. is particularly interesting, as it shows that a wrong estimate here can
corrupt results through limit elimination too, not only through `COUNT(*)`
folding (or MIN/MAX).
I have checked expected values against PostgreSQL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
