neilconway commented on code in PR #22132:
URL: https://github.com/apache/datafusion/pull/22132#discussion_r3658830367
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
@@ -64,10 +64,10 @@ impl OptimizerRule for EliminateGroupByConstant {
.group_expr
.iter()
.partition(|expr| is_redundant_group_expr(expr,
&group_by_columns));
+ // Only eliminate when at least one required group expression
remains. A grouping
+ // aggregate emits 0 rows on empty input; a global aggregate
emits 1 NULL row.
Review Comment:
Slightly inaccurate: ungrouped COUNT returns a row with a 0, not a NULL. How
about this:
```suggestion
// Return now if no simplification can be done. We also
bail out
// if applying the optimization would eliminate all of the
// grouping expressions (e.g., GROUP BY on only constant
// expressions): this would turn a grouped aggregate into
an
// ungrouped aggregate, which changes query semantics
(grouped
// aggregates produce an empty result set on an empty
input,
// whereas ungrouped aggregates return a single row).
```
##########
datafusion/optimizer/src/eliminate_group_by_constant.rs:
##########
Review Comment:
Rename this test func so it doesn't imply that we eliminate constant
grouping keys in this case?
--
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]
