alamb commented on code in PR #17139:
URL: https://github.com/apache/datafusion/pull/17139#discussion_r2270258731
##########
datafusion/sqllogictest/test_files/issue_17138.slt:
##########
@@ -0,0 +1,36 @@
+statement ok
Review Comment:
I recommend we name this file more specifically, or put it in an existing
function, perhaps aggregate.slt ?
Not a blocker, just a suggestion
##########
datafusion/optimizer/src/optimize_projections/mod.rs:
##########
@@ -153,23 +153,16 @@ fn optimize_projections(
// Only use the absolutely necessary aggregate expressions required
// by the parent:
- let mut new_aggr_expr =
aggregate_reqs.get_at_indices(&aggregate.aggr_expr);
-
- // Aggregations always need at least one aggregate expression.
- // With a nested count, we don't require any column as input, but
- // still need to create a correct aggregate, which may be optimized
- // out later. As an example, consider the following query:
- //
- // SELECT count(*) FROM (SELECT count(*) FROM [...])
- //
- // which always returns 1.
- if new_aggr_expr.is_empty()
- && new_group_bys.is_empty()
- && !aggregate.aggr_expr.is_empty()
- {
- // take the old, first aggregate expression
- new_aggr_expr = aggregate.aggr_expr;
- new_aggr_expr.resize_with(1, || unreachable!());
+ let new_aggr_expr =
aggregate_reqs.get_at_indices(&aggregate.aggr_expr);
+
+ if new_group_bys.is_empty() && new_aggr_expr.is_empty() {
+ // Global aggregation with no aggregate functions always
produces 1 row and no columns.
Review Comment:
this seems like a good rule to have, though perhaps it would be eaiser to
find if it were with related rules for aggregates, for example
https://github.com/apache/datafusion/blob/main/datafusion/optimizer/src/eliminate_group_by_constant.rs
##########
datafusion/sqllogictest/test_files/subquery.slt:
##########
@@ -1453,9 +1453,7 @@ logical_plan
01)LeftSemi Join:
02)--TableScan: t1 projection=[a]
03)--SubqueryAlias: __correlated_sq_1
-04)----Projection:
-05)------Aggregate: groupBy=[[]], aggr=[[count(Int64(1))]]
-06)--------TableScan: t2 projection=[]
+04)----EmptyRelation
Review Comment:
seems like an improvement to me
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
