[SPARK-16639][SQL] The query with having condition that contains grouping by column should work #14296

viirya · 2016-07-21T05:08:13Z

What changes were proposed in this pull request?

The query with having condition that contains grouping by column will be failed during analysis. E.g.,

create table tbl(a int, b string);
select count(b) from tbl group by a + 1 having a + 1 = 2;

Having condition should be able to use grouping by column.

How was this patch tested?

Jenkins tests.

…ld work.

cloud-fan · 2016-07-21T05:13:16Z

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

+              case e: Expression =>
+                val alias = Alias(e, e.toString)()
+                aggregateExpressions += alias
+                alias.toAttribute


looks like we are blindly pushing all expression into Aggregate, how about we just push the whole filter condition?

You mean we don't replace and push the expressions, but just push the whole filter condition?

~~Looks like pushing the whole filter condition brings more problem.~~

Not the problem of whole filter condition pushdown. But the problem of this approach. I will update this.

SparkQA · 2016-07-21T05:49:55Z

Test build #62655 has finished for PR 14296 at commit 5704709.

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

SparkQA · 2016-07-22T03:18:00Z

Test build #62702 has finished for PR 14296 at commit 4ca4088.

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

viirya · 2016-07-22T08:14:01Z

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

@@ -1207,6 +1207,12 @@ class Analyzer(
                val alias = Alias(ae, ae.toString)()
                aggregateExpressions += alias
                alias.toAttribute
+              case ne: NamedExpression => ne
+              case e: Expression if grouping.exists(_.semanticEquals(e)) &&


I think we should not push down all expressions or the whole expression. Only push down the group by expressions or aggregate expressions.

SparkQA · 2016-07-22T09:54:27Z

Test build #62722 has finished for PR 14296 at commit 6d0d753.

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

clockfly · 2016-07-22T17:10:25Z

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

@@ -1207,6 +1207,12 @@ class Analyzer(
                val alias = Alias(ae, ae.toString)()
                aggregateExpressions += alias
                alias.toAttribute
+              case ne: NamedExpression => ne
+              case e: Expression if grouping.exists(_.semanticEquals(e)) &&
+                  !ResolveGroupingAnalytics.hasGroupingFunction(e) =>


Why !ResolveGroupingAnalytics.hasGroupingFunction(e) is needed? Do we want a test case for this check?

I am not near my laptop. But pushing grouping function causes test failed in SQLQuerySuite. I remember there is another rule taking care of grouping function.

It will be great if we can figure it out and add comment here.

yah. ResolveGroupingAnalytics performs grouping id and grouping function replacement. We should skip pushdown them here.

viirya · 2016-07-23T05:03:55Z

@cloud-fan any more comments? Thanks!

cloud-fan · 2016-07-25T05:14:58Z

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

@@ -1207,6 +1207,12 @@ class Analyzer(
                val alias = Alias(ae, ae.toString)()
                aggregateExpressions += alias
                alias.toAttribute
+              case ne: NamedExpression => ne


why this case?

I think we don't need to replace named expr (attributes, alias)?

but case e: Expression if grouping.exists(_.semanticEquals(e)) will filter them out right?

Removing it will cause error in replaceGroupingFunc. As we create a new attribute based on the grouping column with additional alias, we can't find it in aggregate's group by expressions.

…rouping-column

SparkQA · 2016-07-25T09:30:55Z

Test build #62808 has finished for PR 14296 at commit 8bab985.

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

cloud-fan · 2016-07-25T09:34:15Z

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

+              case ne: NamedExpression => ne
+              // Grouping functions are handled in the rule [[ResolveGroupingAnalytics]].
+              case e: Expression if grouping.exists(_.semanticEquals(e)) &&
+                  !ResolveGroupingAnalytics.hasGroupingFunction(e) =>


so what will happen if we use grouping function in HAVING? will the query fail?

You mean if we don't add this check? Yes. As the grouping function is pushed down to Aggregation, the rule ResolveGroupingAnalytics can't perform the replacement of grouping function.

I mean with this check. Can we resolve grouping function inside Filter?

Yes. SQLQuerySuite has test for with grouping function in HAVING.

viirya · 2016-07-27T02:05:14Z

ping @cloud-fan Any more comments? Thanks.

cloud-fan · 2016-07-27T04:51:32Z

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

+              // Replacing [[NamedExpression]] causes the error on [[Grouping]] because the
+              // grouping column will be new attribute created by adding additional [[Alias]].
+              // So we can't find the grouping column and replace it in the rule
+              // [[ResolveGroupingAnalytics]].


I don't quite understand this comment, can you give a concrete example?

When there is a Grouping(col#1) in the HAVING clause. If we push down it to Aggregate and use an Alias instead, the grouping function become Grouping(col#1#2). Then in the rule ResolveGroupingAnalytics we try to find the index of col in group by expressions, we can't find it.

but in the next case, we will check: 1) if it's a grouping column. 2) if it's not grouping function. So how can this happen?

Since we do a transform, the inner col of Grouping will match this case.

what about we do some improve for next case:

add one more check: the expression should not exist in the chid output

if the expression is a NamedExpression already, don't alias it.

I updated this. Please take a look.

SparkQA · 2016-07-28T10:04:16Z

Test build #62961 has finished for PR 14296 at commit f5e037a.

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

cloud-fan · 2016-07-28T14:34:54Z

thanks, merging to master!

…ping by column should work ## What changes were proposed in this pull request? The query with having condition that contains grouping by column will be failed during analysis. E.g., create table tbl(a int, b string); select count(b) from tbl group by a + 1 having a + 1 = 2; Having condition should be able to use grouping by column. ## How was this patch tested? Jenkins tests. Author: Liang-Chi Hsieh <simonh@tw.ibm.com> Closes #14296 from viirya/having-contains-grouping-column. (cherry picked from commit 9ade77c) Signed-off-by: Wenchen Fan <wenchen@databricks.com>

cloud-fan · 2016-07-28T14:38:30Z

also backport it to 2.0

viirya · 2016-07-28T15:00:16Z

Thanks for reviewing this.

The query with having condition that contains grouping by column shou…

5704709

…ld work.

cloud-fan reviewed Jul 21, 2016
View reviewed changes

Address comment and fix test.

4ca4088

Fix test.

6d0d753

viirya reviewed Jul 22, 2016
View reviewed changes

clockfly reviewed Jul 22, 2016
View reviewed changes

cloud-fan reviewed Jul 25, 2016
View reviewed changes

viirya added 2 commits July 25, 2016 14:51

Merge remote-tracking branch 'upstream/master' into having-contains-g…

29b4734

…rouping-column

Add comment and use withTempView.

8bab985

cloud-fan reviewed Jul 25, 2016
View reviewed changes

cloud-fan reviewed Jul 27, 2016
View reviewed changes

Address comment.

f5e037a

asfgit closed this in 9ade77c Jul 28, 2016

viirya deleted the having-contains-grouping-column branch December 27, 2023 18:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-16639][SQL] The query with having condition that contains grouping by column should work #14296

[SPARK-16639][SQL] The query with having condition that contains grouping by column should work #14296

viirya commented Jul 21, 2016

cloud-fan Jul 21, 2016

viirya Jul 21, 2016

cloud-fan Jul 21, 2016

viirya Jul 22, 2016 •

edited

Loading

SparkQA commented Jul 21, 2016

SparkQA commented Jul 22, 2016

viirya Jul 22, 2016 •

edited

Loading

SparkQA commented Jul 22, 2016

clockfly Jul 22, 2016

viirya Jul 23, 2016

cloud-fan Jul 25, 2016 •

edited

Loading

viirya Jul 25, 2016

viirya commented Jul 23, 2016

cloud-fan Jul 25, 2016

viirya Jul 25, 2016

cloud-fan Jul 25, 2016

viirya Jul 25, 2016 •

edited

Loading

SparkQA commented Jul 25, 2016

cloud-fan Jul 25, 2016

viirya Jul 25, 2016

cloud-fan Jul 26, 2016

viirya Jul 26, 2016

viirya commented Jul 27, 2016

cloud-fan Jul 27, 2016

viirya Jul 27, 2016

cloud-fan Jul 27, 2016

viirya Jul 27, 2016

cloud-fan Jul 27, 2016

viirya Jul 28, 2016

SparkQA commented Jul 28, 2016

cloud-fan commented Jul 28, 2016

cloud-fan commented Jul 28, 2016

viirya commented Jul 28, 2016

[SPARK-16639][SQL] The query with having condition that contains grouping by column should work #14296

[SPARK-16639][SQL] The query with having condition that contains grouping by column should work #14296

Conversation

viirya commented Jul 21, 2016

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jul 21, 2016

SparkQA commented Jul 22, 2016

viirya Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jul 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented Jul 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Jul 25, 2016 • edited Loading

Choose a reason for hiding this comment

SparkQA commented Jul 25, 2016

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

viirya commented Jul 27, 2016

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

Choose a reason for hiding this comment

SparkQA commented Jul 28, 2016

cloud-fan commented Jul 28, 2016

cloud-fan commented Jul 28, 2016

viirya commented Jul 28, 2016

viirya Jul 22, 2016 •

edited

Loading

viirya Jul 22, 2016 •

edited

Loading

cloud-fan Jul 25, 2016 •

edited

Loading

viirya Jul 25, 2016 •

edited

Loading