findepi commented on code in PR #17139:
URL: https://github.com/apache/datafusion/pull/17139#discussion_r2270995227


##########
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.
+                return Ok(Transformed::yes(LogicalPlan::EmptyRelation(
+                    EmptyRelation {
+                        produce_one_row: true,
+                        schema: Arc::new(DFSchema::empty()),

Review Comment:
   Here we don't produce any symbols, but maybe we use `EmptyRelation` also for 
other cases?
   For exmple `SELECT a, b, c FROM t WHERE false` could be replaced with 
`EmptyRelation`, but something needs to project `a, b, c` symbols. It can be a 
project above `EmptyRelation` or part of `EmptyRelation` itself.
   
   A different question -- why do we have both, `EmptyRelation` and `Values`?
   The latter is strictly more generic, without any visible downsides. It even 
has a better name (`EmptyRelation` is only conditionally an empty relation).
   Can we deprecate `EmptyRelation` and replace with `Values`?
   



-- 
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

Reply via email to