You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zml1206 (via GitHub)" <gi...@apache.org> on 2023/08/23 14:47:34 UTC
[GitHub] [spark] zml1206 opened a new pull request, #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
zml1206 opened a new pull request, #42633:
URL: https://github.com/apache/spark/pull/42633
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
This PR provides a safe way to remove a distinct-like `Aggregate` in rule `RemoveRedundantAggregates`. Just Convert the distinct-like `Aggregate` to `Project`.
### 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 [c#1], [(c#1 * 2) AS d#2]
+- Project [if ((b#0 > 1)) 1 else b#0 AS c#1]
+- 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] wangyum closed pull request #42633: [SPARK-44846][SQL] Convert the lower redundant Aggregate to Project in RemoveRedundantAggregates
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum closed pull request #42633: [SPARK-44846][SQL] Convert the lower redundant Aggregate to Project in RemoveRedundantAggregates
URL: https://github.com/apache/spark/pull/42633
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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 diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1304313007
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -30,7 +30,8 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Batch("RemoveRedundantAggregates", FixedPoint(10),
- RemoveRedundantAggregates) :: Nil
+ RemoveRedundantAggregates,
+ RemoveNoopOperators) :: Nil
Review Comment:
can we also add `CollpaseProject`? So that there is no adjacent `.select`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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 diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1304431931
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -30,7 +30,8 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Batch("RemoveRedundantAggregates", FixedPoint(10),
- RemoveRedundantAggregates) :: Nil
+ RemoveRedundantAggregates,
+ RemoveNoopOperators) :: Nil
Review Comment:
The `rand(0)` is only referenced once, I think `CollpaseProject` can handle 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.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] zml1206 commented on a diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1304347456
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -30,7 +30,8 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Batch("RemoveRedundantAggregates", FixedPoint(10),
- RemoveRedundantAggregates) :: Nil
+ RemoveRedundantAggregates,
+ RemoveNoopOperators) :: Nil
Review Comment:
`CollpaseProject` can not Collpase projects because `rand(0)` is non-deterministic. But there is still a little problem in `RemoveRedundantAggregatesSuite`, I will deal with it right away.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] zml1206 commented on a diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1304347456
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -30,7 +30,8 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Batch("RemoveRedundantAggregates", FixedPoint(10),
- RemoveRedundantAggregates) :: Nil
+ RemoveRedundantAggregates,
+ RemoveNoopOperators) :: Nil
Review Comment:
`CollpaseProject` can not collpase projects because `rand(0)` is non-deterministic. But there is still a little problem in `RemoveRedundantAggregatesSuite`, I will deal with it right away.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] wangyum commented on pull request #42633: [SPARK-44846][SQL] Convert the lower redundant Aggregate to Project in RemoveRedundantAggregates
Posted by "wangyum (via GitHub)" <gi...@apache.org>.
wangyum commented on PR #42633:
URL: https://github.com/apache/spark/pull/42633#issuecomment-1705181689
Merged to master and branch-3.5.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] zml1206 commented on pull request #42633: [SPARK-44846][SQL] Convert the lower redundant Aggregate to Project in RemoveRedundantAggregates
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on PR #42633:
URL: https://github.com/apache/spark/pull/42633#issuecomment-1705264377
Thanks. @cloud-fan @wangyum
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] zml1206 commented on a diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1304049000
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -52,6 +52,7 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
.groupBy($"a")($"a")
.analyze
val expected = relation
+ .select($"a")
Review Comment:
OK, done. But only slightly less change, because rule `RemoveNoopOperators` need `Project(projectList, child) if child.sameOutput(p)`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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 diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1303943447
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -52,6 +52,7 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
.groupBy($"a")($"a")
.analyze
val expected = relation
+ .select($"a")
Review Comment:
to avoid making many changes in this suite, can we add the rule `RemoveNoopOperators` to the test Batch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] zml1206 commented on a diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1304049000
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -52,6 +52,7 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
.groupBy($"a")($"a")
.analyze
val expected = relation
+ .select($"a")
Review Comment:
OK, done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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] zml1206 commented on a diff in pull request #42633: [SPARK-44846][SQL] Convert the distinct-like Aggregate to Project
Posted by "zml1206 (via GitHub)" <gi...@apache.org>.
zml1206 commented on code in PR #42633:
URL: https://github.com/apache/spark/pull/42633#discussion_r1304449708
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/optimizer/RemoveRedundantAggregatesSuite.scala:
##########
@@ -30,7 +30,8 @@ class RemoveRedundantAggregatesSuite extends PlanTest {
object Optimize extends RuleExecutor[LogicalPlan] {
val batches = Batch("RemoveRedundantAggregates", FixedPoint(10),
- RemoveRedundantAggregates) :: Nil
+ RemoveRedundantAggregates,
+ RemoveNoopOperators) :: Nil
Review Comment:
```
producer.deterministic && (count == 1 || alwaysInline || cheapToInlineProducer)
```
count is 1, but producer.deterministic is false.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
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