[SPARK-31334][SQL] Don't ResolveReference/ResolveMissingReference when Filter condition with aggregate expression #28107

AngersZhuuuu · 2020-04-03T08:32:49Z

What changes were proposed in this pull request?

As I have show in https://issues.apache.org/jira/browse/SPARK-31334 's description, same type of sql, when one column type is different as string, catalyst can't analyze it right.

For test sql

test("xxxxxxxx") {
    Seq(
      ("1", "3"),
      ("2", "3"),
      ("3", "6"),
      ("4", "7"),
      ("5", "9"),
      ("6", "9")
    ).toDF("a", "b").createOrReplaceTempView("testData")

    val x = sql(
      """
        | SELECT b, sum(a) as a
        | FROM testData
        | GROUP BY b
        | HAVING sum(a) > 3
      """.stripMargin)

    x.explain()
    x.show()
  }

When analyze having clause's condition by ResolveAggregateFunctions

'Filter ('sum('a) > 3)
+- Aggregate [b#181], [b#181, sum(a#180) AS a#184L]
   +- SubqueryAlias `testdata`
      +- Project [_1#177 AS a#180, _2#178 AS b#181]
         +- LocalRelation [_1#177, _2#178]

Since a is StringType type and then aggregation's agg expression is unresolved (Because Sum. checkInputDataTypes() need NumericType, but a is StringType), so ResolveAggregateFunctions won't make a change on above LogicalPlan, then sum(a) in Filter condition will be resolved in ResolveReference and this a will be resolved as aggregation's output column a#184 , then error happened .

Why are the changes needed?

Fix bug in analyzer

Does this PR introduce any user-facing change?

NO

How was this patch tested?

Added UT

AngersZhuuuu · 2020-04-03T08:34:56Z

cc @dongjoon-hyun @wangyum

cloud-fan · 2020-04-03T09:27:11Z

thanks for reporting this bug! It's fragile to rely on rule order, can we fix it more completely? My proposal: Do not resolve Filter if it contains agg functions. We can implement it in ResolveReferences

AngersZhuuuu · 2020-04-03T09:35:01Z

thanks for reporting this bug! It's fragile to rely on rule order, can we fix it more completely? My proposal: Do not resolve Filter if it contains agg functions. We can implement it in ResolveReferences

Yea, I know it's fragile to change order, but don't have other good ideal, Thanks for you suggestions, I will test follow your suggestion.

AngersZhuuuu · 2020-04-03T14:30:14Z

Do not resolve Filter if it contains agg functions. We can implement it in ResolveReferences

Changed

cloud-fan · 2020-04-03T14:31:52Z

ok to test

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

cloud-fan · 2020-04-03T14:48:07Z

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

@@ -1391,11 +1391,30 @@ class Analyzer(
              notMatchedActions = newNotMatchedActions)
        }

+      case f @ Filter(cond, _) if containsAggregate(cond) => f


add a blank line between cases.

add a blank line between cases.

Done

cloud-fan · 2020-04-03T14:48:30Z

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

        q.mapExpressions(resolveExpressionTopDown(_, q))
    }

+    def containsAggregate(e: Expression): Boolean = {


why can't we reuse ResolveAggregateFunctions.containsAggregate?

why can't we reuse ResolveAggregateFunctions.containsAggregate?

Since here function is still UnresolvedFunction, we can't just reuse this.

SparkQA · 2020-04-03T14:55:14Z

Test build #120779 has finished for PR 28107 at commit 0d81b4d.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-04-03T15:06:02Z

Test build #120778 has finished for PR 28107 at commit 24e9d4d.

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

SparkQA · 2020-04-03T15:20:56Z

Test build #120781 has finished for PR 28107 at commit a5cf877.

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

SparkQA · 2020-04-03T16:10:08Z

Test build #120776 has finished for PR 28107 at commit 54df9d3.

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

SparkQA · 2020-04-04T05:31:32Z

Test build #120792 has finished for PR 28107 at commit 4a799fe.

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

SparkQA · 2020-04-04T07:01:52Z

Test build #120798 has finished for PR 28107 at commit 5f6eef1.

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

AngersZhuuuu · 2020-04-04T08:19:48Z

Test build #120798 has finished for PR 28107 at commit 5f6eef1.

This patch fails Spark unit tests.

This patch merges cleanly.

This patch adds no public classes.

For this test, failed because filter's cond 's expr is unresolved so lookup registered udf failed

SparkQA · 2020-04-04T13:26:19Z

Test build #120799 has finished for PR 28107 at commit cc0b018.

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

AngersZhuuuu · 2020-04-04T13:33:31Z

@cloud-fan
pass other test now. Minimize the scope of this pr's influence.
But I am worried that the processing of the Filter is mixed now between havingClause and where condition.

These makes future job more complex, maybe when beginning, it's better to use a new Class present havingClause and convert it to Filter in Analyzer after handing it.

SparkQA · 2020-04-04T18:08:14Z

Test build #120803 has finished for PR 28107 at commit b2855bd.

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

cloud-fan · 2020-04-06T07:19:44Z

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

+      (6, 9)
+    ).toDF("a", "b").createOrReplaceTempView("testData1")
+
+    checkAnswer(sql(


does this test fail before your patch?

does this test fail before your patch?

No, it's won't failed, here is for contrast.

So this test is not qualified to reproduce the bug?

So this test is not qualified to reproduce the bug?

The first SQL is used for comparison, and the second can reproduce bugs.
If don't need, we can just delete first one.

let's delete

let's delete

Done

SparkQA · 2020-04-06T14:18:26Z

Test build #120868 has finished for PR 28107 at commit 20febce.

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

AngersZhuuuu · 2020-04-20T08:12:07Z

ping @cloud-fan any more need to update?

cloud-fan · 2020-04-24T08:40:22Z

This PR exposes a more serious problem, that we rely on rules order to resolve HAVING. We can easily get correctness issues because of it.

#28294 proposes a new solution to fix this problem completely, can you take a look?

AngersZhuuuu · 2020-04-24T08:45:15Z

This PR exposes a more serious problem, that we rely on rules order to resolve HAVING. We can easily get correctness issues because of it.

#28294 proposes a new solution to fix this problem completely, can you take a look?

The pr you mentioned is similar like I have said in #28107 (comment)

It's better to to like this, I will notice that pr and check if that pr will solve this pr's problem, if not, I will change based on his pr.

AngersZhuuuu · 2020-05-09T01:33:16Z

@cloud-fan
#28294 can fix this issue. Close this pr.

AngersZhuuuu added 2 commits April 3, 2020 16:24

save

11eb148

Merge branch 'master' into agg-column-same-as-col-and-having-use-it

1cabd08

AngersZhuuuu added 2 commits April 3, 2020 22:22

Update Analyzer.scala

df257df

Update Analyzer.scala

54df9d3

AngersZhuuuu changed the title ~~[SPARK-31334][SQL] TypeCoercion should before then ResolveAggregateFunctions~~ [SPARK-31334][SQL] Don't ResolveReference/ResolveMissingReference when Filter condition with aggregate expression Apr 3, 2020

cloud-fan reviewed Apr 3, 2020

View reviewed changes

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

AngersZhuuuu added 2 commits April 3, 2020 22:44

Update Analyzer.scala

24e9d4d

follow comment

0d81b4d

cloud-fan reviewed Apr 3, 2020

View reviewed changes

AngersZhuuuu added 2 commits April 3, 2020 22:49

Update Analyzer.scala

dfda7ff

Update Analyzer.scala

a5cf877

Update Analyzer.scala

4a799fe

Update Analyzer.scala

5f6eef1

solve UDF problem

cc0b018

add comment

b2855bd

cloud-fan reviewed Apr 6, 2020

View reviewed changes

Update SQLQuerySuite.scala

20febce

AngersZhuuuu closed this May 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-31334][SQL] Don't ResolveReference/ResolveMissingReference when Filter condition with aggregate expression #28107

[SPARK-31334][SQL] Don't ResolveReference/ResolveMissingReference when Filter condition with aggregate expression #28107

AngersZhuuuu commented Apr 3, 2020 •

edited

Loading

AngersZhuuuu commented Apr 3, 2020

cloud-fan commented Apr 3, 2020

AngersZhuuuu commented Apr 3, 2020 •

edited

Loading

AngersZhuuuu commented Apr 3, 2020

cloud-fan commented Apr 3, 2020

cloud-fan Apr 3, 2020

AngersZhuuuu Apr 3, 2020

cloud-fan Apr 3, 2020

AngersZhuuuu Apr 3, 2020

SparkQA commented Apr 3, 2020

SparkQA commented Apr 3, 2020

SparkQA commented Apr 3, 2020

SparkQA commented Apr 3, 2020

SparkQA commented Apr 4, 2020

SparkQA commented Apr 4, 2020

AngersZhuuuu commented Apr 4, 2020

SparkQA commented Apr 4, 2020

AngersZhuuuu commented Apr 4, 2020

SparkQA commented Apr 4, 2020

cloud-fan Apr 6, 2020

AngersZhuuuu Apr 6, 2020

cloud-fan Apr 6, 2020

AngersZhuuuu Apr 6, 2020

cloud-fan Apr 6, 2020

AngersZhuuuu Apr 6, 2020

SparkQA commented Apr 6, 2020

AngersZhuuuu commented Apr 20, 2020 •

edited

Loading

cloud-fan commented Apr 24, 2020

AngersZhuuuu commented Apr 24, 2020

AngersZhuuuu commented May 9, 2020

[SPARK-31334][SQL] Don't ResolveReference/ResolveMissingReference when Filter condition with aggregate expression #28107

[SPARK-31334][SQL] Don't ResolveReference/ResolveMissingReference when Filter condition with aggregate expression #28107

Conversation

AngersZhuuuu commented Apr 3, 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?

AngersZhuuuu commented Apr 3, 2020

cloud-fan commented Apr 3, 2020

AngersZhuuuu commented Apr 3, 2020 • edited Loading

AngersZhuuuu commented Apr 3, 2020

cloud-fan commented Apr 3, 2020

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 Apr 3, 2020

SparkQA commented Apr 3, 2020

SparkQA commented Apr 3, 2020

SparkQA commented Apr 3, 2020

SparkQA commented Apr 4, 2020

SparkQA commented Apr 4, 2020

AngersZhuuuu commented Apr 4, 2020

SparkQA commented Apr 4, 2020

AngersZhuuuu commented Apr 4, 2020

SparkQA commented Apr 4, 2020

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 Apr 6, 2020

AngersZhuuuu commented Apr 20, 2020 • edited Loading

cloud-fan commented Apr 24, 2020

AngersZhuuuu commented Apr 24, 2020

AngersZhuuuu commented May 9, 2020

AngersZhuuuu commented Apr 3, 2020 •

edited

Loading

AngersZhuuuu commented Apr 3, 2020 •

edited

Loading

AngersZhuuuu commented Apr 20, 2020 •

edited

Loading