[SPARK-32237][SQL] Resolve hint in CTE #29062

LantaoJin · 2020-07-10T04:36:58Z

What changes were proposed in this pull request?

This PR is to move Substitution rule before Hints rule in Analyzer to avoid hint in CTE not working.

Why are the changes needed?

Below SQL in Spark3.0 will throw AnalysisException, but it works in Spark2.x

WITH cte AS (SELECT /*+ REPARTITION(3) */ T.id, T.data FROM $t1 T)
SELECT cte.id, cte.data FROM cte

Failed to analyze query: org.apache.spark.sql.AnalysisException: cannot resolve '`cte.id`' given input columns: [cte.data, cte.id]; line 3 pos 7;
'Project ['cte.id, 'cte.data]
+- SubqueryAlias cte
   +- Project [id#21L, data#22]
      +- SubqueryAlias T
         +- SubqueryAlias testcat.ns1.ns2.tbl
            +- RelationV2[id#21L, data#22] testcat.ns1.ns2.tbl

'Project ['cte.id, 'cte.data]
+- SubqueryAlias cte
   +- Project [id#21L, data#22]
      +- SubqueryAlias T
         +- SubqueryAlias testcat.ns1.ns2.tbl
            +- RelationV2[id#21L, data#22] testcat.ns1.ns2.tbl

Does this PR introduce any user-facing change?

No

How was this patch tested?

Add a unit test

sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2SQLSuite.scala

TJX2014 · 2020-07-10T06:59:39Z

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

  lazy val batches: Seq[Batch] = Seq(
+    Batch("Substitution", fixedPoint,
+      CTESubstitution,
+      WindowsSubstitution,
+      EliminateUnions,
+      new SubstituteUnresolvedOrdinals(conf)),
    Batch("Disable Hints", Once,
      new ResolveHints.DisableHints(conf)),
    Batch("Hints", fixedPoint,
      new ResolveHints.ResolveJoinStrategyHints(conf),
      new ResolveHints.ResolveCoalesceHints(conf)),
    Batch("Simple Sanity Check", Once,
      LookupFunctions),
-    Batch("Substitution", fixedPoint,
-      CTESubstitution,
-      WindowsSubstitution,
-      EliminateUnions,
-      new SubstituteUnresolvedOrdinals(conf)),


Seems cool : )

SparkQA · 2020-07-10T07:05:01Z

Test build #125553 has finished for PR 29062 at commit b183cba.

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

SparkQA · 2020-07-10T07:46:28Z

Test build #125579 has started for PR 29062 at commit adc2b59.

maropu · 2020-07-10T22:27:16Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

      Seq("Window function row_number() requires an OVER clause."))
  }
+
+  test("SPARK-32237: Hint in CTE") {


Could you add end-2-end tests, too, e.g., in SQLQueryTestSuite or SQLQuerySuite?

maropu · 2020-07-10T22:27:44Z

cc: @cloud-fan @viirya

SparkQA · 2020-07-11T07:05:01Z

Test build #125662 has finished for PR 29062 at commit e361401.

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

LantaoJin · 2020-07-11T08:18:33Z

retest this please

SparkQA · 2020-07-11T13:51:53Z

Test build #125674 has finished for PR 29062 at commit e361401.

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

viirya · 2020-07-11T22:13:10Z

Can you keep the original PR template? Like "What changes were proposed in this pull request?" was removed. Please describe why the issue happened and how this fixes it in the PR description.

LantaoJin · 2020-07-12T00:47:51Z

Can you keep the original PR template? Like "What changes were proposed in this pull request?" was removed. Please describe why the issue happened and how this fixes it in the PR description.

Done

viirya · 2020-07-12T06:25:39Z

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

+      CTESubstitution,
+      WindowsSubstitution,
+      EliminateUnions,
+      new SubstituteUnresolvedOrdinals(conf)),


I checked the order of analysis rules in branch-2.4. Substitution batch is after Hints too, like branch-3.0. Is there any change causing this issue?

It's hard to find which change causing it for me since 2.4 and 3.0 have many differences that I don't know.

cloud-fan · 2020-07-21T14:07:18Z

the patch itself looks fine.

AnalysisException: cannot resolve 'cte.id' given input columns: [cte.data, cte.id];

Do you know why? I expect to see we just ignore the hint, but it reports "can't resolve column", which is pretty weird.

LantaoJin · 2020-07-22T06:47:21Z

PS:

    val plan = With(
      Project(
        Seq(UnresolvedAttribute("cte.a")),
        UnresolvedRelation(TableIdentifier("cte"))
      ),
      Seq(
        (
          "cte",
          SubqueryAlias(
            AliasIdentifier("cte"),
            Project(testRelation.output, testRelation)
          )
        )
      )
    )
    assertAnalysisSuccess(plan)

above test case without hint only works in v3.0.0. So the test case I given both fail in v3.0.0 and v2.4.6

spark/sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/AnalysisSuite.scala

Line 899 in e361401

test("SPARK-32237: Hint in CTE") {

LantaoJin · 2020-07-22T09:43:24Z

WITH cte AS (SELECT /*+ REPARTITION(3) */ * FROM t) SELECT * FROM cte

throws java.lang.IllegalStateException: Internal error: logical hint operator should have been removed during analysis at #25029, but throws AnalysisException: cannot resolve at #27454 (These two PRs are to fix one issue SPARK-28228). But between them, there are so many PRs. I can't tell which one lead to this change. @cloud-fan

cloud-fan · 2020-07-22T16:57:01Z

The patch LGTM. It will be great if you can take a look at 2.4 and see if there is an easy fix to make your tests pass. We did a big refactor about CTE substitution in 3.0 and I don't know how hard it is to fix the bug in 2.4.

dongjoon-hyun · 2020-07-23T00:56:05Z

cc @peter-toth , too.

cloud-fan · 2020-07-23T03:12:25Z

merged to master, thanks!

@LantaoJin can you send a backport to 3.0?

LantaoJin · 2020-07-23T04:15:20Z

Thanks. I will file a backport PR.

### What changes were proposed in this pull request? The backport of #29062 This PR is to move `Substitution` rule before `Hints` rule in `Analyzer` to avoid hint in CTE not working. ### Why are the changes needed? Below SQL in Spark3.0 will throw AnalysisException, but it works in Spark2.x ```sql WITH cte AS (SELECT /*+ REPARTITION(3) */ T.id, T.data FROM $t1 T) SELECT cte.id, cte.data FROM cte ``` ``` Failed to analyze query: org.apache.spark.sql.AnalysisException: cannot resolve '`cte.id`' given input columns: [cte.data, cte.id]; line 3 pos 7; 'Project ['cte.id, 'cte.data] +- SubqueryAlias cte +- Project [id#21L, data#22] +- SubqueryAlias T +- SubqueryAlias testcat.ns1.ns2.tbl +- RelationV2[id#21L, data#22] testcat.ns1.ns2.tbl 'Project ['cte.id, 'cte.data] +- SubqueryAlias cte +- Project [id#21L, data#22] +- SubqueryAlias T +- SubqueryAlias testcat.ns1.ns2.tbl +- RelationV2[id#21L, data#22] testcat.ns1.ns2.tbl ``` ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add a unit test Closes #29201 from LantaoJin/SPARK-32237_branch-3.0. Lead-authored-by: LantaoJin <[email protected]> Co-authored-by: Alan Jin <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

gatorsmile · 2020-07-30T20:13:50Z

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

  val postHocResolutionRules: Seq[Rule[LogicalPlan]] = Nil

  lazy val batches: Seq[Batch] = Seq(
+    Batch("Substitution", fixedPoint,


Why we have to execute the batch Substitution before the batch ResolveHints? What is the root cause? I am unable to tell it from either PR description or the code comments. Could any of you explain it?

cc @maryannxue

I don't know which PR causes this regression problem since there is a big gap between 2.4 and 3.0 about CTE. I am not familiar with this part.
I fixed this by debuging. In 3.0, I found only the child of CTE could enter into the rule of ResolveCoalesceHints. For the above test case:

CTE [cte] : +- 'SubqueryAlias `cte` : +- 'UnresolvedHint REPARTITION, [3] : +- 'Project [*] : +- 'UnresolvedRelation `t` +- 'Project [*] +- 'UnresolvedRelation `cte`

Only this child Project branch could enter into the rule of ResolveCoalesceHints

+- 'Project [*] +- 'UnresolvedRelation `cte`

gatorsmile · 2020-07-30T20:18:50Z

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

+      sql("CREATE TABLE t USING PARQUET AS SELECT 1 AS id")
+      checkAnswer(
+        sql(s"""
+          |WITH cte AS (SELECT /*+ REPARTITION(3) */ * FROM t)


as long as we have a hint in CTE, it will fail in 3.0? can you add more test cases?

Yes. The same issue reported by multiple times in Jira with different use cases, like SPARK-32237, SPARK-32347, SPARK-32535, I will file a new PR to add more unit tests.

gatorsmile · 2020-07-30T20:20:12Z

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

    }
  }
+
+  test("SPARK-32237: Hint in CTE") {


Like JoinHintSuite, we should have our own hint-specific suite. It can help us understand the test coverage of SQL HINT.

Sure. I will add a new suite.

### What changes were proposed in this pull request? Add a new test suite `CTEHintSuite` ### Why are the changes needed? This ticket is to address the below comments to help us understand the test coverage of SQL HINT for CTE. #29062 (comment) #29062 (comment) ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? Add a test suite. Closes #29359 from LantaoJin/SPARK-32537. Authored-by: LantaoJin <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

[SPARK-32237][SQL] Resolve hint in CTE

b183cba

probot-autolabeler bot added the SQL label Jul 10, 2020

maropu reviewed Jul 10, 2020

View reviewed changes

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

maropu reviewed Jul 10, 2020

View reviewed changes

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

TJX2014 reviewed Jul 10, 2020

View reviewed changes

refactor unit test

adc2b59

maropu reviewed Jul 10, 2020

View reviewed changes

add a end-2-end test

e361401

viirya reviewed Jul 12, 2020

View reviewed changes

TJX2014 mentioned this pull request Jul 19, 2020

[SPARK-32347][SQL] Hint in CTE should be resolved in Hints batch rule #29156

Closed

cloud-fan closed this in 182566b Jul 23, 2020

LantaoJin mentioned this pull request Jul 23, 2020

[SPARK-32237][SQL][3.0] Resolve hint in CTE #29201

Closed

gatorsmile reviewed Jul 30, 2020

View reviewed changes

LantaoJin mentioned this pull request Aug 5, 2020

[SPARK-32537][SQL][TEST] Add a CTEHintSuite for test coverage #29359

Closed

[SPARK-32237][SQL] Resolve hint in CTE #29062

[SPARK-32237][SQL] Resolve hint in CTE #29062

Uh oh!

Conversation

LantaoJin commented Jul 10, 2020 • edited by dongjoon-hyun Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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?

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maropu commented Jul 10, 2020

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

LantaoJin commented Jul 11, 2020

Uh oh!

SparkQA commented Jul 11, 2020

Uh oh!

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

Uh oh!

Uh oh!

LantaoJin commented Jul 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jul 21, 2020

Uh oh!

LantaoJin commented Jul 22, 2020

Uh oh!

LantaoJin commented Jul 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cloud-fan commented Jul 22, 2020

Uh oh!

dongjoon-hyun commented Jul 23, 2020

Uh oh!

cloud-fan commented Jul 23, 2020

Uh oh!

LantaoJin commented Jul 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LantaoJin Aug 5, 2020 • 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!

LantaoJin Aug 5, 2020 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

LantaoJin commented Jul 10, 2020 •

edited by dongjoon-hyun

Loading

viirya commented Jul 11, 2020 •

edited

Loading

LantaoJin commented Jul 22, 2020 •

edited

Loading

LantaoJin Aug 5, 2020 •

edited

Loading

LantaoJin Aug 5, 2020 •

edited

Loading