You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2021/03/21 09:37:55 UTC

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

peter-toth opened a new pull request #31913:
URL: https://github.com/apache/spark/pull/31913


   ### What changes were proposed in this pull request?
   The `CollapseProject` rule can collapse a `Project` over an `Aggregate` node into one `Aggregate` node. If the expressions in the `Project` and `Aggregate` nodes are complex ones then further optimizations can happen on the collapsed expressions. These optimizations can result that an expression without an aggregate function in the `Aggregate` node doesn't reference to any grouping expressions any longer.
   
   Here is a simple example:
   ```
   SELECT not(id)
   FROM (
     SELECT t.id IS NULL AS id
     FROM t
     GROUP BY t.id IS NULL
   ) t
   ```
   In this case the `CollapseProject` does this:
   ```
   === Applying Rule org.apache.spark.sql.catalyst.optimizer.CollapseProject ===
   !Project [NOT id#224 AS (NOT id)#227, c#225L]                                    Aggregate [isnull(id#222)], [NOT isnull(id#222) AS (NOT id)#227, count(1) AS c#225L]
   !+- Aggregate [isnull(id#222)], [isnull(id#222) AS id#224, count(1) AS c#225L]   +- Project [value#219 AS id#222]
   !   +- Project [value#219 AS id#222]                                                +- LocalRelation [value#219]
   !      +- LocalRelation [value#219]                                              
   ```
   and then `NOT isnull(id#222)` is optimized to `isnull(id#222)` and so no longer refers to any grouping expression:
   ```
   === Applying Rule org.apache.spark.sql.catalyst.optimizer.BooleanSimplification ===
   !Aggregate [isnull(id#222)], [NOT isnull(id#222) AS (NOT id)#227, count(1) AS c#225L]   Aggregate [isnull(id#222)], [isnotnull(id#222) AS (NOT id)#227, count(1) AS c#225L]
    +- Project [value#219 AS id#222]                                                       +- Project [value#219 AS id#222]
       +- LocalRelation [value#219]                                                           +- LocalRelation [value#219]
   ```
   
   This PR introduces `GroupingExpression` as a simple wrapper around complex grouping expressions to prevent further optimizations on them.
   
   Before this PR:
   ```
   (5) HashAggregate
   Input [2]: [isnull(id#222)#230, count#232L]
   Keys [1]: [isnull(id#222)#230]
   Functions [1]: [count(1)]
   Aggregate Attributes [1]: [count(1)#226L]
   Results [2]: [isnotnull(id#222) AS (NOT id)#227, count(1)#226L AS c#225L]
   ```
   and running the query throws an error:
   ```
   Couldn't find id#222 in [isnull(id#222)#230,count(1)#226L]
   java.lang.IllegalStateException: Couldn't find id#222 in [isnull(id#222)#230,count(1)#226L]
   ```
   
   After this PR:
   ```
   (5) HashAggregate
   Input [2]: [isnull(id#222)#242, count#238L]
   Keys [1]: [isnull(id#222)#242]
   Functions [1]: [count(1)]
   Aggregate Attributes [1]: [count(1)#226L]
   Results [2]: [NOT groupingexpression(isnull(id#222)#242) AS (NOT id)#227, count(1)#226L AS c#225L]
   ```
   and the query works.
   
   
   ### Why are the changes needed?
   To keep grouping expressions in aggregate expressions when it is required for an aggregate expression to refer to a grouping expression.
   
   
   ### Does this PR introduce _any_ user-facing change?
   Yes, the query works.
   
   
   ### How was this patch tested?
   Added new UT.
   


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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822166011


   thanks, merging to master!


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-809571526


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136668/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819340008






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

[GitHub] [spark] sigmod edited a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
sigmod edited a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825318373


   Posted a message on the git commit page and copy it here.
   
   Hi @peter-toth
   
   @cloud-fan @maryannxue @rednaxelafx and I had an offline discussion about this PR.
   
   Here's a summary:
   
   (1) This PR would bring complexities when we add more optimization around grouping columns down the road. E.g., if we want to reorder grouping columns in order to reuse partitioning properties or optimize away certain grouping columns, we would need to re-generate the ordinals in grouping refs.
   (2) It seems safe to pull out non-attribute grouping expressions from an Aggregate and put them in a Project below the Aggregate. Then, grouping expressions in the Aggregate are only AttributeReferences. The rest of Optimizer rules can still match the Aggregate as before. We do need to pay attention to rules that collapse Aggregate and Project. In the planner, we can combine Aggregate and Project, so that the physical plan doesn't change.
   
   Thus, would you mind reverting this PR and try the proposal in (2)? Thanks in advance!
   
   Best,
   Yingyi


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813013052


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41471/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804046361


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136337/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813098284


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136896/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611788588



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -655,12 +666,76 @@ case class Aggregate(
     }
   }
 
+  private def expandGroupingReferences(e: Expression): Expression = {
+    e match {
+      case _: AggregateExpression => e
+      case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+      case g: GroupingExprRef => groupingExpressions(g.ordinal)
+      case _ => e.mapChildren(expandGroupingReferences)
+    }
+  }
+
+  lazy val aggregateExpressionsWithoutGroupingRefs = {
+    aggregateExpressions.map(expandGroupingReferences(_).asInstanceOf[NamedExpression])
+  }
+
   override lazy val validConstraints: ExpressionSet = {
     val nonAgg = aggregateExpressions.filter(_.find(_.isInstanceOf[AggregateExpression]).isEmpty)
     getAllValidConstraints(nonAgg)
   }
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (ge.deterministic && !ge.foldable && ge.children.nonEmpty &&

Review comment:
       Indeed, `PullOutNondeterministic` is taking care of them. Fixed in https://github.com/apache/spark/pull/31913/commits/06224449062e2f1e97a419adf44de45dce9eeb6d.
   
   `GroupingExprRef` is not needed when it would refer to a simple `Attribute`. The purpose of `GroupingExprRef`s to wrap expressions that could change during optimization, I don't think that a simple attribute could change.
   Also, `GroupingExprRef` is not a `NamedExpression` now. If we wanted to allow `GroupingExprRef` to refer to a simple attribute in aggregate expressions then either it should implement `NamedExpression` or it should be wrapped into a new `Alias`.
   

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -795,7 +795,7 @@ object NullPropagation extends Rule[LogicalPlan] {
  */
 object FoldablePropagation extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = {
-    CleanupAliases(propagateFoldables(plan)._1)
+    EnforceGroupingReferencesInAggregates(CleanupAliases(propagateFoldables(plan)._1))

Review comment:
       Fixed in https://github.com/apache/spark/pull/31913/commits/06224449062e2f1e97a419adf44de45dce9eeb6d.




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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-806711502


   So, I was thinking a bit about this `case class GroupingExprRef(ordinal: Int)` and I don't know how we could implement its `eval()` (without changing the ` <: BaseAggregateExec` implementations) or how could we keep its properties in sync with the possible changing properties of the referenced grouping expression? Like `.nullable()` of the referenced can change during optimization and `GroupingExprRef` should be in sync with that so as to allow further optimizations on aggregate expressions.
   
   Obviously we could capture the referenced grouping expression itself instead of the ordinal (like `ReusedExchangeExec` captures the referenced `Excange`), but transformations on the plan can easily replace the referenced to a different instance and so ruining our `GroupingExprRef`.
   (Sidenote: Actually `ReusedExchangeExec`/`ReusedSubquery` nodes only work because the reuse rules are the last plan changing ones before the execution begins and we still have issues with these rules: https://github.com/apache/spark/pull/28885) 


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812892996


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136882/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819417506


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137336/
   


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612561296



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e

Review comment:
       What if we have `a + b + rand()` while `a + b` is a grouping expression?




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822377537






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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820360645


   **[Test build #137416 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137416/testReport)** for PR 31913 at commit [`72c173b`](https://github.com/apache/spark/commit/72c173beebaf5ec63a9fd9360410b355462943d1).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819417506


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137336/
   


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612555562



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e
+        case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+        case _ if groupingExpressions.contains(e.canonicalized) =>
+          val (groupingExpression, ordinal) = groupingExpressions(e.canonicalized)
+          GroupingExprRef(ordinal, groupingExpression.dataType, groupingExpression.nullable)
+        case _ => e.mapChildren(insertGroupingExprRefs)
+      }
+    }
+
+    aggregateExpressions.map(insertGroupingExprRefs(_).asInstanceOf[NamedExpression])
+  }
+
+  def withGroupingRefs(
+      groupingExpressions: Seq[Expression],
+      aggregateExpressions: Seq[NamedExpression],
+      child: LogicalPlan): Aggregate = {
+    val dealiasedGroupingExpressions = groupingExpressions.map {
+      case a: Alias => a.child

Review comment:
       Do we need to do this? IIUC analyzer will strip unnecessary aliases from grouping expressions, by the rule `CleanupAliases`




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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612544670



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/UpdateNullability.scala
##########
@@ -52,3 +52,22 @@ object UpdateAttributeNullability extends Rule[LogicalPlan] {
       }
   }
 }
+
+/**
+ * Updates nullability of [[GroupingExprRef]]s in a resolved LogicalPlan by using the nullability of
+ * referenced grouping expression.
+ */
+object UpdateGroupingExprRefNullability extends Rule[LogicalPlan] {
+  def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsUp {

Review comment:
       This is an optimizer rule, shall we use `transformUp`?




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817277585


   **[Test build #137178 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137178/testReport)** for PR 31913 at commit [`c2ba804`](https://github.com/apache/spark/commit/c2ba80457bd86d11ad26311bbc3c42607f33b19a).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820387280






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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612560780



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e
+        case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+        case _ if groupingExpressions.contains(e.canonicalized) =>
+          val (groupingExpression, ordinal) = groupingExpressions(e.canonicalized)
+          GroupingExprRef(ordinal, groupingExpression.dataType, groupingExpression.nullable)
+        case _ => e.mapChildren(insertGroupingExprRefs)
+      }
+    }
+
+    aggregateExpressions.map(insertGroupingExprRefs(_).asInstanceOf[NamedExpression])
+  }
+
+  def withGroupingRefs(
+      groupingExpressions: Seq[Expression],
+      aggregateExpressions: Seq[NamedExpression],
+      child: LogicalPlan): Aggregate = {
+    val dealiasedGroupingExpressions = groupingExpressions.map {
+      case a: Alias => a.child
+      case o => o
+    }
+    val complexGroupingExpressions =
+      collectComplexGroupingExpressions(dealiasedGroupingExpressions)
+    val aggrExprWithGroupingReferences = if (complexGroupingExpressions.nonEmpty) {
+      insertGroupingReferences(aggregateExpressions, complexGroupingExpressions)

Review comment:
       I agree that it's not necessary to replace `Attribute` with `GroupingExprRef`, but it makes the framework more consistent if we always use `GroupingExprRef`. We can make `GroupingExprRef` a named expression to fix issues.
   
   This is just my thought though, I'm happy to hear more opinions.




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805076974


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136400/
   


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819497235


   **[Test build #137349 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137349/testReport)** for PR 31913 at commit [`cff9b9a`](https://github.com/apache/spark/commit/cff9b9ab289412529728749001ded9550f14e66e).


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r613035645



##########
File path: sql/core/src/test/resources/sql-tests/inputs/group-by.sql
##########
@@ -179,3 +179,8 @@ SELECT count(*) FROM test_agg WHERE k = 1 or k = 2 or count(*) + 1L > 1L or max(
 
 -- Aggregate with multiple distinct decimal columns
 SELECT AVG(DISTINCT decimal_col), SUM(DISTINCT decimal_col) FROM VALUES (CAST(1 AS DECIMAL(9, 0))) t(decimal_col);
+
+-- SPARK-34581: Don't optimize out grouping expressions from aggregate expressions without aggregate function
+SELECT not(a IS NULL), count(*) AS c
+FROM testData
+GROUP BY a IS NULL

Review comment:
       can we also test `select a + b + rand() .... group by a + b`？




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

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

Posted by GitBox <gi...@apache.org>.
peter-toth edited a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803548910


   The `Not(IsNull(e)) => IsNotNull(e)` and `Not(IsNotNull(e)) => IsNull(e)` optimizations were introduced in https://github.com/apache/spark/pull/25878 so the example query is a regression from 2.4 to 3.x, but I'm not sure that the issue doesn't come up in Spark 2.4 with other optimizations.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804041041


   **[Test build #136337 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136337/testReport)** for PR 31913 at commit [`3de19ca`](https://github.com/apache/spark/commit/3de19cae711790a4c520221da1965277011f6f65).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813057385


   **[Test build #136896 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136896/testReport)** for PR 31913 at commit [`977c0bf`](https://github.com/apache/spark/commit/977c0bff247987eec7efd68bea8b33236b0c1612).


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819536053


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41928/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819731883


   **[Test build #137349 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137349/testReport)** for PR 31913 at commit [`cff9b9a`](https://github.com/apache/spark/commit/cff9b9ab289412529728749001ded9550f14e66e).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818285463


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137230/
   


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

[GitHub] [spark] cloud-fan edited a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan edited a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825470374


   I think the new idea should also be simpler:
   1. `CollapseProject` only collpase Project + Aggregate, but not Aggregate + Project. No change is needed. For example:
   ```
   scala> spark.range(10).select('id + 1 as 'a).groupBy('a).count().explain(true)
   == Parsed Logical Plan ==
   'Aggregate ['a], ['a, count(1) AS count#17L]
   +- Project [(id#11L + cast(1 as bigint)) AS a#13L]
      +- Range (0, 10, step=1, splits=Some(1))
   
   == Analyzed Logical Plan ==
   a: bigint, count: bigint
   Aggregate [a#13L], [a#13L, count(1) AS count#17L]
   +- Project [(id#11L + cast(1 as bigint)) AS a#13L]
      +- Range (0, 10, step=1, splits=Some(1))
   
   == Optimized Logical Plan ==
   Aggregate [a#13L], [a#13L, count(1) AS count#17L]
   +- Project [(id#11L + 1) AS a#13L]
      +- Range (0, 10, step=1, splits=Some(1))
   
   == Physical Plan ==
   AdaptiveSparkPlan isFinalPlan=false
   +- HashAggregate(keys=[a#13L], functions=[count(1)], output=[a#13L, count#17L])
      +- HashAggregate(keys=[a#13L], functions=[partial_count(1)], output=[a#13L, count#21L])
         +- Project [(id#11L + 1) AS a#13L]
            +- Range (0, 10, step=1, splits=1)
   ```
   2. `PhysicalAggregation` needs update to collapse Project under Aggregate.
   
   Anyway, I'm reverting this PR, @peter-toth look forward to your new PR!


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822220836






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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803555086


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40888/
   


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817994781


   **[Test build #137230 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137230/testReport)** for PR 31913 at commit [`343f35e`](https://github.com/apache/spark/commit/343f35eccf4db5f47dbf6a08eed18ab90a3e528b).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813094624


   **[Test build #136896 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136896/testReport)** for PR 31913 at commit [`977c0bf`](https://github.com/apache/spark/commit/977c0bff247987eec7efd68bea8b33236b0c1612).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `case class GroupingExprRef(`
     * `case class AggregateWithGroupingReferences(`


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611788925



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -795,7 +795,7 @@ object NullPropagation extends Rule[LogicalPlan] {
  */
 object FoldablePropagation extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = {
-    CleanupAliases(propagateFoldables(plan)._1)
+    EnforceGroupingReferencesInAggregates(CleanupAliases(propagateFoldables(plan)._1))

Review comment:
       Hmm, sorry, I don't think this is needed. I will revert this in the next commit...




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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r613014125



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e
+        case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+        case _ if groupingExpressions.contains(e.canonicalized) =>
+          val (groupingExpression, ordinal) = groupingExpressions(e.canonicalized)
+          GroupingExprRef(ordinal, groupingExpression.dataType, groupingExpression.nullable)
+        case _ => e.mapChildren(insertGroupingExprRefs)
+      }
+    }
+
+    aggregateExpressions.map(insertGroupingExprRefs(_).asInstanceOf[NamedExpression])
+  }
+
+  def withGroupingRefs(
+      groupingExpressions: Seq[Expression],
+      aggregateExpressions: Seq[NamedExpression],
+      child: LogicalPlan): Aggregate = {
+    val dealiasedGroupingExpressions = groupingExpressions.map {
+      case a: Alias => a.child
+      case o => o
+    }
+    val complexGroupingExpressions =
+      collectComplexGroupingExpressions(dealiasedGroupingExpressions)
+    val aggrExprWithGroupingReferences = if (complexGroupingExpressions.nonEmpty) {
+      insertGroupingReferences(aggregateExpressions, complexGroupingExpressions)

Review comment:
       SGTM




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819381256


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137335/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813043176


   **[Test build #136894 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136894/testReport)** for PR 31913 at commit [`86047df`](https://github.com/apache/spark/commit/86047df797dbfb157a24ec71a285bea99db4f600).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813047036


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136894/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822166597


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42127/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607202293



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -622,31 +624,47 @@ case class Range(
 /**
  * This is a Group by operator with the aggregate functions and projections.
  *
- * @param groupingExpressions expressions for grouping keys
- * @param aggregateExpressions expressions for a project list, which could contain
- *                             [[AggregateExpression]]s.
+ * @param groupingExpressions Expressions for grouping keys.
+ * @param aggrExprWithGroupingRefs Expressions for a project list, which could contain
+ *                                 [[AggregateExpression]]s and [[GroupingExprRef]]s.
+ * @param child The child of the aggregate node.
+ * @param enforceGroupingReferences If [[aggrExprWithGroupingRefs]] should contain
+ *                                  [[GroupingExprRef]]s.
+ *
+ * [[aggrExprWithGroupingRefs]] without aggregate functions can contain [[GroupingExprRef]]
+ * expressions to refer to complex grouping expressions in [[groupingExpressions]]. These references
+ * ensure that optimization rules don't change the aggregate expressions to invalid ones that no
+ * longer refer to any grouping expressions and also simplify the expression transformations on the
+ * node (need to transform the expression only once).
  *
- * Note: Currently, aggregateExpressions is the project list of this Group by operator. Before
- * separating projection from grouping and aggregate, we should avoid expression-level optimization
- * on aggregateExpressions, which could reference an expression in groupingExpressions.
- * For example, see the rule [[org.apache.spark.sql.catalyst.optimizer.SimplifyExtractValueOps]]
+ * For example, in the following query Spark shouldn't optimize the aggregate expression
+ * `Not(IsNull(c))` to `IsNotNull(c)` as the grouping expression is `IsNull(c)`:
+ * SELECT not(c IS NULL)
+ * FROM t
+ * GROUP BY c IS NULL
+ * Instead, the aggregate expression should contain `Not(GroupingExprRef(0))`.
  */
-case class Aggregate(
+case class AggregateWithGroupingReferences(

Review comment:
       During the optimization phase some of the rules need the "expanded" version of aggregate expressions (references are restored to original expressions). This is why I wanted to keep the meaning of `Aggregate.aggregateExpressions` and the second argument of the `Aggregate.unapply()` extractor. Also when an optimizer rule creates an aggregate using `Aggregete.apply()` then it should automatically insert references the grouping expressions.
   I think it would be error prone if the optimizer rules need to keep in mind that they need to convert/restore these references and it is probably better to convert/restore references transparently. 




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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-806721780


   I assume `GroupingExprRef` should be similar to `Attribute`  and needs to be bound to `BoundReference` before execution.
   
   You made a good point that properties like `nullable` may go out of sync. Actually the same applies to `Attribute` as well. The solution we made for `Attribute` is the rule `UpdateAttributeNullability`. We can update nullability of `GroupingExprRef` in that rule as well.
   
   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.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818929089


   **[Test build #137300 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137300/testReport)** for PR 31913 at commit [`2e79eb9`](https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805095549


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40993/
   


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

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

Posted by GitBox <gi...@apache.org>.
sigmod commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825318373


   Posted a message on the git commit page and copy it here.
   
   Hi @peter-toth
   
   @cloud-fan @maryannxue @rednaxelafx and I had an offline discussion about PR.
   
   Here's a summary:
   
   (1) This PR would bring complexities when we add more optimization around grouping columns down the road. E.g., if we want to reorder grouping columns in order to reuse partitioning properties or optimize away certain grouping columns, we would need to re-generate the ordinals in grouping refs.
   (2) It seems safe to pull out non-attribute grouping expressions from an Aggregate and put them in a Project below the Aggregate. Then, grouping expressions in the Aggregate are only AttributeReferences. The rest of Optimizer rules can still match the Aggregate as before. We do need to pay attention to rules that collapse Aggregate and Project. In the planner, we can combine Aggregate and Project, so that the physical plan doesn't change.
   Thus, would you mind reverting this PR and try the proposal in (2)? Thanks in advance!
   
   Best,
   Yingyi


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607202293



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -622,31 +624,47 @@ case class Range(
 /**
  * This is a Group by operator with the aggregate functions and projections.
  *
- * @param groupingExpressions expressions for grouping keys
- * @param aggregateExpressions expressions for a project list, which could contain
- *                             [[AggregateExpression]]s.
+ * @param groupingExpressions Expressions for grouping keys.
+ * @param aggrExprWithGroupingRefs Expressions for a project list, which could contain
+ *                                 [[AggregateExpression]]s and [[GroupingExprRef]]s.
+ * @param child The child of the aggregate node.
+ * @param enforceGroupingReferences If [[aggrExprWithGroupingRefs]] should contain
+ *                                  [[GroupingExprRef]]s.
+ *
+ * [[aggrExprWithGroupingRefs]] without aggregate functions can contain [[GroupingExprRef]]
+ * expressions to refer to complex grouping expressions in [[groupingExpressions]]. These references
+ * ensure that optimization rules don't change the aggregate expressions to invalid ones that no
+ * longer refer to any grouping expressions and also simplify the expression transformations on the
+ * node (need to transform the expression only once).
  *
- * Note: Currently, aggregateExpressions is the project list of this Group by operator. Before
- * separating projection from grouping and aggregate, we should avoid expression-level optimization
- * on aggregateExpressions, which could reference an expression in groupingExpressions.
- * For example, see the rule [[org.apache.spark.sql.catalyst.optimizer.SimplifyExtractValueOps]]
+ * For example, in the following query Spark shouldn't optimize the aggregate expression
+ * `Not(IsNull(c))` to `IsNotNull(c)` as the grouping expression is `IsNull(c)`:
+ * SELECT not(c IS NULL)
+ * FROM t
+ * GROUP BY c IS NULL
+ * Instead, the aggregate expression should contain `Not(GroupingExprRef(0))`.
  */
-case class Aggregate(
+case class AggregateWithGroupingReferences(

Review comment:
       During the optimization phase some of the rules need the "expanded" version of aggregate expressions (references are restored to original expressions). This is why I wanted to keep the meaning of `Aggregate.aggregateExpressions` and the second argument of the `Aggregate.unapply()` extractor. Also when an optimizer rule creates an aggregate using `Aggregete.apply()` then it should insert references to grouping expressions to avoid issues like the one in description with subsequent optimizer rules.  
   I think it would be error prone if the optimizer rules need to keep in mind that they need to convert/restore these references and it is probably better to convert/restore references transparently. 




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819340770


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41915/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612682891



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e
+        case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+        case _ if groupingExpressions.contains(e.canonicalized) =>
+          val (groupingExpression, ordinal) = groupingExpressions(e.canonicalized)
+          GroupingExprRef(ordinal, groupingExpression.dataType, groupingExpression.nullable)
+        case _ => e.mapChildren(insertGroupingExprRefs)
+      }
+    }
+
+    aggregateExpressions.map(insertGroupingExprRefs(_).asInstanceOf[NamedExpression])
+  }
+
+  def withGroupingRefs(
+      groupingExpressions: Seq[Expression],
+      aggregateExpressions: Seq[NamedExpression],
+      child: LogicalPlan): Aggregate = {
+    val dealiasedGroupingExpressions = groupingExpressions.map {
+      case a: Alias => a.child

Review comment:
       It does so let's give a try to removing this part. Changed in https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a.




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819745010


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137349/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819696226


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41940/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813006354


   **[Test build #136894 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136894/testReport)** for PR 31913 at commit [`86047df`](https://github.com/apache/spark/commit/86047df797dbfb157a24ec71a285bea99db4f600).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607126167



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/ComplexTypes.scala
##########
@@ -18,23 +18,14 @@
 package org.apache.spark.sql.catalyst.optimizer
 
 import org.apache.spark.sql.catalyst.expressions._
-import org.apache.spark.sql.catalyst.plans.logical.{Aggregate, LogicalPlan}
+import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
 import org.apache.spark.sql.catalyst.rules.Rule
 
 /**
  * Simplify redundant [[CreateNamedStruct]], [[CreateArray]] and [[CreateMap]] expressions.
  */
 object SimplifyExtractValueOps extends Rule[LogicalPlan] {
   override def apply(plan: LogicalPlan): LogicalPlan = plan transform {
-    // One place where this optimization is invalid is an aggregation where the select

Review comment:
       We can remove this limitation now.




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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612562805



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala
##########
@@ -96,7 +96,7 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
         .groupBy('a + 'b)(('a + 'b) as 'c)
         .analyze
       val optimized = Optimize.execute(query)
-      comparePlans(optimized, expected)
+      comparePlans(optimized, EnforceGroupingReferencesInAggregates(expected))

Review comment:
       hmm, the `Optimize` in this test doesn't include the new rule `EnforceGroupingReferencesInAggregates`, why would the `optimzed` plan contains grouping refs?




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813064991


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41473/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812886818


   **[Test build #136882 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136882/testReport)** for PR 31913 at commit [`de95a9a`](https://github.com/apache/spark/commit/de95a9aa13e5c763b4d8f7e87e1f1fe99dea466a).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `case class GroupingExprRef(`
     * `case class AggregateWithGroupingReferences(`


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817284155


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41756/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822340167


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137552/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821860619


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42083/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822336716


   **[Test build #137552 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137552/testReport)** for PR 31913 at commit [`34f0439`](https://github.com/apache/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `public class VectorizedBLAS extends F2jBLAS `
     * `class PandasOnSparkFrameMethods(object):`
     * `class PandasOnSparkSeriesMethods(object):`
     * `class PandasOnSparkPlotAccessor(PandasObject):`
     * `class PandasOnSparkBarPlot(PandasBarPlot, TopNPlotBase):`
     * `class PandasOnSparkBoxPlot(PandasBoxPlot, BoxPlotBase):`
     * `class PandasOnSparkHistPlot(PandasHistPlot, HistogramPlotBase):`
     * `class PandasOnSparkPiePlot(PandasPiePlot, TopNPlotBase):`
     * `class PandasOnSparkAreaPlot(PandasAreaPlot, SampledPlotBase):`
     * `class PandasOnSparkLinePlot(PandasLinePlot, SampledPlotBase):`
     * `class PandasOnSparkBarhPlot(PandasBarhPlot, TopNPlotBase):`
     * `class PandasOnSparkScatterPlot(PandasScatterPlot, TopNPlotBase):`
     * `class PandasOnSparkKdePlot(PandasKdePlot, KdePlotBase):`
     * `class PandasOnSparkUsageLogger(object):`
     * `trait AnalysisOnlyCommand extends Command `
     * `  implicit class MetadataColumnHelper(attr: Attribute) `
     * `case class WriteToDataSourceV2(`
     * `case class WriteToMicroBatchDataSource(`


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819681117






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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-826971452


   I've started working on the new PR. I'm a bit busy this week, but will try to open it by the end of this week.


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818032842


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41810/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r598382819



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -870,8 +870,19 @@ object CollapseProject extends Rule[LogicalPlan] with AliasHelper {
       if (haveCommonNonDeterministicOutput(p.projectList, agg.aggregateExpressions)) {
         p
       } else {
-        agg.copy(aggregateExpressions = buildCleanedProjectList(
-          p.projectList, agg.aggregateExpressions))
+        val complexGroupingExpressions =
+          ExpressionSet(agg.groupingExpressions.filter(_.children.nonEmpty))
+
+        def wrapGroupingExpression(e: Expression): Expression = e match {
+          case _: AggregateExpression => e
+          case _ if complexGroupingExpressions.contains(e) => GroupingExpression(e)
+          case _ => e.mapChildren(wrapGroupingExpression)
+        }
+
+        val wrappedAggregateExpressions =
+          agg.aggregateExpressions.map(wrapGroupingExpression(_).asInstanceOf[NamedExpression])
+        agg.copy(aggregateExpressions =
+          buildCleanedProjectList(p.projectList, wrappedAggregateExpressions))

Review comment:
       Ahh you are right, even the initial query can contain such a term in which Spark shoud keep the grouping expression. I think I just placed the `GroupingExpression` wrapper around `id IS NULL` at the wrong place, maybe it should happen during analysis. Will look into it...




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822152807


   **[Test build #137552 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137552/testReport)** for PR 31913 at commit [`34f0439`](https://github.com/apache/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804934176


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40984/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825454129


   @sigmod, @cloud-fan, I'm ok with reverting this change and try the new proposal if it better fits the roadmap. Probably the new rule should be similar to `PullOutNondeterministic`. Do you have a PR ready or shall I open a new one?


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819855885


   **[Test build #137361 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137361/testReport)** for PR 31913 at commit [`78296a8`](https://github.com/apache/spark/commit/78296a86585e9f0cad5c27fa534aa614a710793e).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817277585


   **[Test build #137178 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137178/testReport)** for PR 31913 at commit [`c2ba804`](https://github.com/apache/spark/commit/c2ba80457bd86d11ad26311bbc3c42607f33b19a).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611206703



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -622,31 +624,47 @@ case class Range(
 /**
  * This is a Group by operator with the aggregate functions and projections.
  *
- * @param groupingExpressions expressions for grouping keys
- * @param aggregateExpressions expressions for a project list, which could contain
- *                             [[AggregateExpression]]s.
+ * @param groupingExpressions Expressions for grouping keys.
+ * @param aggrExprWithGroupingRefs Expressions for a project list, which could contain
+ *                                 [[AggregateExpression]]s and [[GroupingExprRef]]s.
+ * @param child The child of the aggregate node.
+ * @param enforceGroupingReferences If [[aggrExprWithGroupingRefs]] should contain
+ *                                  [[GroupingExprRef]]s.
+ *
+ * [[aggrExprWithGroupingRefs]] without aggregate functions can contain [[GroupingExprRef]]
+ * expressions to refer to complex grouping expressions in [[groupingExpressions]]. These references
+ * ensure that optimization rules don't change the aggregate expressions to invalid ones that no
+ * longer refer to any grouping expressions and also simplify the expression transformations on the
+ * node (need to transform the expression only once).
  *
- * Note: Currently, aggregateExpressions is the project list of this Group by operator. Before
- * separating projection from grouping and aggregate, we should avoid expression-level optimization
- * on aggregateExpressions, which could reference an expression in groupingExpressions.
- * For example, see the rule [[org.apache.spark.sql.catalyst.optimizer.SimplifyExtractValueOps]]
+ * For example, in the following query Spark shouldn't optimize the aggregate expression
+ * `Not(IsNull(c))` to `IsNotNull(c)` as the grouping expression is `IsNull(c)`:
+ * SELECT not(c IS NULL)
+ * FROM t
+ * GROUP BY c IS NULL
+ * Instead, the aggregate expression should contain `Not(GroupingExprRef(0))`.
  */
-case class Aggregate(
+case class AggregateWithGroupingReferences(

Review comment:
       I think you are right, only a very few of them seems to really need expanded aggregate expressions ( `RemoveRedundantAggregates`, `RewriteCorrelatedScalarSubquery`). In https://github.com/apache/spark/commit/c2ba80457bd86d11ad26311bbc3c42607f33b19a I dropped handling group references transparently.




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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607202293



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -622,31 +624,47 @@ case class Range(
 /**
  * This is a Group by operator with the aggregate functions and projections.
  *
- * @param groupingExpressions expressions for grouping keys
- * @param aggregateExpressions expressions for a project list, which could contain
- *                             [[AggregateExpression]]s.
+ * @param groupingExpressions Expressions for grouping keys.
+ * @param aggrExprWithGroupingRefs Expressions for a project list, which could contain
+ *                                 [[AggregateExpression]]s and [[GroupingExprRef]]s.
+ * @param child The child of the aggregate node.
+ * @param enforceGroupingReferences If [[aggrExprWithGroupingRefs]] should contain
+ *                                  [[GroupingExprRef]]s.
+ *
+ * [[aggrExprWithGroupingRefs]] without aggregate functions can contain [[GroupingExprRef]]
+ * expressions to refer to complex grouping expressions in [[groupingExpressions]]. These references
+ * ensure that optimization rules don't change the aggregate expressions to invalid ones that no
+ * longer refer to any grouping expressions and also simplify the expression transformations on the
+ * node (need to transform the expression only once).
  *
- * Note: Currently, aggregateExpressions is the project list of this Group by operator. Before
- * separating projection from grouping and aggregate, we should avoid expression-level optimization
- * on aggregateExpressions, which could reference an expression in groupingExpressions.
- * For example, see the rule [[org.apache.spark.sql.catalyst.optimizer.SimplifyExtractValueOps]]
+ * For example, in the following query Spark shouldn't optimize the aggregate expression
+ * `Not(IsNull(c))` to `IsNotNull(c)` as the grouping expression is `IsNull(c)`:
+ * SELECT not(c IS NULL)
+ * FROM t
+ * GROUP BY c IS NULL
+ * Instead, the aggregate expression should contain `Not(GroupingExprRef(0))`.
  */
-case class Aggregate(
+case class AggregateWithGroupingReferences(

Review comment:
       During the optimization phase some of the rules need the "expanded" version of aggregate expressions (references are restored to original expressions). This is why I wanted to keep the meaning of `Aggregate.aggregateExpressions` and the second argument of the `Aggregate.unapply()` extractor. Also when an optimizer rule creates an aggregate with the `Aggregete.apply()` it should automatically insert references the grouping expressions.
   I think it would be error prone if the optimizer rules need to keep in mind that they need to convert/restore these references.




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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812664868


   **[Test build #136867 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136867/testReport)** for PR 31913 at commit [`fbf5a92`](https://github.com/apache/spark/commit/fbf5a9235db944e6c4e77a1a130a61d4b2f7f507).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813012823






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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612677707



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/UpdateNullability.scala
##########
@@ -52,3 +52,22 @@ object UpdateAttributeNullability extends Rule[LogicalPlan] {
       }
   }
 }
+
+/**
+ * Updates nullability of [[GroupingExprRef]]s in a resolved LogicalPlan by using the nullability of
+ * referenced grouping expression.
+ */
+object UpdateGroupingExprRefNullability extends Rule[LogicalPlan] {
+  def apply(plan: LogicalPlan): LogicalPlan = plan resolveOperatorsUp {

Review comment:
       Sure, fixed in https://github.com/apache/spark/pull/31913/commits/2e79eb910f5d4ef60227f7352e86dbed2786cb7a




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821892472


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137509/
   


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607620303



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -622,31 +624,47 @@ case class Range(
 /**
  * This is a Group by operator with the aggregate functions and projections.
  *
- * @param groupingExpressions expressions for grouping keys
- * @param aggregateExpressions expressions for a project list, which could contain
- *                             [[AggregateExpression]]s.
+ * @param groupingExpressions Expressions for grouping keys.
+ * @param aggrExprWithGroupingRefs Expressions for a project list, which could contain
+ *                                 [[AggregateExpression]]s and [[GroupingExprRef]]s.
+ * @param child The child of the aggregate node.
+ * @param enforceGroupingReferences If [[aggrExprWithGroupingRefs]] should contain
+ *                                  [[GroupingExprRef]]s.
+ *
+ * [[aggrExprWithGroupingRefs]] without aggregate functions can contain [[GroupingExprRef]]
+ * expressions to refer to complex grouping expressions in [[groupingExpressions]]. These references
+ * ensure that optimization rules don't change the aggregate expressions to invalid ones that no
+ * longer refer to any grouping expressions and also simplify the expression transformations on the
+ * node (need to transform the expression only once).
  *
- * Note: Currently, aggregateExpressions is the project list of this Group by operator. Before
- * separating projection from grouping and aggregate, we should avoid expression-level optimization
- * on aggregateExpressions, which could reference an expression in groupingExpressions.
- * For example, see the rule [[org.apache.spark.sql.catalyst.optimizer.SimplifyExtractValueOps]]
+ * For example, in the following query Spark shouldn't optimize the aggregate expression
+ * `Not(IsNull(c))` to `IsNotNull(c)` as the grouping expression is `IsNull(c)`:
+ * SELECT not(c IS NULL)
+ * FROM t
+ * GROUP BY c IS NULL
+ * Instead, the aggregate expression should contain `Not(GroupingExprRef(0))`.
  */
-case class Aggregate(
+case class AggregateWithGroupingReferences(

Review comment:
       > some of the rules need the "expanded" version of aggregate expressions (references are restored to original expressions)
   
   do you have some examples?




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

[GitHub] [spark] cloud-fan closed pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #31913:
URL: https://github.com/apache/spark/pull/31913


   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819607779


   > to trigger the bug, I think we need `if(not(a IS NULL), rand(), 1.0)`?
   
   Yes that triggers and is a better test. I will replace `a + b + rand()` to that soon in the next commit.


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611788588



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -655,12 +666,76 @@ case class Aggregate(
     }
   }
 
+  private def expandGroupingReferences(e: Expression): Expression = {
+    e match {
+      case _: AggregateExpression => e
+      case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+      case g: GroupingExprRef => groupingExpressions(g.ordinal)
+      case _ => e.mapChildren(expandGroupingReferences)
+    }
+  }
+
+  lazy val aggregateExpressionsWithoutGroupingRefs = {
+    aggregateExpressions.map(expandGroupingReferences(_).asInstanceOf[NamedExpression])
+  }
+
   override lazy val validConstraints: ExpressionSet = {
     val nonAgg = aggregateExpressions.filter(_.find(_.isInstanceOf[AggregateExpression]).isEmpty)
     getAllValidConstraints(nonAgg)
   }
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (ge.deterministic && !ge.foldable && ge.children.nonEmpty &&

Review comment:
       Indeed, `PullOutNondeterministic` is taking care of them.
   
   `GroupingExprRef` is not needed when it would refer to a simple `Attribute`. The purpose of `GroupingExprRef`s to wrap expressions that could change during optimization, I don't think that a simple attribute could change.
   Also, `GroupingExprRef` is not a `NamedExpression` now. If we wanted to allow `GroupingExprRef` to refer to a simple attribute in aggregate expressions then either it should implement `NamedExpression` or it should be wrapped into a new `Alias`.
   




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812687895


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41445/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804909791


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40984/
   


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821852990


   **[Test build #137509 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137509/testReport)** for PR 31913 at commit [`fb3a19d`](https://github.com/apache/spark/commit/fb3a19dad5ac4448b8fe9d1b12ed1c3f6a0369a7).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611788299



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -655,12 +666,76 @@ case class Aggregate(
     }
   }
 
+  private def expandGroupingReferences(e: Expression): Expression = {
+    e match {
+      case _: AggregateExpression => e
+      case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+      case g: GroupingExprRef => groupingExpressions(g.ordinal)
+      case _ => e.mapChildren(expandGroupingReferences)
+    }
+  }
+
+  lazy val aggregateExpressionsWithoutGroupingRefs = {
+    aggregateExpressions.map(expandGroupingReferences(_).asInstanceOf[NamedExpression])
+  }
+
   override lazy val validConstraints: ExpressionSet = {
     val nonAgg = aggregateExpressions.filter(_.find(_.isInstanceOf[AggregateExpression]).isEmpty)
     getAllValidConstraints(nonAgg)
   }
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (ge.deterministic && !ge.foldable && ge.children.nonEmpty &&
+            !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e

Review comment:
       No, I didn't want to limit possible optimizations inside `AggregateExpression`s. Eg. `first(not(t.id IS NULL))` => `first(isnotnull(id))` should happen.




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803636428


   **[Test build #136310 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136310/testReport)** for PR 31913 at commit [`5ab9f75`](https://github.com/apache/spark/commit/5ab9f75cfcd403d38fde6d0e39a319fc550c10f1).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805856791


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41038/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r600386244



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
##########
@@ -66,6 +66,34 @@ object RewriteNonCorrelatedExists extends Rule[LogicalPlan] {
   }
 }
 
+/**
+ * Wrap complex grouping expression in aggregate expressions without aggregate function into
+ * `GroupingExpression` nodes so as to avoid further optimizations between the expression and its
+ * parent.
+ *
+ * This is required as further optimizations could change the grouping expression and so make the
+ * aggregate expression invalid.

Review comment:
       Sure, added in https://github.com/apache/spark/pull/31913/commits/f46b89d6d81be4a7867fd3bc9edeccc1f7e9aea5

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
##########
@@ -66,6 +66,34 @@ object RewriteNonCorrelatedExists extends Rule[LogicalPlan] {
   }
 }
 
+/**
+ * Wrap complex grouping expression in aggregate expressions without aggregate function into

Review comment:
       Described in https://github.com/apache/spark/pull/31913/commits/f46b89d6d81be4a7867fd3bc9edeccc1f7e9aea5




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-809733789


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41250/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812750905


   **[Test build #136867 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136867/testReport)** for PR 31913 at commit [`fbf5a92`](https://github.com/apache/spark/commit/fbf5a9235db944e6c4e77a1a130a61d4b2f7f507).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `case class GroupingExprRef(`
     * `case class AggregateWithGroupingReferences(`


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813013052


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41471/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-809536671


   **[Test build #136668 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136668/testReport)** for PR 31913 at commit [`267b4a8`](https://github.com/apache/spark/commit/267b4a8fc1b5c4c16fe9c0e1ab95da31a4df3622).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812687880


   Kubernetes integration test status success
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41445/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819414124


   **[Test build #137336 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137336/testReport)** for PR 31913 at commit [`2e79eb9`](https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a).
    * This patch **fails Spark unit tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818284495


   **[Test build #137230 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137230/testReport)** for PR 31913 at commit [`343f35e`](https://github.com/apache/spark/commit/343f35eccf4db5f47dbf6a08eed18ab90a3e528b).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `class KoalasFrameMethods(object):`
     * `class KoalasSeriesMethods(object):`
     * `class IndexOpsMixin(object, metaclass=ABCMeta):`
     * `class CategoricalAccessor(object):`
     * `    however, expected types are [(<class 'float'>, <class 'int'>)].`
     * `class OptionError(AttributeError, KeyError):`
     * `class DatetimeMethods(object):`
     * `class DataError(Exception):`
     * `class SparkPandasIndexingError(Exception):`
     * `class SparkPandasNotImplementedError(NotImplementedError):`
     * `class PandasNotImplementedError(NotImplementedError):`
     * `            new_class = type(\"NameType\", (NameTypeHolder,), `
     * `            new_class = type(\"NameType\", (NameTypeHolder,), `
     * `class DataFrame(Frame, Generic[T]):`
     * `        [defaultdict(<class 'list'>, `
     * `defaultdict(<class 'list'>, `
     * `class CachedDataFrame(DataFrame):`
     * `class Frame(object, metaclass=ABCMeta):`
     * `class GroupBy(object, metaclass=ABCMeta):`
     * `class DataFrameGroupBy(GroupBy):`
     * `class SeriesGroupBy(GroupBy):`
     * `class Index(IndexOpsMixin):`
     * `class CategoricalIndex(Index):`
     * `class DatetimeIndex(Index):`
     * `class MultiIndex(Index):`
     * `            a single :class:`Index` (or subclass thereof).`
     * `class NumericIndex(Index):`
     * `class IntegerIndex(NumericIndex):`
     * `class Int64Index(IntegerIndex):`
     * `class Float64Index(NumericIndex):`
     * `class IndexerLike(object):`
     * `class AtIndexer(IndexerLike):`
     * `class iAtIndexer(IndexerLike):`
     * `class LocIndexerLike(IndexerLike, metaclass=ABCMeta):`
     * `class LocIndexer(LocIndexerLike):`
     * `class iLocIndexer(LocIndexerLike):`
     * `class InternalFrame(object):`
     * `class _MissingPandasLikeDataFrame(object):`
     * `class MissingPandasLikeDataFrameGroupBy(object):`
     * `class MissingPandasLikeSeriesGroupBy(object):`
     * `class MissingPandasLikeIndex(object):`
     * `class MissingPandasLikeDatetimeIndex(MissingPandasLikeIndex):`
     * `class MissingPandasLikeCategoricalIndex(MissingPandasLikeIndex):`
     * `class MissingPandasLikeMultiIndex(object):`
     * `class MissingPandasLikeSeries(object):`
     * `class MissingPandasLikeExpanding(object):`
     * `class MissingPandasLikeRolling(object):`
     * `class MissingPandasLikeExpandingGroupby(object):`
     * `class MissingPandasLikeRollingGroupby(object):`
     * `class PythonModelWrapper(object):`
     * `class KoalasPlotAccessor(PandasObject):`
     * `class KoalasBarPlot(PandasBarPlot, TopNPlotBase):`
     * `class KoalasBoxPlot(PandasBoxPlot, BoxPlotBase):`
     * `class KoalasHistPlot(PandasHistPlot, HistogramPlotBase):`
     * `class KoalasPiePlot(PandasPiePlot, TopNPlotBase):`
     * `class KoalasAreaPlot(PandasAreaPlot, SampledPlotBase):`
     * `class KoalasLinePlot(PandasLinePlot, SampledPlotBase):`
     * `class KoalasBarhPlot(PandasBarhPlot, TopNPlotBase):`
     * `class KoalasScatterPlot(PandasScatterPlot, TopNPlotBase):`
     * `class KoalasKdePlot(PandasKdePlot, KdePlotBase):`
     * `        new_class = type(\"NameType\", (NameTypeHolder,), `
     * `        new_class = param.type if isinstance(param, np.dtype) else param`
     * `class Series(Frame, IndexOpsMixin, Generic[T]):`
     * `        dictionary is a ``dict`` subclass that defines ``__missing__`` (i.e.`
     * `        defaultdict(<class 'list'>, `
     * `class SparkIndexOpsMethods(object, metaclass=ABCMeta):`
     * `class SparkSeriesMethods(SparkIndexOpsMethods):`
     * `class SparkIndexMethods(SparkIndexOpsMethods):`
     * `class SparkFrameMethods(object):`
     * `class CachedSparkFrameMethods(SparkFrameMethods):`
     * `class SQLProcessor(object):`
     * `class StringMethods(object):`
     * `class SeriesType(Generic[T]):`
     * `class DataFrameType(object):`
     * `class ScalarType(object):`
     * `class UnknownType(object):`
     * `class NameTypeHolder(object):`
     * `    The returned type class indicates both dtypes (a pandas only dtype object`
     * `class KoalasUsageLogger(object):`
     * `class RollingAndExpanding(object):`
     * `class Rolling(RollingAndExpanding):`
     * `class RollingGroupby(Rolling):`
     * `class Expanding(RollingAndExpanding):`
     * `class ExpandingGroupby(Expanding):`
     * `trait FunctionRegistryBase[T] `
     * `trait SimpleFunctionRegistryBase[T] extends FunctionRegistryBase[T] with Logging `
     * `trait EmptyFunctionRegistryBase[T] extends FunctionRegistryBase[T] `
     * `trait FunctionRegistry extends FunctionRegistryBase[Expression] `
     * `trait TableFunctionRegistry extends FunctionRegistryBase[LogicalPlan] `
     * `class NoSuchFunctionException(`
     * `case class ResolveTableValuedFunctions(catalog: SessionCatalog) extends Rule[LogicalPlan] `
     * `abstract class QuaternaryExpression extends Expression with QuaternaryLike[Expression] `
     * `abstract class Covariance(val left: Expression, val right: Expression, nullOnDivideByZero: Boolean)`
     * `case class KnownFloatingPointNormalized(child: Expression) extends TaggingExpression `
     * `trait BaseGroupingSets extends Expression with CodegenFallback `
     * `case class Cube(`
     * `trait SimpleHigherOrderFunction extends HigherOrderFunction with BinaryLike[Expression] `
     * `case class Acos(child: Expression) extends UnaryMathExpression(math.acos, \"ACOS\") `
     * `case class Asin(child: Expression) extends UnaryMathExpression(math.asin, \"ASIN\") `
     * `case class Atan(child: Expression) extends UnaryMathExpression(math.atan, \"ATAN\") `
     * `case class Cbrt(child: Expression) extends UnaryMathExpression(math.cbrt, \"CBRT\") `
     * `case class Cos(child: Expression) extends UnaryMathExpression(math.cos, \"COS\") `
     * `case class Cosh(child: Expression) extends UnaryMathExpression(math.cosh, \"COSH\") `
     * `case class Log10(child: Expression) extends UnaryLogExpression(StrictMath.log10, \"LOG10\") `
     * `case class Signum(child: Expression) extends UnaryMathExpression(math.signum, \"SIGNUM\") `
     * `case class Sin(child: Expression) extends UnaryMathExpression(math.sin, \"SIN\") `
     * `case class Sinh(child: Expression) extends UnaryMathExpression(math.sinh, \"SINH\") `
     * `case class Sqrt(child: Expression) extends UnaryMathExpression(math.sqrt, \"SQRT\") `
     * `case class Tan(child: Expression) extends UnaryMathExpression(math.tan, \"TAN\") `
     * `case class Tanh(child: Expression) extends UnaryMathExpression(math.tanh, \"TANH\") `
     * `case class DeleteAction(condition: Option[Expression]) extends MergeAction `
     * `case class UpdateStarAction(condition: Option[Expression]) extends MergeAction `
     * `case class InsertStarAction(condition: Option[Expression]) extends MergeAction `
     * `case class RefreshTable(child: LogicalPlan) extends UnaryCommand `
     * `case class CommentOnNamespace(child: LogicalPlan, comment: String) extends UnaryCommand `
     * `case class CommentOnTable(child: LogicalPlan, comment: String) extends UnaryCommand `
     * `case class RefreshFunction(child: LogicalPlan) extends UnaryCommand `
     * `case class DescribeFunction(child: LogicalPlan, isExtended: Boolean) extends UnaryCommand `
     * `case class RecoverPartitions(child: LogicalPlan) extends UnaryCommand `
     * `case class RuleId(id: Int) `
     * `abstract class TreeNode[BaseType <: TreeNode[BaseType]] extends Product with TreePatternBits `
     * `trait QuaternaryLike[T <: TreeNode[T]] `
     * `trait TreePatternBits `
     * `trait DataWritingCommand extends UnaryCommand `
     * `case class SetCommand(kv: Option[(String, Option[String])])`
     * `case class ResetCommand(config: Option[String]) extends LeafRunnableCommand with IgnoreCachedData `
     * `trait RunnableCommand extends Command `
     * `case class AddJarCommand(path: String) extends LeafRunnableCommand `
     * `case class AddFileCommand(path: String) extends LeafRunnableCommand `
     * `case class AddArchiveCommand(path: String) extends LeafRunnableCommand `
     * `case class ListFilesCommand(files: Seq[String] = Seq.empty[String]) extends LeafRunnableCommand `
     * `case class ListJarsCommand(jars: Seq[String] = Seq.empty[String]) extends LeafRunnableCommand `
     * `case class ListArchivesCommand(archives: Seq[String] = Seq.empty[String])`
     * `abstract class DescribeCommandBase extends LeafRunnableCommand `
     * `trait BaseCacheTableExec extends LeafV2CommandExec `
     * `sealed trait V1FallbackWriters extends LeafV2CommandExec with SupportsV1Write `
     * `case class LocalLimitExec(limit: Int, child: SparkPlan) extends BaseLimitExec `


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822294820


   Thanks @cloud-fan, @maropu, @dongjoon-hyun for the review.
   
   I will try look into the idea of https://github.com/apache/spark/pull/31913#discussion_r612560780 and maybe open a PR this week. 


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612558996



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e
+        case _ if PythonUDF.isGroupedAggPandasUDF(e) => e

Review comment:
       Can we add a util function in `object AggregateExpression` to check if an expression is an aggregate function? I've seen this pattern many times to check aggregate functions.




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812892996


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136882/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805772355


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136454/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-809571526


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136668/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820548004


   **[Test build #137416 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137416/testReport)** for PR 31913 at commit [`72c173b`](https://github.com/apache/spark/commit/72c173beebaf5ec63a9fd9360410b355462943d1).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

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

Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822868174


   late lgtm. Nice fix, @peter-toth ~


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803553957


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40888/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818032120


   Kubernetes integration test unable to build dist.
   
   exiting with code: 1
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41810/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820220531


   @cloud-fan, I think we need to add `EnforceGroupingReferencesInAggregates` and `UpdateGroupingExprRefNullability` to `Optimizer.nonExcludableRules`, don't we?


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820391094


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41992/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803899349


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40921/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803555086


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40888/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819307520


   **[Test build #137335 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137335/testReport)** for PR 31913 at commit [`2e79eb9`](https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612686569



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala
##########
@@ -96,7 +96,7 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
         .groupBy('a + 'b)(('a + 'b) as 'c)
         .analyze
       val optimized = Optimize.execute(query)
-      comparePlans(optimized, expected)
+      comparePlans(optimized, EnforceGroupingReferencesInAggregates(expected))

Review comment:
       `RemoveRedundantAggregates` explicitly calls `Aggregate.withGroupingRefs()`: https://github.com/apache/spark/pull/31913/files#diff-11264d807efa58054cca2d220aae8fba644ee0f0f2a4722c46d52828394846efR511. Actually this call wouldn't be necessary if a `GroupingExprRef` could replace an `Attribute`...




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804934176


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40984/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820572602


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137416/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812757806


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136867/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820393289


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41993/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822360631


   **[Test build #137581 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137581/testReport)** for PR 31913 at commit [`34f0439`](https://github.com/apache/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).
    * This patch passes all tests.
    * This patch **does not merge cleanly**.
    * This patch adds the following public classes _(experimental)_:
     * `public class VectorizedBLAS extends F2jBLAS `
     * `class PandasOnSparkFrameMethods(object):`
     * `class PandasOnSparkSeriesMethods(object):`
     * `class PandasOnSparkPlotAccessor(PandasObject):`
     * `class PandasOnSparkBarPlot(PandasBarPlot, TopNPlotBase):`
     * `class PandasOnSparkBoxPlot(PandasBoxPlot, BoxPlotBase):`
     * `class PandasOnSparkHistPlot(PandasHistPlot, HistogramPlotBase):`
     * `class PandasOnSparkPiePlot(PandasPiePlot, TopNPlotBase):`
     * `class PandasOnSparkAreaPlot(PandasAreaPlot, SampledPlotBase):`
     * `class PandasOnSparkLinePlot(PandasLinePlot, SampledPlotBase):`
     * `class PandasOnSparkBarhPlot(PandasBarhPlot, TopNPlotBase):`
     * `class PandasOnSparkScatterPlot(PandasScatterPlot, TopNPlotBase):`
     * `class PandasOnSparkKdePlot(PandasKdePlot, KdePlotBase):`
     * `class PandasOnSparkUsageLogger(object):`
     * `trait AnalysisOnlyCommand extends Command `
     * `  implicit class MetadataColumnHelper(attr: Attribute) `
     * `case class WriteToDataSourceV2(`
     * `case class WriteToMicroBatchDataSource(`


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803906495


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40921/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803871124


   > Thanks, @peter-toth . Could you update thee PR descritpion with the following?
   > 
   > ```
   > - then NOT isnull(id#222) is optimized to isnull(id#222) 
   > + then NOT isnull(id#222) is optimized to isnotnull(id#222)
   > ```
   
   Thanks @dongjoon-hyun, I updated the description.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812859908


   Kubernetes integration test unable to build dist.
   
   exiting with code: 1
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41458/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803597025


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40892/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805812129


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41038/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819696226


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41940/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805161974


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40993/
   


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612556980



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {

Review comment:
       It looks weird to use `foldLeft` with a mutable map. How about just creates the mutable map at the beginning and write a for loop? Then it's more readable to Java people.




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819497235


   **[Test build #137349 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137349/testReport)** for PR 31913 at commit [`cff9b9a`](https://github.com/apache/spark/commit/cff9b9ab289412529728749001ded9550f14e66e).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803590604


   **[Test build #136306 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136306/testReport)** for PR 31913 at commit [`ae1186f`](https://github.com/apache/spark/commit/ae1186f4be87b2136c3e55bf4ae3d41c58b03142).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `case class GroupingExpression(child: Expression) extends UnaryExpression `


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

[GitHub] [spark] maropu commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r598368983



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -870,8 +870,19 @@ object CollapseProject extends Rule[LogicalPlan] with AliasHelper {
       if (haveCommonNonDeterministicOutput(p.projectList, agg.aggregateExpressions)) {
         p
       } else {
-        agg.copy(aggregateExpressions = buildCleanedProjectList(
-          p.projectList, agg.aggregateExpressions))
+        val complexGroupingExpressions =
+          ExpressionSet(agg.groupingExpressions.filter(_.children.nonEmpty))
+
+        def wrapGroupingExpression(e: Expression): Expression = e match {
+          case _: AggregateExpression => e
+          case _ if complexGroupingExpressions.contains(e) => GroupingExpression(e)
+          case _ => e.mapChildren(wrapGroupingExpression)
+        }
+
+        val wrappedAggregateExpressions =
+          agg.aggregateExpressions.map(wrapGroupingExpression(_).asInstanceOf[NamedExpression])
+        agg.copy(aggregateExpressions =
+          buildCleanedProjectList(p.projectList, wrappedAggregateExpressions))

Review comment:
       I think this issue is not related to `CollapseProject`. For example, we can reproduce it like this;
   ```
   $ ./bin/spark-shell --conf spark.sql.optimizer.excludedRules=org.apache.spark.sql.catalyst.optimizer.CollapseProject
   
   Seq(Some(1), None).toDF("id").createOrReplaceTempView("t")
   val df = sql("""
     SELECT NOT(t.id IS NULL) AS X, count(*) AS c
     FROM t
     GROUP BY t.id IS NULL
   """)
   
   df.show()
   ```
   
   The query fails because `BooleanSimplification` applys illegal expr transformation to break the group-by constraint (that is, `group-by columns must appear in aggregate exprs`). In the ohter DBMS-like systems (e.g., PostgreSQL), the transformed query fails like this;
   ```
   postgres=# SELECT t.id IS NOT NULL AS X, count(*) AS c FROM t GROUP BY t.id IS NULL;
   ERROR:  column "t.id" must appear in the GROUP BY clause or be used in an aggregate function
   LINE 1: SELECT t.id IS NOT NULL AS X, count(*) AS c FROM t GROUP BY ...
   ```
   I'm currently not sure that this issue can happen only in `BooleanSimplification` though, I think we need a general solution to fix this kind of the illegal transformation instead of band-aid fixes.




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805161974


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40993/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607149811



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -622,31 +624,47 @@ case class Range(
 /**
  * This is a Group by operator with the aggregate functions and projections.
  *
- * @param groupingExpressions expressions for grouping keys
- * @param aggregateExpressions expressions for a project list, which could contain
- *                             [[AggregateExpression]]s.
+ * @param groupingExpressions Expressions for grouping keys.
+ * @param aggrExprWithGroupingRefs Expressions for a project list, which could contain
+ *                                 [[AggregateExpression]]s and [[GroupingExprRef]]s.
+ * @param child The child of the aggregate node.
+ * @param enforceGroupingReferences If [[aggrExprWithGroupingRefs]] should contain
+ *                                  [[GroupingExprRef]]s.
+ *
+ * [[aggrExprWithGroupingRefs]] without aggregate functions can contain [[GroupingExprRef]]
+ * expressions to refer to complex grouping expressions in [[groupingExpressions]]. These references
+ * ensure that optimization rules don't change the aggregate expressions to invalid ones that no
+ * longer refer to any grouping expressions and also simplify the expression transformations on the
+ * node (need to transform the expression only once).
  *
- * Note: Currently, aggregateExpressions is the project list of this Group by operator. Before
- * separating projection from grouping and aggregate, we should avoid expression-level optimization
- * on aggregateExpressions, which could reference an expression in groupingExpressions.
- * For example, see the rule [[org.apache.spark.sql.catalyst.optimizer.SimplifyExtractValueOps]]
+ * For example, in the following query Spark shouldn't optimize the aggregate expression
+ * `Not(IsNull(c))` to `IsNotNull(c)` as the grouping expression is `IsNull(c)`:
+ * SELECT not(c IS NULL)
+ * FROM t
+ * GROUP BY c IS NULL
+ * Instead, the aggregate expression should contain `Not(GroupingExprRef(0))`.
  */
-case class Aggregate(
+case class AggregateWithGroupingReferences(

Review comment:
       The reason for this rename is to keep the semantics of `Aggregate([grouping expressions], [aggregate expressions without grouping references], [child])` extractor, but redefining the extractor of a case class doesn't seem to work.




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-809733789


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41250/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r598382819



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -870,8 +870,19 @@ object CollapseProject extends Rule[LogicalPlan] with AliasHelper {
       if (haveCommonNonDeterministicOutput(p.projectList, agg.aggregateExpressions)) {
         p
       } else {
-        agg.copy(aggregateExpressions = buildCleanedProjectList(
-          p.projectList, agg.aggregateExpressions))
+        val complexGroupingExpressions =
+          ExpressionSet(agg.groupingExpressions.filter(_.children.nonEmpty))
+
+        def wrapGroupingExpression(e: Expression): Expression = e match {
+          case _: AggregateExpression => e
+          case _ if complexGroupingExpressions.contains(e) => GroupingExpression(e)
+          case _ => e.mapChildren(wrapGroupingExpression)
+        }
+
+        val wrappedAggregateExpressions =
+          agg.aggregateExpressions.map(wrapGroupingExpression(_).asInstanceOf[NamedExpression])
+        agg.copy(aggregateExpressions =
+          buildCleanedProjectList(p.projectList, wrappedAggregateExpressions))

Review comment:
       Ahh you are right, even the initial query can contain terms in which Spark should keep grouping expressions. I think I just inserted the `GroupingExpression` wrapper around `id IS NULL` at the wrong place, maybe it should happen during analysis. Will look into this today.




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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803546837


   **[Test build #136306 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136306/testReport)** for PR 31913 at commit [`ae1186f`](https://github.com/apache/spark/commit/ae1186f4be87b2136c3e55bf4ae3d41c58b03142).


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812687895


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41445/
   


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

[GitHub] [spark] maropu commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r599567993



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/grouping.scala
##########
@@ -155,3 +155,21 @@ object GroupingID {
     if (SQLConf.get.integerGroupingIdEnabled) IntegerType else LongType
   }
 }
+
+/**
+ * Wrapper expression to avoid further optimizations between the parent and child of this
+ * expression.
+ */
+case class GroupingExpression(child: Expression) extends UnaryExpression {

Review comment:
       This is a tagging-like expr (`TaggingExpression`) ?




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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825380767


   I think pulling out grouping expressions into a project is a good idea. By-name matching is more robust than by-ordinal matching. It's a pity that I didn't come up with this idea while reviewing this PR. @peter-toth what do you think about the new idea?


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611684476



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -655,12 +666,76 @@ case class Aggregate(
     }
   }
 
+  private def expandGroupingReferences(e: Expression): Expression = {
+    e match {
+      case _: AggregateExpression => e
+      case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+      case g: GroupingExprRef => groupingExpressions(g.ordinal)
+      case _ => e.mapChildren(expandGroupingReferences)
+    }
+  }
+
+  lazy val aggregateExpressionsWithoutGroupingRefs = {
+    aggregateExpressions.map(expandGroupingReferences(_).asInstanceOf[NamedExpression])
+  }
+
   override lazy val validConstraints: ExpressionSet = {
     val nonAgg = aggregateExpressions.filter(_.find(_.isInstanceOf[AggregateExpression]).isEmpty)
     getAllValidConstraints(nonAgg)
   }
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (ge.deterministic && !ge.foldable && ge.children.nonEmpty &&

Review comment:
       IIRC the analyzer enforces that `groupingExpressions` are deterministic?
   
   And why do we exclude `Attribute` to be `GroupingExprRef`?




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821852990


   **[Test build #137509 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137509/testReport)** for PR 31913 at commit [`fb3a19d`](https://github.com/apache/spark/commit/fb3a19dad5ac4448b8fe9d1b12ed1c3f6a0369a7).


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813006354


   **[Test build #136894 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136894/testReport)** for PR 31913 at commit [`86047df`](https://github.com/apache/spark/commit/86047df797dbfb157a24ec71a285bea99db4f600).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819536053


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41928/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612683167



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e
+        case _ if PythonUDF.isGroupedAggPandasUDF(e) => e

Review comment:
       Added in https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a.




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

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

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821848912


   **[Test build #758850368](https://github.com/peter-toth/spark/actions/runs/758850368)** for PR 31913 at commit [`fb3a19d`](https://github.com/peter-toth/spark/commit/fb3a19dad5ac4448b8fe9d1b12ed1c3f6a0369a7).


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822229253






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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822377537






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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805053500


   **[Test build #136400 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136400/testReport)** for PR 31913 at commit [`04e61c5`](https://github.com/apache/spark/commit/04e61c5c3a41dac3fd51eaac66e4e561b3d403b3).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds the following public classes _(experimental)_:
     * `case class WriteToStream(`
     * `case class WriteToStreamStatement(`


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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819307071


   retest this please


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

[GitHub] [spark] maropu commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r600518216



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/constraintExpressions.scala
##########
@@ -39,3 +39,9 @@ case class KnownNotNull(child: Expression) extends TaggingExpression {
 }
 
 case class KnownFloatingPointNormalized(child: Expression) extends TaggingExpression
+
+/**
+ * Wrapper expression to avoid further optimizations between the parent and child of this
+ * expression.
+ */
+case class GroupingExpression(child: Expression) extends TaggingExpression

Review comment:
       nit: How about `GroupingExpression` -> `ReferencedByGroupingExpr`?




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803596183


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40892/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803598652






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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804780675


   **[Test build #136400 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136400/testReport)** for PR 31913 at commit [`04e61c5`](https://github.com/apache/spark/commit/04e61c5c3a41dac3fd51eaac66e4e561b3d403b3).


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

[GitHub] [spark] sigmod edited a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
sigmod edited a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825318373


   Posted a message on the git commit page and copy it here.
   
   Hi @peter-toth
   
   @cloud-fan @maryannxue @rednaxelafx and I had an offline discussion about PR.
   
   Here's a summary:
   
   (1) This PR would bring complexities when we add more optimization around grouping columns down the road. E.g., if we want to reorder grouping columns in order to reuse partitioning properties or optimize away certain grouping columns, we would need to re-generate the ordinals in grouping refs.
   (2) It seems safe to pull out non-attribute grouping expressions from an Aggregate and put them in a Project below the Aggregate. Then, grouping expressions in the Aggregate are only AttributeReferences. The rest of Optimizer rules can still match the Aggregate as before. We do need to pay attention to rules that collapse Aggregate and Project. In the planner, we can combine Aggregate and Project, so that the physical plan doesn't change.
   
   Thus, would you mind reverting this PR and try the proposal in (2)? Thanks in advance!
   
   Best,
   Yingyi


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818032842


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41810/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803906526






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

[GitHub] [spark] maropu commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
maropu commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r600167101



##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -4140,6 +4140,20 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
       }
     }
   }
+
+  test("SPARK-34581: Don't optimize out grouping expressions from aggregate expressions") {
+    withTempView("t") {
+      Seq[Integer](null, 1, 2, 3, null).toDF("id").createOrReplaceTempView("t")
+
+      val df = spark.sql(

Review comment:
       If this is a test to check if a query work correctly or not, could you move this test into `SQLQueryTestSuite`?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
##########
@@ -66,6 +66,34 @@ object RewriteNonCorrelatedExists extends Rule[LogicalPlan] {
   }
 }
 
+/**
+ * Wrap complex grouping expression in aggregate expressions without aggregate function into

Review comment:
       What does "complex" means in this context?

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/finishAnalysis.scala
##########
@@ -66,6 +66,34 @@ object RewriteNonCorrelatedExists extends Rule[LogicalPlan] {
   }
 }
 
+/**
+ * Wrap complex grouping expression in aggregate expressions without aggregate function into
+ * `GroupingExpression` nodes so as to avoid further optimizations between the expression and its
+ * parent.
+ *
+ * This is required as further optimizations could change the grouping expression and so make the
+ * aggregate expression invalid.

Review comment:
       Could you add a simple query example to explain about the case above?




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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r598506466



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -870,8 +870,19 @@ object CollapseProject extends Rule[LogicalPlan] with AliasHelper {
       if (haveCommonNonDeterministicOutput(p.projectList, agg.aggregateExpressions)) {
         p
       } else {
-        agg.copy(aggregateExpressions = buildCleanedProjectList(
-          p.projectList, agg.aggregateExpressions))
+        val complexGroupingExpressions =
+          ExpressionSet(agg.groupingExpressions.filter(_.children.nonEmpty))
+
+        def wrapGroupingExpression(e: Expression): Expression = e match {
+          case _: AggregateExpression => e
+          case _ if complexGroupingExpressions.contains(e) => GroupingExpression(e)
+          case _ => e.mapChildren(wrapGroupingExpression)
+        }
+
+        val wrappedAggregateExpressions =
+          agg.aggregateExpressions.map(wrapGroupingExpression(_).asInstanceOf[NamedExpression])
+        agg.copy(aggregateExpressions =
+          buildCleanedProjectList(p.projectList, wrappedAggregateExpressions))

Review comment:
       Thanks for the test case @maropu.
   I moved the logic to a new rule and simplified the test case in: https://github.com/apache/spark/pull/31913/commits/2293fd40449b9b540de9aa6614b6ca10698b7433




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803552711


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40888/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822166597


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42127/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804859400


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40984/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819865911


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137361/
   


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r613033278



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -784,6 +795,19 @@ case class Aggregate(
     }
   }
 
+  private def expandGroupingReferences(e: Expression): Expression = {
+    e match {
+      case _: AggregateExpression => e
+      case _ if PythonUDF.isGroupedAggPandasUDF(e) => e

Review comment:
       nit: now we can write `case _ if AggregateExpression.isAggregtate(e) => e`




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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804780675


   **[Test build #136400 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136400/testReport)** for PR 31913 at commit [`04e61c5`](https://github.com/apache/spark/commit/04e61c5c3a41dac3fd51eaac66e4e561b3d403b3).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-806797791


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


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

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

Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820361489


   **[Test build #751890144](https://github.com/peter-toth/spark/actions/runs/751890144)** for PR 31913 at commit [`34f0439`](https://github.com/peter-toth/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).


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

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

Posted by GitBox <gi...@apache.org>.
sigmod commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825455970


   >> Probably the new rule should be similar to PullOutNondeterministic. 
   
   Yeah, exactly!
   
   >> Do you have a PR ready or shall I open a new one?
   
   We don't have one. So please open a new one.  Thanks a lot, Peter!


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819536012






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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607159190



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -622,31 +624,47 @@ case class Range(
 /**
  * This is a Group by operator with the aggregate functions and projections.
  *
- * @param groupingExpressions expressions for grouping keys
- * @param aggregateExpressions expressions for a project list, which could contain
- *                             [[AggregateExpression]]s.
+ * @param groupingExpressions Expressions for grouping keys.
+ * @param aggrExprWithGroupingRefs Expressions for a project list, which could contain
+ *                                 [[AggregateExpression]]s and [[GroupingExprRef]]s.
+ * @param child The child of the aggregate node.
+ * @param enforceGroupingReferences If [[aggrExprWithGroupingRefs]] should contain
+ *                                  [[GroupingExprRef]]s.
+ *
+ * [[aggrExprWithGroupingRefs]] without aggregate functions can contain [[GroupingExprRef]]
+ * expressions to refer to complex grouping expressions in [[groupingExpressions]]. These references
+ * ensure that optimization rules don't change the aggregate expressions to invalid ones that no
+ * longer refer to any grouping expressions and also simplify the expression transformations on the
+ * node (need to transform the expression only once).
  *
- * Note: Currently, aggregateExpressions is the project list of this Group by operator. Before
- * separating projection from grouping and aggregate, we should avoid expression-level optimization
- * on aggregateExpressions, which could reference an expression in groupingExpressions.
- * For example, see the rule [[org.apache.spark.sql.catalyst.optimizer.SimplifyExtractValueOps]]
+ * For example, in the following query Spark shouldn't optimize the aggregate expression
+ * `Not(IsNull(c))` to `IsNotNull(c)` as the grouping expression is `IsNull(c)`:
+ * SELECT not(c IS NULL)
+ * FROM t
+ * GROUP BY c IS NULL
+ * Instead, the aggregate expression should contain `Not(GroupingExprRef(0))`.
  */
-case class Aggregate(
+case class AggregateWithGroupingReferences(

Review comment:
       This change is much bigger than I expect. What's the reason here? I was expecting something like
   1. A rule that runs at the beginning of optimizer, to create the grouping refs.
   2. Restore grouping refs to the original expressions when converting logical aggregate to phyiscal versions.




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818285463


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137230/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819361845


   **[Test build #137335 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137335/testReport)** for PR 31913 at commit [`2e79eb9`](https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a).
    * This patch **fails Spark unit tests**.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819340770


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41915/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813062833






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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821860619


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42083/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821888875


   **[Test build #137509 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137509/testReport)** for PR 31913 at commit [`fb3a19d`](https://github.com/apache/spark/commit/fb3a19dad5ac4448b8fe9d1b12ed1c3f6a0369a7).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820257071


   @peter-toth you are right. We should do so.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819623780


   **[Test build #137361 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137361/testReport)** for PR 31913 at commit [`78296a8`](https://github.com/apache/spark/commit/78296a86585e9f0cad5c27fa534aa614a710793e).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812861549


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41458/
   


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820360645


   **[Test build #137416 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137416/testReport)** for PR 31913 at commit [`72c173b`](https://github.com/apache/spark/commit/72c173beebaf5ec63a9fd9360410b355462943d1).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819309758


   **[Test build #137336 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137336/testReport)** for PR 31913 at commit [`2e79eb9`](https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r607147955



##########
File path: sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/complexTypesSuite.scala
##########
@@ -405,14 +405,6 @@ class ComplexTypesSuite extends PlanTest with ExpressionEvalHelper {
     val arrayAggRel = relation.groupBy(
       CreateArray(Seq('nullable_id)))(GetArrayItem(CreateArray(Seq('nullable_id)), 0))
     checkRule(arrayAggRel, arrayAggRel)
-
-    // This could be done if we had a more complex rule that checks that

Review comment:
       This can be removed now. It is optimized to:
   ```
   Aggregate [*id#0L], [CASE WHEN (0 = *id#0L) THEN (*id#0L + 1) END AS a#0L]
   +- LocalRelation <empty>, [*id#0L, nullable_id#0L]
   ```




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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611685232



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -655,12 +666,76 @@ case class Aggregate(
     }
   }
 
+  private def expandGroupingReferences(e: Expression): Expression = {
+    e match {
+      case _: AggregateExpression => e
+      case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+      case g: GroupingExprRef => groupingExpressions(g.ordinal)
+      case _ => e.mapChildren(expandGroupingReferences)
+    }
+  }
+
+  lazy val aggregateExpressionsWithoutGroupingRefs = {
+    aggregateExpressions.map(expandGroupingReferences(_).asInstanceOf[NamedExpression])
+  }
+
   override lazy val validConstraints: ExpressionSet = {
     val nonAgg = aggregateExpressions.filter(_.find(_.isInstanceOf[AggregateExpression]).isEmpty)
     getAllValidConstraints(nonAgg)
   }
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (ge.deterministic && !ge.foldable && ge.children.nonEmpty &&
+            !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e

Review comment:
       Can aggregate functions take grouping expressions as input?




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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813057385


   **[Test build #136896 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136896/testReport)** for PR 31913 at commit [`977c0bf`](https://github.com/apache/spark/commit/977c0bff247987eec7efd68bea8b33236b0c1612).


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820362521


   **[Test build #137417 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137417/testReport)** for PR 31913 at commit [`34f0439`](https://github.com/apache/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812678887


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41445/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819452486


   Hmm, I need to look into the latest test failures...


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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818836518


   looks good in general, thanks for cleaning this up!


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817282271






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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822229207






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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819623780


   **[Test build #137361 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137361/testReport)** for PR 31913 at commit [`78296a8`](https://github.com/apache/spark/commit/78296a86585e9f0cad5c27fa534aa614a710793e).


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803586691


   **[Test build #136310 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136310/testReport)** for PR 31913 at commit [`5ab9f75`](https://github.com/apache/spark/commit/5ab9f75cfcd403d38fde6d0e39a319fc550c10f1).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805076974


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136400/
   


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

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

Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803697422


   Anyway, nice catch, @peter-toth !


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612682998



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {

Review comment:
       Fixed in https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a.




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803642405


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136310/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r598506466



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -870,8 +870,19 @@ object CollapseProject extends Rule[LogicalPlan] with AliasHelper {
       if (haveCommonNonDeterministicOutput(p.projectList, agg.aggregateExpressions)) {
         p
       } else {
-        agg.copy(aggregateExpressions = buildCleanedProjectList(
-          p.projectList, agg.aggregateExpressions))
+        val complexGroupingExpressions =
+          ExpressionSet(agg.groupingExpressions.filter(_.children.nonEmpty))
+
+        def wrapGroupingExpression(e: Expression): Expression = e match {
+          case _: AggregateExpression => e
+          case _ if complexGroupingExpressions.contains(e) => GroupingExpression(e)
+          case _ => e.mapChildren(wrapGroupingExpression)
+        }
+
+        val wrappedAggregateExpressions =
+          agg.aggregateExpressions.map(wrapGroupingExpression(_).asInstanceOf[NamedExpression])
+        agg.copy(aggregateExpressions =
+          buildCleanedProjectList(p.projectList, wrappedAggregateExpressions))

Review comment:
       Thanks for the test case @maropu.
   I moved the logic to a new rule and simplified the test case in: https://github.com/apache/spark/pull/31913/commits/2293fd40449b9b540de9aa6614b6ca10698b7433 and updated the PR description.




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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-806009269


   > Shall we introduce something like `case class GroupingExprRef(ordinal: Int)` and replace all the grouping expressions with `GroupingExprRef` in `Aggregate.aggregateExpressions` during analysis? Then we don't need to worry about inconsistency anymore.
   
   I can give it a try.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812855049


   **[Test build #136882 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136882/testReport)** for PR 31913 at commit [`de95a9a`](https://github.com/apache/spark/commit/de95a9aa13e5c763b4d8f7e87e1f1fe99dea466a).


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803859941


   **[Test build #136337 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136337/testReport)** for PR 31913 at commit [`3de19ca`](https://github.com/apache/spark/commit/3de19cae711790a4c520221da1965277011f6f65).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803546837


   **[Test build #136306 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136306/testReport)** for PR 31913 at commit [`ae1186f`](https://github.com/apache/spark/commit/ae1186f4be87b2136c3e55bf4ae3d41c58b03142).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803548910


   The `Not(IsNull(e)) => IsNotNull(e)` and `Not(IsNotNull(e)) => IsNull(e)` optimizations were introduced in https://github.com/apache/spark/pull/25878 so the example query is a regression from 2.4 to 3.x, but I'm not sure that the issue doesn't come up in Spark 2.4 at all.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822165742


   **[Test build #137561 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137561/testReport)** for PR 31913 at commit [`fb3a19d`](https://github.com/apache/spark/commit/fb3a19dad5ac4448b8fe9d1b12ed1c3f6a0369a7).


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-829204512


   I've opened a new PR here: https://github.com/apache/spark/pull/32396 with the new approach.
   I didn't update `PhysicalAggregation` to collapse Project under Aggregate as is doesn't seem necessary.


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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820543382


   cc @hvanhovell as well


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r598382819



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -870,8 +870,19 @@ object CollapseProject extends Rule[LogicalPlan] with AliasHelper {
       if (haveCommonNonDeterministicOutput(p.projectList, agg.aggregateExpressions)) {
         p
       } else {
-        agg.copy(aggregateExpressions = buildCleanedProjectList(
-          p.projectList, agg.aggregateExpressions))
+        val complexGroupingExpressions =
+          ExpressionSet(agg.groupingExpressions.filter(_.children.nonEmpty))
+
+        def wrapGroupingExpression(e: Expression): Expression = e match {
+          case _: AggregateExpression => e
+          case _ if complexGroupingExpressions.contains(e) => GroupingExpression(e)
+          case _ => e.mapChildren(wrapGroupingExpression)
+        }
+
+        val wrappedAggregateExpressions =
+          agg.aggregateExpressions.map(wrapGroupingExpression(_).asInstanceOf[NamedExpression])
+        agg.copy(aggregateExpressions =
+          buildCleanedProjectList(p.projectList, wrappedAggregateExpressions))

Review comment:
       Ahh you are right, even the initial query can contain such a term in which Spark shoud keep the grouping expression. I think I just placed the 'GroupingExpression' wrapper around 'id IS NULL' at the wrong place, maybe it should happen during analysis. Will look into it...




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819865911


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137361/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805741698


   **[Test build #136454 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136454/testReport)** for PR 31913 at commit [`f46b89d`](https://github.com/apache/spark/commit/f46b89d6d81be4a7867fd3bc9edeccc1f7e9aea5).


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819307520


   **[Test build #137335 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137335/testReport)** for PR 31913 at commit [`2e79eb9`](https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a).


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812757806


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136867/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820541992


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137417/
   


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

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

Posted by GitBox <gi...@apache.org>.
maropu commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805858497


   cc: @cloud-fan @viirya 


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612688038



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e
+        case _: AggregateExpression => e
+        case _ if PythonUDF.isGroupedAggPandasUDF(e) => e
+        case _ if groupingExpressions.contains(e.canonicalized) =>
+          val (groupingExpression, ordinal) = groupingExpressions(e.canonicalized)
+          GroupingExprRef(ordinal, groupingExpression.dataType, groupingExpression.nullable)
+        case _ => e.mapChildren(insertGroupingExprRefs)
+      }
+    }
+
+    aggregateExpressions.map(insertGroupingExprRefs(_).asInstanceOf[NamedExpression])
+  }
+
+  def withGroupingRefs(
+      groupingExpressions: Seq[Expression],
+      aggregateExpressions: Seq[NamedExpression],
+      child: LogicalPlan): Aggregate = {
+    val dealiasedGroupingExpressions = groupingExpressions.map {
+      case a: Alias => a.child
+      case o => o
+    }
+    val complexGroupingExpressions =
+      collectComplexGroupingExpressions(dealiasedGroupingExpressions)
+    val aggrExprWithGroupingReferences = if (complexGroupingExpressions.nonEmpty) {
+      insertGroupingReferences(aggregateExpressions, complexGroupingExpressions)

Review comment:
       I also see the pros of making this framework more general, and I'm happy to do it, but it is not required for this bugfix. How about doing it in a follow-up ticket?




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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803598651






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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822152807


   **[Test build #137552 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137552/testReport)** for PR 31913 at commit [`34f0439`](https://github.com/apache/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).


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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812855049


   **[Test build #136882 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136882/testReport)** for PR 31913 at commit [`de95a9a`](https://github.com/apache/spark/commit/de95a9aa13e5c763b4d8f7e87e1f1fe99dea466a).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821892472


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137509/
   


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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-825470374


   I think the new idea should also be simpler:
   1. `CollapseProject` only collpase Project + Aggregate, but not Aggregate + Project. No change is needed. For example:
   ```
   scala> spark.range(10).select('id + 1 as 'a).groupBy('a).count().explain(true)
   == Parsed Logical Plan ==
   'Aggregate ['a], ['a, count(1) AS count#17L]
   +- Project [(id#11L + cast(1 as bigint)) AS a#13L]
      +- Range (0, 10, step=1, splits=Some(1))
   
   == Analyzed Logical Plan ==
   a: bigint, count: bigint
   Aggregate [a#13L], [a#13L, count(1) AS count#17L]
   +- Project [(id#11L + cast(1 as bigint)) AS a#13L]
      +- Range (0, 10, step=1, splits=Some(1))
   
   == Optimized Logical Plan ==
   Aggregate [a#13L], [a#13L, count(1) AS count#17L]
   +- Project [(id#11L + 1) AS a#13L]
      +- Range (0, 10, step=1, splits=Some(1))
   
   == Physical Plan ==
   AdaptiveSparkPlan isFinalPlan=false
   +- HashAggregate(keys=[a#13L], functions=[count(1)], output=[a#13L, count#17L])
      +- HashAggregate(keys=[a#13L], functions=[partial_count(1)], output=[a#13L, count#21L])
         +- Project [(id#11L + 1) AS a#13L]
            +- Range (0, 10, step=1, splits=1)
   ```
   2. `PhysicalAggregation` needs update to collapse Project above Aggregate.
   
   Anyway, I'm reverting this PR, @peter-toth look forward to your new PR!


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

[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

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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817994781


   **[Test build #137230 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137230/testReport)** for PR 31913 at commit [`343f35e`](https://github.com/apache/spark/commit/343f35eccf4db5f47dbf6a08eed18ab90a3e528b).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-818954867


   Kubernetes integration test unable to build dist.
   
   exiting with code: 141
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41880/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805149736


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/40993/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819381256


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137335/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-821858491






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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r598382819



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/Optimizer.scala
##########
@@ -870,8 +870,19 @@ object CollapseProject extends Rule[LogicalPlan] with AliasHelper {
       if (haveCommonNonDeterministicOutput(p.projectList, agg.aggregateExpressions)) {
         p
       } else {
-        agg.copy(aggregateExpressions = buildCleanedProjectList(
-          p.projectList, agg.aggregateExpressions))
+        val complexGroupingExpressions =
+          ExpressionSet(agg.groupingExpressions.filter(_.children.nonEmpty))
+
+        def wrapGroupingExpression(e: Expression): Expression = e match {
+          case _: AggregateExpression => e
+          case _ if complexGroupingExpressions.contains(e) => GroupingExpression(e)
+          case _ => e.mapChildren(wrapGroupingExpression)
+        }
+
+        val wrappedAggregateExpressions =
+          agg.aggregateExpressions.map(wrapGroupingExpression(_).asInstanceOf[NamedExpression])
+        agg.copy(aggregateExpressions =
+          buildCleanedProjectList(p.projectList, wrappedAggregateExpressions))

Review comment:
       Ahh you are right, even the initial query can contain terms in which Spark should keep grouping expressions. I think I just inserted the `GroupingExpression` wrapper around `id IS NULL` at the wrong place, it should happen during analysis. Will look into this today.




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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r599579148



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/grouping.scala
##########
@@ -155,3 +155,21 @@ object GroupingID {
     if (SQLConf.get.integerGroupingIdEnabled) IntegerType else LongType
   }
 }
+
+/**
+ * Wrapper expression to avoid further optimizations between the parent and child of this
+ * expression.
+ */
+case class GroupingExpression(child: Expression) extends UnaryExpression {

Review comment:
       Thanks, I think it is. Changed in https://github.com/apache/spark/pull/31913/commits/6e05f14c1a21e2894c1a4a85546a790189a43f5a




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805772355


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136454/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822361649


   **[Test build #137561 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137561/testReport)** for PR 31913 at commit [`fb3a19d`](https://github.com/apache/spark/commit/fb3a19dad5ac4448b8fe9d1b12ed1c3f6a0369a7).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817311554


   **[Test build #137178 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137178/testReport)** for PR 31913 at commit [`c2ba804`](https://github.com/apache/spark/commit/c2ba80457bd86d11ad26311bbc3c42607f33b19a).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813064991


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41473/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820541992


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137417/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-804937339


   **[Test build #136410 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136410/testReport)** for PR 31913 at commit [`6e05f14`](https://github.com/apache/spark/commit/6e05f14c1a21e2894c1a4a85546a790189a43f5a).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820572602


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137416/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820362521


   **[Test build #137417 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137417/testReport)** for PR 31913 at commit [`34f0439`](https://github.com/apache/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813098284


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136896/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819496368


   > Hmm, I need to look into the latest test failures...
   
   Should be ok after https://github.com/apache/spark/pull/31913/commits/cff9b9ab289412529728749001ded9550f14e66e.


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822220836


   **[Test build #137581 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137581/testReport)** for PR 31913 at commit [`34f0439`](https://github.com/apache/spark/commit/34f0439892650c30b6ae4e670f3771c350868ff5).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812664868


   **[Test build #136867 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136867/testReport)** for PR 31913 at commit [`fbf5a92`](https://github.com/apache/spark/commit/fbf5a9235db944e6c4e77a1a130a61d4b2f7f507).


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-813047036


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136894/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819745010


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137349/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-820393236






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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805856791


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41038/
   


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

[GitHub] [spark] AmplabJenkins removed a comment on pull request #31913: [WIP][SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-812861549


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41458/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r612683378



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -793,6 +817,56 @@ case class Aggregate(
     copy(child = newChild)
 }
 
+object Aggregate {
+  private def collectComplexGroupingExpressions(groupingExpressions: Seq[Expression]) = {
+    groupingExpressions.zipWithIndex
+      .foldLeft(mutable.Map.empty[Expression, (Expression, Int)]) {
+        case (m, (ge, i)) =>
+          if (!ge.foldable && ge.children.nonEmpty && !m.contains(ge.canonicalized)) {
+            m += ge.canonicalized -> (ge, i)
+          }
+          m
+      }
+  }
+
+  private def insertGroupingReferences(
+      aggregateExpressions: Seq[NamedExpression],
+      groupingExpressions: collection.Map[Expression, (Expression, Int)]): Seq[NamedExpression] = {
+    def insertGroupingExprRefs(e: Expression): Expression = {
+      e match {
+        case _ if !e.deterministic => e

Review comment:
       Good catch, fixed in https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a.




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

[GitHub] [spark] SparkQA removed a comment on pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-819309758


   **[Test build #137336 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/137336/testReport)** for PR 31913 at commit [`2e79eb9`](https://github.com/apache/spark/commit/2e79eb910f5d4ef60227f7352e86dbed2786cb7a).


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803859941


   **[Test build #136337 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136337/testReport)** for PR 31913 at commit [`3de19ca`](https://github.com/apache/spark/commit/3de19cae711790a4c520221da1965277011f6f65).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817284155


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/41756/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803586691


   **[Test build #136310 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/136310/testReport)** for PR 31913 at commit [`5ab9f75`](https://github.com/apache/spark/commit/5ab9f75cfcd403d38fde6d0e39a319fc550c10f1).


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803642405


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/136310/
   


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611680524



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala
##########
@@ -795,7 +795,7 @@ object NullPropagation extends Rule[LogicalPlan] {
  */
 object FoldablePropagation extends Rule[LogicalPlan] {
   def apply(plan: LogicalPlan): LogicalPlan = {
-    CleanupAliases(propagateFoldables(plan)._1)
+    EnforceGroupingReferencesInAggregates(CleanupAliases(propagateFoldables(plan)._1))

Review comment:
       why is this change needed?




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-822229253


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/42133/
   


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r600386630



##########
File path: sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
##########
@@ -4140,6 +4140,20 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
       }
     }
   }
+
+  test("SPARK-34581: Don't optimize out grouping expressions from aggregate expressions") {
+    withTempView("t") {
+      Seq[Integer](null, 1, 2, 3, null).toDF("id").createOrReplaceTempView("t")
+
+      val df = spark.sql(

Review comment:
       It is, moved in https://github.com/apache/spark/pull/31913/commits/09f1a85d8312e56c886b31f26ae87d245288d2b9




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

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

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805896134


   Ah this problem is tricky. I think it's error-prone to duplicate the grouping expressions in `Aggregate.groupingExpressions` and `Aggregate.aggregateExpressions`, and assume all the rules will transform them in the same way to keep them the same.
   
   Shall we introduce something like `case class GroupingExprRef(ordinal: Int)` and replace all the grouping expressions with `GroupingExprRef` in `Aggregate.aggregateExpressions` during analysis? Then we don't need to worry about inconsistency anymore.


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

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

Posted by GitBox <gi...@apache.org>.
peter-toth commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r613221340



##########
File path: sql/core/src/test/resources/sql-tests/inputs/group-by.sql
##########
@@ -179,3 +179,8 @@ SELECT count(*) FROM test_agg WHERE k = 1 or k = 2 or count(*) + 1L > 1L or max(
 
 -- Aggregate with multiple distinct decimal columns
 SELECT AVG(DISTINCT decimal_col), SUM(DISTINCT decimal_col) FROM VALUES (CAST(1 AS DECIMAL(9, 0))) t(decimal_col);
+
+-- SPARK-34581: Don't optimize out grouping expressions from aggregate expressions without aggregate function
+SELECT not(a IS NULL), count(*) AS c
+FROM testData
+GROUP BY a IS NULL

Review comment:
       Added in https://github.com/apache/spark/pull/31913/commits/cff9b9ab289412529728749001ded9550f14e66e.

##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
##########
@@ -784,6 +795,19 @@ case class Aggregate(
     }
   }
 
+  private def expandGroupingReferences(e: Expression): Expression = {
+    e match {
+      case _: AggregateExpression => e
+      case _ if PythonUDF.isGroupedAggPandasUDF(e) => e

Review comment:
       Fixed in https://github.com/apache/spark/pull/31913/commits/cff9b9ab289412529728749001ded9550f14e66e




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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-817316507


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/137178/
   


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

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

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-803906526


   
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/40921/
   


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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-809732967


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41250/
   


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

[GitHub] [spark] cloud-fan commented on a change in pull request #31913: [SPARK-34581][SQL] Don't optimize out grouping expressions from aggregate expressions without aggregate function

Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #31913:
URL: https://github.com/apache/spark/pull/31913#discussion_r611675199



##########
File path: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/grouping.scala
##########
@@ -212,3 +212,22 @@ object GroupingID {
     if (SQLConf.get.integerGroupingIdEnabled) IntegerType else LongType
   }
 }
+
+/**
+ * A reference to an grouping expression in [[Aggregate]] node.
+ *
+ * @param ordinal The ordinal of the grouping expression in [[Aggregate]] that this expression
+ *                refers to.
+ * @param dataType The [[DataType]] of the referenced grouping expression.
+ * @param nullable True if null is a valid value for the referenced grouping expression.
+ */
+case class GroupingExprRef(
+    ordinal: Int,
+    dataType: DataType,
+    nullable: Boolean)
+  extends LeafExpression with Unevaluable {
+
+  override def stringArgs: Iterator[Any] = {
+    Iterator(ordinal)

Review comment:
       nit: shall we include data type and nullability for debug-ability?




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

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

Posted by GitBox <gi...@apache.org>.
SparkQA commented on pull request #31913:
URL: https://github.com/apache/spark/pull/31913#issuecomment-805851067


   Kubernetes integration test status failure
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/41038/
   


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