[SPARK-44846][SQL] Pull out complex grouping expressions after remove redundant aggregates #42531

zml1206 · 2023-08-17T09:58:32Z

What changes were proposed in this pull request?

This PR makes the aggregate do not contains complex grouping expressions after RemoveRedundantAggregates. In rule RemoveRedundantAggregates, if trigger remove redundant aggregates, then do PullOutGroupingExpressions to pull out complex grouping expressions to a Project node under an Aggregate.

Why are the changes needed?

The aggregate contains complex grouping expressions after RemoveRedundantAggregates, if aggregateExpressions has (if / case) branches, it is possible that groupingExpressions is no longer a subexpression of aggregateExpressions after execute PushFoldableIntoBranches rule, Then cause boundReference error.
For example

SELECT c * 2 AS d
FROM (
         SELECT if(b > 1, 1, b) AS c
         FROM (
                  SELECT if(a < 0, 0, a) AS b
                  FROM VALUES (-1), (1), (2) AS t1(a)
              ) t2
         GROUP BY b
     ) t3
GROUP BY c

Before pr

== Optimized Logical Plan ==
Aggregate [if ((b#0 > 1)) 1 else b#0], [if ((b#0 > 1)) 2 else (b#0 * 2) AS d#2]
+- Project [if ((a#3 < 0)) 0 else a#3 AS b#0]
   +- LocalRelation [a#3]

== Error ==
Couldn't find b#0 in [if ((b#0 > 1)) 1 else b#0#7]
java.lang.IllegalStateException: Couldn't find b#0 in [if ((b#0 > 1)) 1 else b#0#7]
	at org.apache.spark.sql.catalyst.expressions.BindReferences$$anonfun$bindReference$1.applyOrElse(BoundAttribute.scala:80)
	at org.apache.spark.sql.catalyst.expressions.BindReferences$$anonfun$bindReference$1.applyOrElse(BoundAttribute.scala:73)
	at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$transformDownWithPruning$1(TreeNode.scala:461)
	at org.apache.spark.sql.catalyst.trees.CurrentOrigin$.withOrigin(origin.scala:76)
	at org.apache.spark.sql.catalyst.trees.TreeNode.transformDownWithPruning(TreeNode.scala:461)
	at org.apache.spark.sql.catalyst.trees.TreeNode.$anonfun$transformDownWithPruning$3(TreeNode.scala:466)
	at org.apache.spark.sql.catalyst.trees.BinaryLike.mapChildren(TreeNode.scala:1241)
	at org.apache.spark.sql.catalyst.trees.BinaryLike.mapChildren$(TreeNode.scala:1240)
	at org.apache.spark.sql.catalyst.expressions.BinaryExpression.mapChildren(Expression.scala:653)
        ......

After pr

== Optimized Logical Plan ==
Aggregate [_groupingexpression#5], [(_groupingexpression#5 * 2) AS d#2]
+- Project [if ((b#0 > 1)) 1 else b#0 AS _groupingexpression#5]
   +- Project [if ((a#3 < 0)) 0 else a#3 AS b#0]
      +- LocalRelation [a#3]

Does this PR introduce any user-facing change?

No

How was this patch tested?

UT

…ndReference error PushFoldableIntoBranches in complex grouping expressions may cause bindReference error

wangyum · 2023-08-17T14:04:40Z

sql/core/src/test/scala/org/apache/spark/sql/DataFrameSuite.scala

@@ -3674,6 +3674,21 @@ class DataFrameSuite extends QueryTest
        parameters = Map("viewName" -> "AUTHORIZATION"))
    }
  }
+
+  test("SPARK-44846: PushFoldableIntoBranches in complex grouping expressions " +


Move the test to https://github.com/apache/spark/blob/master/sql/core/src/test/resources/sql-tests/inputs/group-by.sql?

wangyum · 2023-08-17T14:30:42Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/expressions.scala

+    case a @ Aggregate(groupingExpressions, _, _)
+      if !groupingExpressions.forall(_.isInstanceOf[NamedExpression]) => a


This is because the aggregate contains complex grouping expressions after RemoveRedundantAggregates. So could we fix it there.

Thanks. In PhysicalAggregation，if the expression is not a NamedExpressions, it add an alias. Also I'm not sure if complex expressions are generated anywhere else other than RemoveRedundantAggregates. So is it safer to fix PushFoldableIntoBranches, and the impact is smaller?

We had a similar issue before: https://issues.apache.org/jira/browse/SPARK-34581

Ok, later I will do something like PullOutGroupingExpressions after RemoveRedundantAggregates triggers Remove.

update update update update

wangyum · 2023-08-18T16:18:16Z

.../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, PullOutGroupingExpressions.apply(expected))


val expected = relation .select($"a", $"b", ($"a" + $"b") as "_groupingexpression") .groupBy($"_groupingexpression")($"_groupingexpression" as "c") .analyze

...alyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregates.scala

wangyum · 2023-08-19T00:22:47Z

Could you update the PR title and PR description?

zml1206 · 2023-08-19T02:16:13Z

Could you update the PR title and PR description?

Done.

wangyum · 2023-08-19T02:30:32Z

cc @cloud-fan @peter-toth

cloud-fan · 2023-08-23T04:37:18Z

...alyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregates.scala

@@ -42,11 +42,12 @@ object RemoveRedundantAggregates extends Rule[LogicalPlan] with AliasHelper {
      )

      // We might have introduces non-deterministic grouping expression


I have a different idea. I think it's risky to put the new grouping expressions back to the Aggregate, as the grouping expression contains things in the SELECT list. This is a long-standing issue and I feel it's better to just create a Project below the Aggregate to calculate grouping expressions, and other optimizer rules can merge/eliminate this extra Project if it only contains Attributes.

@cloud-fan Thanks. I'm sorry I didn't understand. Before this PR, it will create a Project below the Aggregate to Pulls out nondeterministic grouping expressions. In this PR, I keep this logic, and create a Project below the Aggregate to Pulls out complex grouping expressions by rule PullOutGroupingExpressions. I do not put the new grouping expressions back to the Aggregate.

let me put it this way: if we can safely remove a distinct-like Aggregate, it means we can just turn it into a Project (e.g. case a: Aggregate => Project(a.aggregateExpressions, a.child)).

The code today is merging the distinct-like Aggregate to the upper Aggregate, which is problematic and we have to pull something out to a Project case by case. This is fragile and this PR just finds another case.

My proposal is to not merge. Just convert the distinct-like Aggregate to Project, and let other optimizer rules decide if we can merge it or not.

Thank you, I understand, I will resubmit a PR later.

zml1206 · 2023-08-23T14:49:19Z

New PR: #42633

PushFoldableIntoBranches in complex grouping expressions may cause bi…

e2e5216

…ndReference error PushFoldableIntoBranches in complex grouping expressions may cause bindReference error

github-actions bot added the SQL label Aug 17, 2023

fix scalastyle

b116319

wangyum reviewed Aug 17, 2023

View reviewed changes

update

65abc3d

update update update update

wangyum reviewed Aug 18, 2023

View reviewed changes

...alyst/src/main/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregates.scala Outdated Show resolved Hide resolved

update

e9b1819

zml1206 changed the title ~~[SPARK-44846][SQL] PushFoldableIntoBranches in complex grouping expressions may cause bi…~~ [SPARK-44846][SQL] Pull out complex grouping expressions after remove redundant aggregates Aug 19, 2023

wangyum approved these changes Aug 19, 2023

View reviewed changes

cloud-fan reviewed Aug 23, 2023

View reviewed changes

zml1206 closed this Sep 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-44846][SQL] Pull out complex grouping expressions after remove redundant aggregates #42531

[SPARK-44846][SQL] Pull out complex grouping expressions after remove redundant aggregates #42531

zml1206 commented Aug 17, 2023 •

edited

Loading

wangyum Aug 17, 2023

wangyum Aug 17, 2023 •

edited

Loading

zml1206 Aug 17, 2023

wangyum Aug 17, 2023

zml1206 Aug 18, 2023

wangyum Aug 18, 2023

wangyum commented Aug 19, 2023

zml1206 commented Aug 19, 2023

wangyum commented Aug 19, 2023

cloud-fan Aug 23, 2023

zml1206 Aug 23, 2023

cloud-fan Aug 23, 2023

zml1206 Aug 23, 2023

zml1206 commented Aug 23, 2023

		case a @ Aggregate(groupingExpressions, _, _)
		if !groupingExpressions.forall(_.isInstanceOf[NamedExpression]) => a

		@@ -42,11 +42,12 @@ object RemoveRedundantAggregates extends Rule[LogicalPlan] with AliasHelper {
		)

		// We might have introduces non-deterministic grouping expression

[SPARK-44846][SQL] Pull out complex grouping expressions after remove redundant aggregates #42531

[SPARK-44846][SQL] Pull out complex grouping expressions after remove redundant aggregates #42531

Conversation

zml1206 commented Aug 17, 2023 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

wangyum Aug 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangyum commented Aug 19, 2023

zml1206 commented Aug 19, 2023

wangyum commented Aug 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zml1206 commented Aug 23, 2023

zml1206 commented Aug 17, 2023 •

edited

Loading

wangyum Aug 17, 2023 •

edited

Loading