[SPARK-24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET #21813

dilipbiswal · 2018-07-19T05:14:53Z

What changes were proposed in this pull request?

Enhances the parser and analyzer to support ANSI compliant syntax for GROUPING SET. As part of this change we derive the grouping expressions from user supplied groupings in the grouping sets clause.

SELECT c1, c2, max(c3) 
FROM t1
GROUP BY GROUPING SETS ((c1), (c1, c2))

How was this patch tested?

Added tests in SQLQueryTestSuite and ResolveGroupingAnalyticsSuite.

Please review http://spark.apache.org/contributing.html before opening a pull request.

SparkQA · 2018-07-19T07:05:02Z

Test build #93260 has finished for PR 21813 at commit b5ada3f.

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

dilipbiswal · 2018-07-19T07:09:08Z

retest this please

SparkQA · 2018-07-19T11:11:11Z

Test build #93266 has finished for PR 21813 at commit b5ada3f.

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

viirya · 2018-07-19T17:41:27Z

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

        aggregationExprs: Seq[NamedExpression],
        child: LogicalPlan): LogicalPlan = {
+
+


this blank lines can be removed?

viirya · 2018-07-19T17:41:54Z

sql/catalyst/src/main/antlr4/org/apache/spark/sql/catalyst/parser/SqlBase.g4

 fromClause
    : FROM relation (',' relation)* lateralView* pivotClause?
    ;
-


revert this?

viirya · 2018-07-19T19:16:51Z

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

        child: LogicalPlan): LogicalPlan = {
      val gid = AttributeReference(VirtualColumn.groupingIdName, IntegerType, false)()

+      val finalGroupByExpressions = if (groupByExprs == Nil) {


Shouldn't we do this in the branch of case x: GroupingSets if x.expressions.forall(_.resolved) =>? I think this constructAggregate method is also used by other clauses like Cube and Rollup.

@viirya Yeah.. so for cube and rollup, we will always have groupByExprs setup right ? So i felt its better to keep the code consolidated here in this function. What do u think ?

Ok. Mind to add a comment on this like SPARK-24424: this only happens for ANSI-SQL compliant syntax for GROUPING SET?

@viirya Sure will do.

viirya · 2018-07-19T19:20:33Z

...st/src/test/scala/org/apache/spark/sql/catalyst/analysis/ResolveGroupingAnalyticsSuite.scala

+
+    val originalPlan2 = GroupingSets(Seq(Seq(), Seq(unresolved_a), Seq(unresolved_a, unresolved_b)),
+      Nil, r1,
+      Seq(unresolved_a, unresolved_b, UnresolvedAlias(count(unresolved_c))))


hmm, I think originalPlan2 looks the same as originalPlan?

@viirya Thanks.. u right. I will remove it.

SparkQA · 2018-07-19T20:07:17Z

Test build #93291 has finished for PR 21813 at commit ac8f04f.

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

SparkQA · 2018-07-20T00:56:18Z

Test build #93299 has finished for PR 21813 at commit 7cf187d.

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

viirya · 2018-07-20T00:57:14Z

sql/core/src/test/resources/sql-tests/inputs/grouping_set.sql

+HAVING GROUPING__ID > 1;
+
+-- Group sets without explicit group by
+SELECT grouping(c1) FROM (VALUES ('x', 'a', 10), ('y', 'b', 20)) AS t (c1, c2, c3) GROUP BY c1,c2 GROUPING SETS (c1,c2);


Doesn't this have explicit group by?

@viirya sorry.. yeah.. i will remove the explicit group by cols.. I had it for testing but forgot to take them out.

viirya · 2018-07-20T01:00:53Z

sql/core/src/test/resources/sql-tests/inputs/grouping_set.sql

+FROM   (VALUES (1, 2), (3, 2)) t(c1, c2)
+GROUP  BY GROUPING SETS ( ( c1 ), ( c1, c2 ) )
+HAVING col2 IS NOT NULL
+ORDER  BY -col1;


I've manually verified the results should be correct.

@viirya Sorry Simon.. do i have to do something for this comment ?

Not at all. @dilipbiswal :-)

viirya · 2018-07-20T01:05:10Z

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

+      // can be null. In such case, we derive the groupByExprs from the user supplied values for
+      // grouping sets.
+      val finalGroupByExpressions = if (groupByExprs == Nil) {
+        selectedGroupByExprs.flatten.foldLeft(Seq.empty[Expression]) { (result, currentExpr) =>


What if GROUP BY GROUPING SETS (())? Is it a valid query?

@viirya No. We should be getting an error as we don't have a group by specification. I had tried this scenario against db2 to double check.

Can we have a test case for it too?

@viirya Yeah.. was already adding it .. knew u would ask :-)

@dilipbiswal Thanks! :-)

SparkQA · 2018-07-20T03:44:07Z

Test build #93304 has finished for PR 21813 at commit e0c57f7.

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

SparkQA · 2018-07-20T06:40:04Z

Test build #93313 has finished for PR 21813 at commit 2ecf3e1.

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

gatorsmile

LGTM

gatorsmile · 2018-07-20T06:52:28Z

Thanks! Merged to master.

dilipbiswal · 2018-07-20T07:20:33Z

Thank you very much @gatorsmile @viirya

[SPARK-24424] Support ANSI-SQL compliant syntax for GROUPING SET

b5ada3f

dilipbiswal changed the title ~~[SPARK 24424] Support ANSI-SQL compliant syntax for GROUPING SET~~ [SPARK 24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET Jul 19, 2018

dilipbiswal changed the title ~~[SPARK 24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET~~ [SPARK-24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET Jul 19, 2018

viirya reviewed Jul 19, 2018

View reviewed changes

code review

ac8f04f

viirya reviewed Jul 19, 2018

View reviewed changes

dilipbiswal added 2 commits July 19, 2018 13:49

Remove redundant test

7cf187d

Add comment

e0c57f7

viirya reviewed Jul 20, 2018

View reviewed changes

Add one more negative test

2ecf3e1

gatorsmile reviewed Jul 20, 2018

View reviewed changes

asfgit closed this in 2b91d99 Jul 20, 2018

rdblue mentioned this pull request Jun 7, 2019

[SPARK-27919][SQL] Add v2 session catalog #24768

Closed

		aggregationExprs: Seq[NamedExpression],
		child: LogicalPlan): LogicalPlan = {

[SPARK-24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET #21813

[SPARK-24424][SQL] Support ANSI-SQL compliant syntax for GROUPING SET #21813

Uh oh!

Conversation

dilipbiswal commented Jul 19, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 19, 2018

Uh oh!

dilipbiswal commented Jul 19, 2018

Uh oh!

SparkQA commented Jul 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 19, 2018

Uh oh!

SparkQA commented Jul 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilipbiswal Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 20, 2018

Uh oh!

SparkQA commented Jul 20, 2018

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Jul 20, 2018

Uh oh!

dilipbiswal commented Jul 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

viirya Jul 20, 2018 •

edited

Loading

dilipbiswal Jul 20, 2018 •

edited

Loading