[SPARK-31663][SQL] Grouping sets with having clause returns the wrong result #28501

xuanyuanking · 2020-05-11T14:53:37Z

What changes were proposed in this pull request?

Resolve the havingcondition with expanding the GROUPING SETS/CUBE/ROLLUP expressions together in ResolveGroupingAnalytics:
- Change the operations resolving directions to top-down.
- Try resolving the condition of the filter as though it is in the aggregate clause by reusing the function in ResolveAggregateFunctions
- Push the aggregate expressions into the aggregate which contains the expanded operations.
Use UnresolvedHaving for all having clause.

Why are the changes needed?

Correctness bug fix. See the demo and analysis in SPARK-31663.

Does this PR introduce any user-facing change?

Yes, correctness bug fix for HAVING with GROUPING SETS.

How was this patch tested?

New UTs added.

SparkQA · 2020-05-11T15:50:06Z

Test build #122510 has finished for PR 28501 at commit ced9568.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

SparkQA · 2020-05-12T19:33:38Z

Test build #122547 has finished for PR 28501 at commit 6f618ab.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-12T19:37:49Z

Test build #122548 has finished for PR 28501 at commit b48eb75.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class UnresolvedHaving(

SparkQA · 2020-05-12T20:07:35Z

Test build #122551 has finished for PR 28501 at commit 9d3dc8e.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

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

SparkQA · 2020-05-13T07:05:02Z

Test build #122572 has finished for PR 28501 at commit 47a3b33.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2020-05-13T08:31:04Z

retest this please

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

SparkQA · 2020-05-13T14:36:08Z

Test build #122580 has finished for PR 28501 at commit 47a3b33.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-14T10:31:10Z

Test build #122610 has finished for PR 28501 at commit 567ea7f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2020-05-14T11:20:51Z

retest this please

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala

cloud-fan · 2020-05-14T13:18:23Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

-      case _ =>
-        Filter(predicate, plan)
-    }
+    UnresolvedHaving(predicate, plan)


does global aggregate still work? e.g. UnresolvedHaving(Project(agg_func ...))

Yes, it still works, the UnresolvedHaving will be changed to Filter in rule ResolveAggregateFunction.

SparkQA · 2020-05-14T14:51:30Z

Test build #122615 has finished for PR 28501 at commit 9b3dfb4.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-14T15:29:05Z

Test build #122611 has finished for PR 28501 at commit 9b3dfb4.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

holdenk · 2020-05-14T22:28:03Z

I'm planning to cut RC2 tomorrow for 2.4.6, looking at the state of this PR I'm assuming it won't make it for 2.4.6 and we can put it in 2.4.7. Does that sound ok to folks?

xuanyuanking · 2020-05-15T03:23:44Z

@holdenk Thanks for notifying, I'll address all the comments today. Yep, if it can be merged before cutting 2.4.6, let's put it in 2.4.7.

cloud-fan · 2020-05-15T04:30:14Z

It's a long-standing bug, so it doesn't block 2.4.6. I'll see if I can merge it today.

SparkQA · 2020-05-15T07:05:02Z

Test build #122648 has finished for PR 28501 at commit 3b48e38.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2020-05-15T07:32:02Z

retest this please

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala

SparkQA · 2020-05-15T13:43:29Z

Test build #5038 has started for PR 28501 at commit 1de0c75.

SparkQA · 2020-05-15T14:18:23Z

Test build #122657 has finished for PR 28501 at commit 3b48e38.

This patch fails from timeout after a configured wait of 400m.
This patch merges cleanly.
This patch adds no public classes.

xuanyuanking · 2020-05-15T14:22:01Z

retest this please

SparkQA · 2020-05-15T20:26:13Z

Test build #122678 has finished for PR 28501 at commit 1de0c75.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-05-15T21:04:03Z

Test build #122679 has finished for PR 28501 at commit 1de0c75.

This patch fails from timeout after a configured wait of 400m.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-05-16T04:37:14Z

thanks, merging to master/3.0/2.4!

dongjoon-hyun · 2020-05-16T04:44:15Z

Thank you so much, @xuanyuanking and @cloud-fan .
cc @holdenk

… result - Resolve the havingcondition with expanding the GROUPING SETS/CUBE/ROLLUP expressions together in `ResolveGroupingAnalytics`: - Change the operations resolving directions to top-down. - Try resolving the condition of the filter as though it is in the aggregate clause by reusing the function in `ResolveAggregateFunctions` - Push the aggregate expressions into the aggregate which contains the expanded operations. - Use UnresolvedHaving for all having clause. Correctness bug fix. See the demo and analysis in SPARK-31663. Yes, correctness bug fix for HAVING with GROUPING SETS. New UTs added. Closes apache#28501 from xuanyuanking/SPARK-31663. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 86bd37f) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

dongjoon-hyun · 2020-05-16T05:06:51Z

Hi, @cloud-fan . This seems to be not in 2.4 yet.

thanks, merging to master/3.0/2.4!

… result - Resolve the havingcondition with expanding the GROUPING SETS/CUBE/ROLLUP expressions together in `ResolveGroupingAnalytics`: - Change the operations resolving directions to top-down. - Try resolving the condition of the filter as though it is in the aggregate clause by reusing the function in `ResolveAggregateFunctions` - Push the aggregate expressions into the aggregate which contains the expanded operations. - Use UnresolvedHaving for all having clause. Correctness bug fix. See the demo and analysis in SPARK-31663. Yes, correctness bug fix for HAVING with GROUPING SETS. New UTs added. Closes #28501 from xuanyuanking/SPARK-31663. Authored-by: Yuanjian Li <xyliyuanjian@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com> (cherry picked from commit 86bd37f) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2020-05-16T05:35:13Z

done. It has some conflicts, so I fixed them and ran tests locally, which takes some time.

WIP

ced9568

probot-autolabeler bot added the SQL label May 11, 2020

xuanyuanking added 3 commits May 12, 2020 20:02

fix test failure by expanding GROUPING SETS in AggregateWithHaving

ba97a6c

fix test related with subquery

6f618ab

rename

b48eb75

xuanyuanking changed the title ~~[WIP][SPARK-31663][SQL] Grouping sets with having clause returns the wrong result~~ [SPARK-31663][SQL] Grouping sets with having clause returns the wrong result May 12, 2020

rename and rephrase comment

9d3dc8e

cloud-fan reviewed May 12, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

dilipbiswal reviewed May 12, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

dilipbiswal reviewed May 12, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala Outdated Show resolved Hide resolved

xuanyuanking commented May 13, 2020

View reviewed changes

sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala Outdated Show resolved Hide resolved

xuanyuanking added 2 commits May 13, 2020 13:31

address comments

f2a2ad1

fix

47a3b33

cloud-fan reviewed May 13, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

xuanyuanking added 2 commits May 14, 2020 17:20

remove hack

567ea7f

rename

9b3dfb4

cloud-fan reviewed May 14, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

cloud-fan reviewed May 14, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

cloud-fan reviewed May 14, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

cloud-fan reviewed May 14, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

cloud-fan reviewed May 14, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/unresolved.scala Outdated Show resolved Hide resolved

cloud-fan reviewed May 14, 2020

View reviewed changes

address comments

3b48e38

cloud-fan reviewed May 15, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

cloud-fan reviewed May 15, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated Show resolved Hide resolved

address comment

1de0c75

cloud-fan approved these changes May 15, 2020

View reviewed changes

cloud-fan closed this in 86bd37f May 16, 2020

xuanyuanking deleted the SPARK-31663 branch May 21, 2020 05:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-31663][SQL] Grouping sets with having clause returns the wrong result #28501

[SPARK-31663][SQL] Grouping sets with having clause returns the wrong result #28501

xuanyuanking commented May 11, 2020 •

edited

Loading

SparkQA commented May 11, 2020

SparkQA commented May 12, 2020

SparkQA commented May 12, 2020

SparkQA commented May 12, 2020

SparkQA commented May 13, 2020

xuanyuanking commented May 13, 2020

SparkQA commented May 13, 2020

SparkQA commented May 14, 2020

xuanyuanking commented May 14, 2020

cloud-fan May 14, 2020

xuanyuanking May 15, 2020

SparkQA commented May 14, 2020

SparkQA commented May 14, 2020

holdenk commented May 14, 2020

xuanyuanking commented May 15, 2020

cloud-fan commented May 15, 2020

SparkQA commented May 15, 2020

xuanyuanking commented May 15, 2020

SparkQA commented May 15, 2020

SparkQA commented May 15, 2020

xuanyuanking commented May 15, 2020

SparkQA commented May 15, 2020

SparkQA commented May 15, 2020

cloud-fan commented May 16, 2020

dongjoon-hyun commented May 16, 2020

dongjoon-hyun commented May 16, 2020

cloud-fan commented May 16, 2020

[SPARK-31663][SQL] Grouping sets with having clause returns the wrong result #28501

[SPARK-31663][SQL] Grouping sets with having clause returns the wrong result #28501

Conversation

xuanyuanking commented May 11, 2020 • 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?

SparkQA commented May 11, 2020

SparkQA commented May 12, 2020

SparkQA commented May 12, 2020

SparkQA commented May 12, 2020

SparkQA commented May 13, 2020

xuanyuanking commented May 13, 2020

SparkQA commented May 13, 2020

SparkQA commented May 14, 2020

xuanyuanking commented May 14, 2020

cloud-fan May 14, 2020

Choose a reason for hiding this comment

xuanyuanking May 15, 2020

Choose a reason for hiding this comment

SparkQA commented May 14, 2020

SparkQA commented May 14, 2020

holdenk commented May 14, 2020

xuanyuanking commented May 15, 2020

cloud-fan commented May 15, 2020

SparkQA commented May 15, 2020

xuanyuanking commented May 15, 2020

SparkQA commented May 15, 2020

SparkQA commented May 15, 2020

xuanyuanking commented May 15, 2020

SparkQA commented May 15, 2020

SparkQA commented May 15, 2020

cloud-fan commented May 16, 2020

dongjoon-hyun commented May 16, 2020

dongjoon-hyun commented May 16, 2020

cloud-fan commented May 16, 2020

xuanyuanking commented May 11, 2020 •

edited

Loading