[SPARK-33131][SQL] Fix grouping sets with having clause can not resolve qualified col name #30029

ulysses-you · 2020-10-13T11:21:25Z

What changes were proposed in this pull request?

Correct the resolution of having clause.

Why are the changes needed?

Grouping sets construct new aggregate lost the qualified name of grouping expression. Here is a example:

-- Works resolved by `ResolveReferences`
select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having c1 = 1

-- Works because of the extra expression c1
select c1 as c2 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1

-- Failed
select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1

It wroks with Aggregate without grouping sets through ResolveReferences, but Grouping sets not works since the exprId has been changed.

Does this PR introduce any user-facing change?

Yes, bug fix.

How was this patch tested?

add test.

ulysses-you · 2020-10-13T11:23:01Z

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

+SELECT c1 FROM VALUES (1, 2) as t(c1, c2) GROUP BY GROUPING SETS(t.c1) HAVING t.c1 = 1;
+SELECT c1 FROM VALUES (1, 2) as t(c1, c2) GROUP BY CUBE(t.c1) HAVING t.c1 = 1;
+SELECT c1 FROM VALUES (1, 2) as t(c1, c2) GROUP BY ROLLUP(t.c1) HAVING t.c1 = 1;
+SELECT c1 FROM VALUES (1, 2) as t(c1, c2) GROUP BY t.c1 HAVING t.c1 = 1;


this test is just ensure we support it.

ulysses-you · 2020-10-14T05:47:00Z

@maropu @cloud-fan do you have time to take a look ?

cloud-fan · 2020-10-14T06:27:03Z

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

-        val exprMap = extraAggExprs.zip(
-          newChild.asInstanceOf[Aggregate].aggregateExpressions.takeRight(
-            extraAggExprs.length)).toMap
+        // We also add the original aggregateExpressions in Map to avoid extraAggExprs is empty.


what's wrong if extraAggExprs is empty?

If we resolved the having condition but extraAggExprs is empty, that means the resolution is in original aggregateExpressions.

We need use original aggregateExpressions mapping having condition NamedExpression to the new since grouping sets constructAggregate will build the new alias for it.

I don't get it. The comment says

// Since the exprId of extraAggExprs will be changed in the constructed aggregate, and the // aggregateExpressions keeps the input order. So here we build an exprMap to resolve the // condition again.

I don't see why we need to include the original aggregateExpressions in exprMap.

The comment is right if we not return the resolved having condition when exprMap is empty.

This pr break the behavior that return it. If we not add the original aggregateExpressions, we can't mapping the resolved having condition since exprMap is empty.

can you make the comment clearer?

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

cloud-fan · 2020-10-14T11:37:06Z

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

@@ -2315,7 +2315,8 @@ class Analyzer(
          if (aggregateExpressions.nonEmpty) {
            Some(aggregateExpressions.toSeq, transformedAggregateFilter)
          } else {
-            None
+            // We need to return the resolved having condition
+            Some(Seq.empty, transformedAggregateFilter.asInstanceOf[Alias].child)


what if we don't trim the Alias here?

maropu · 2020-10-14T14:21:06Z

sql/core/src/test/resources/sql-tests/results/having.sql.out

+
+
+-- !query
+SELECT c1 FROM VALUES (1, 2) as t(c1, c2) GROUP BY t.c1 HAVING t.c1 = 1


It seems this test can pass even in master.

Yeah this test is just for better coverage.

SparkQA · 2020-10-14T20:17:31Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34354/

SparkQA · 2020-10-14T20:41:28Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34354/

SparkQA · 2020-10-14T21:08:49Z

Test build #129748 has finished for PR 30029 at commit d3893b2.

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

ulysses-you · 2020-10-15T00:03:25Z

Seems this way will introduce unexpect thing.

We want to return the resolved having condition which is extra expression or exists in grouping expression. Current pr implementation will break such sql

select c1, count(*) as c2 from values(1, 2) as t(c1, c2) group by c1 having c1 = 1 and c2 = 1;

Let me find an another way to solve it.

SparkQA · 2020-10-15T04:55:52Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34398/

SparkQA · 2020-10-15T05:19:50Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34398/

cloud-fan · 2020-10-15T06:38:59Z

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

@@ -470,7 +470,7 @@ class Analyzer(
     */
    private def constructGroupByAlias(groupByExprs: Seq[Expression]): Seq[Alias] = {
      groupByExprs.map {
-        case e: NamedExpression => Alias(e, e.name)()
+        case e: NamedExpression => Alias(e, e.name)(qualifier = e.qualifier)


ah this is a good catch! do we have this bug in 3.0 and 2.4?

Yeah, seems it's a long time bug.

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

Test build #129791 has finished for PR 30029 at commit a4c2028.

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

cloud-fan · 2020-10-16T11:26:24Z

GA passed, thanks, merging to master!

cloud-fan · 2020-10-16T11:27:15Z

@ulysses-you can you send backport PRs for 3.0 and 2.4? thanks!

ulysses-you · 2020-10-16T14:28:48Z

@cloud-fan sure will do it tomorrow.

…resolve qualified col name This is [#30029](#30029) backport for branch-3.0. ### What changes were proposed in this pull request? Correct the resolution of having clause. ### Why are the changes needed? Grouping sets construct new aggregate lost the qualified name of grouping expression. Here is a example: ``` -- Works resolved by `ResolveReferences` select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having c1 = 1 -- Works because of the extra expression c1 select c1 as c2 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1 -- Failed select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1 ``` It wroks with `Aggregate` without grouping sets through `ResolveReferences`, but Grouping sets not works since the exprId has been changed. ### Does this PR introduce _any_ user-facing change? Yes, bug fix. ### How was this patch tested? add test. Closes #30077 from ulysses-you/SPARK-33131-branch-3.0. Authored-by: ulysses <youxiduo@weidian.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

…resolve qualified col name This is [#30029](#30029) backport for branch-2.4. ### What changes were proposed in this pull request? Correct the resolution of having clause. ### Why are the changes needed? Grouping sets construct new aggregate lost the qualified name of grouping expression. Here is a example: ``` -- Works resolved by `ResolveReferences` select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having c1 = 1 -- Works because of the extra expression c1 select c1 as c2 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1 -- Failed select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1 ``` It wroks with `Aggregate` without grouping sets through `ResolveReferences`, but Grouping sets not works since the exprId has been changed. ### Does this PR introduce _any_ user-facing change? Yes, bug fix. ### How was this patch tested? add test. Closes #30075 from ulysses-you/SPARK-33131-branch-2.4. Authored-by: ulysses <youxiduo@weidian.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…resolve qualified col name This is [apache#30029](apache#30029) backport for branch-3.0. ### What changes were proposed in this pull request? Correct the resolution of having clause. ### Why are the changes needed? Grouping sets construct new aggregate lost the qualified name of grouping expression. Here is a example: ``` -- Works resolved by `ResolveReferences` select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having c1 = 1 -- Works because of the extra expression c1 select c1 as c2 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1 -- Failed select c1 from values (1) as t1(c1) group by grouping sets(t1.c1) having t1.c1 = 1 ``` It wroks with `Aggregate` without grouping sets through `ResolveReferences`, but Grouping sets not works since the exprId has been changed. ### Does this PR introduce _any_ user-facing change? Yes, bug fix. ### How was this patch tested? add test. Closes apache#30077 from ulysses-you/SPARK-33131-branch-3.0. Authored-by: ulysses <youxiduo@weidian.com> Signed-off-by: Dongjoon Hyun <dhyun@apple.com>

ulysses-you commented Oct 13, 2020

View reviewed changes

cloud-fan reviewed Oct 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 Oct 14, 2020

View reviewed changes

maropu reviewed Oct 14, 2020

View reviewed changes

alias

a4c2028

ulysses-you force-pushed the SPARK-33131 branch from d3893b2 to a4c2028 Compare October 15, 2020 03:45

cloud-fan reviewed Oct 15, 2020

View reviewed changes

cloud-fan approved these changes Oct 15, 2020

View reviewed changes

dongjoon-hyun added the SQL label Oct 15, 2020

cloud-fan closed this in 3ae1520 Oct 16, 2020

This was referenced Oct 17, 2020

[SPARK-33131][SQL][2.4] Fix grouping sets with having clause can not resolve qualified col name #30075

Closed

[SPARK-33131][SQL][3.0] Fix grouping sets with having clause can not resolve qualified col name #30077

Closed

ulysses-you deleted the SPARK-33131 branch February 19, 2021 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-33131][SQL] Fix grouping sets with having clause can not resolve qualified col name #30029

[SPARK-33131][SQL] Fix grouping sets with having clause can not resolve qualified col name #30029

ulysses-you commented Oct 13, 2020 •

edited

Loading

ulysses-you Oct 13, 2020

ulysses-you commented Oct 14, 2020

cloud-fan Oct 14, 2020

ulysses-you Oct 14, 2020

cloud-fan Oct 14, 2020

ulysses-you Oct 14, 2020 •

edited

Loading

cloud-fan Oct 14, 2020

cloud-fan Oct 14, 2020

maropu Oct 14, 2020

ulysses-you Oct 14, 2020

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

ulysses-you commented Oct 15, 2020

SparkQA commented Oct 15, 2020

SparkQA commented Oct 15, 2020

cloud-fan Oct 15, 2020

ulysses-you Oct 15, 2020

SparkQA commented Oct 15, 2020

cloud-fan commented Oct 16, 2020

cloud-fan commented Oct 16, 2020

ulysses-you commented Oct 16, 2020



		-- !query
		SELECT c1 FROM VALUES (1, 2) as t(c1, c2) GROUP BY t.c1 HAVING t.c1 = 1

[SPARK-33131][SQL] Fix grouping sets with having clause can not resolve qualified col name #30029

[SPARK-33131][SQL] Fix grouping sets with having clause can not resolve qualified col name #30029

Conversation

ulysses-you commented Oct 13, 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?

Choose a reason for hiding this comment

ulysses-you commented Oct 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulysses-you Oct 14, 2020 • 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

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

SparkQA commented Oct 14, 2020

ulysses-you commented Oct 15, 2020

SparkQA commented Oct 15, 2020

SparkQA commented Oct 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Oct 15, 2020

cloud-fan commented Oct 16, 2020

cloud-fan commented Oct 16, 2020

ulysses-you commented Oct 16, 2020

ulysses-you commented Oct 13, 2020 •

edited

Loading

ulysses-you Oct 14, 2020 •

edited

Loading