[GitHub] [spark] peter-toth commented on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

[GitBox <gi...@apache.org>]

peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813432427


   > > In general, GroupingExprRef should be a variant of Attribute. It's bound to the grouping expression in the current operator, instead of the output of child operator.
   > 
   > I see, thanks.
   
   @cloud-fan I've modified this PR to follow your suggested fix and introduced `GroupingExprRef` in https://github.com/apache/spark/commit/977c0bff247987eec7efd68bea8b33236b0c1612 and the current PR fixes the optimization issue in desctription.
   But I also realized that logical `Aggregate` and physical `BaseAggregateExec` implementations are very different in terms of grouping expressions. In the physical plan grouping expressions are `NamedExpression`s and aggregate expressions (without aggregate functions) already refer to the produced attributes of these expressions: (https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/planning/patterns.scala#L336-L343). So I wonder if `GroupingExprRef`s should appear in physical plans at all? In this current PR I haven't modified `PhysicalAggregation` so no `GroupingExprRef` appear in the physical plan.


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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org