[SPARK-28228][SQL] Fix substitution order of nested WITH clauses #25029

peter-toth · 2019-07-02T06:52:18Z

What changes were proposed in this pull request?

This PR adds compatibility of handling a WITH clause within another WITH cause. Before this PR these queries retuned 1 while after this PR they return 2 as PostgreSQL does:

WITH
  t AS (SELECT 1),
  t2 AS (
    WITH t AS (SELECT 2)
    SELECT * FROM t
  )
SELECT * FROM t2

WITH t AS (SELECT 1)
SELECT (
  WITH t AS (SELECT 2)
  SELECT * FROM t
)

As this is an incompatible change, the PR introduces the spark.sql.legacy.cte.substitution.enabled flag as an option to restore old behaviour.

How was this patch tested?

Added new UTs.

peter-toth · 2019-07-02T06:54:03Z

This is WIP as it contains changes of #24831

cc @maropu @dongjoon-hyun @gatorsmile @mgaido91

maropu · 2019-07-02T06:57:59Z

Also, you need to update the title.

peter-toth · 2019-07-02T07:03:09Z

Also, you need to update the title.

Thanks, I changed it to "Fix substitution order of nested WITH clauses". I will update the migration guide too a bit later.

SparkQA · 2019-07-02T07:05:02Z

Test build #107104 has finished for PR 25029 at commit 2ef85e5.

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

dongjoon-hyun · 2019-07-02T19:10:17Z

Retest this please.

SparkQA · 2019-07-02T22:18:56Z

Test build #107126 has finished for PR 25029 at commit 2ef85e5.

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

maropu · 2019-07-02T23:51:28Z

retest this please

maropu · 2019-07-03T01:27:17Z

(If this pr gets ready for reviews, I think it'd be better to drop WIP in the title...)

SparkQA · 2019-07-03T03:12:27Z

Test build #107135 has finished for PR 25029 at commit 2ef85e5.

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

peter-toth · 2019-07-03T07:30:36Z

(If this pr gets ready for reviews, I think it'd be better to drop WIP in the title...)

Sure, I will remove WIP if #24831 gets accepted and I can rebase this PR on that.

SparkQA · 2019-07-03T10:57:16Z

Test build #107171 has finished for PR 25029 at commit 26b351e.

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

dongjoon-hyun · 2019-07-04T14:36:16Z

Could you rebase this, @peter-toth ?

peter-toth · 2019-07-04T15:47:15Z

@maropu, @dongjoon-hyun, @mgaido91 I removed the WIP tag and this PR is ready for review now.

SparkQA · 2019-07-04T18:22:22Z

Test build #107239 has finished for PR 25029 at commit ca1c7f0.

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

docs/sql-migration-guide-upgrade.md

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

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

SparkQA · 2019-07-05T22:27:44Z

Test build #107292 has finished for PR 25029 at commit 4a913e8.

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

docs/sql-migration-guide-upgrade.md

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

SparkQA · 2019-07-08T18:27:16Z

Test build #107359 has finished for PR 25029 at commit 73824bf.

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

dongjoon-hyun · 2019-07-10T18:28:58Z

Retest this please.

dongjoon-hyun · 2019-07-10T18:30:46Z

Hi, @gatorsmile .
Could you review this PR?

SparkQA · 2019-07-10T22:18:34Z

Test build #107477 has finished for PR 25029 at commit 73824bf.

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

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

Test build #107512 has finished for PR 25029 at commit 45f0642.

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

SparkQA · 2019-07-11T10:38:37Z

Test build #107518 has finished for PR 25029 at commit 55a01ea.

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

dongjoon-hyun · 2019-07-11T16:30:07Z

Let's use spark.sql.legacy.ctePrecedence.enabled.

[SPARK-28228][SQL] Fix substitution order of nested WITH clauses #25029 (comment)

For the other review comments, it seems that all are addressed. @maropu . Could you review this once more? Or, do you want a more test case?

mgaido91

only some minor comments, LGTM otherwise.

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

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

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

peter-toth

Let's use spark.sql.legacy.ctePrecedence.enabled.

Thanks. I've changed it.

SparkQA · 2019-07-12T13:48:19Z

Test build #107590 has finished for PR 25029 at commit 7d9d96f.

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

dongjoon-hyun

+1, LGTM. Merged to master.
Thank you, @peter-toth , @maropu , @mgaido91 !

peter-toth · 2019-07-12T14:27:20Z

Thanks so much for the review @dongjoon-hyun, @maropu, @mgaido91!

## What changes were proposed in this pull request? This PR adds compatibility of handling a `WITH` clause within another `WITH` cause. Before this PR these queries retuned `1` while after this PR they return `2` as PostgreSQL does: ``` WITH t AS (SELECT 1), t2 AS ( WITH t AS (SELECT 2) SELECT * FROM t ) SELECT * FROM t2 ``` ``` WITH t AS (SELECT 1) SELECT ( WITH t AS (SELECT 2) SELECT * FROM t ) ``` As this is an incompatible change, the PR introduces the `spark.sql.legacy.cte.substitution.enabled` flag as an option to restore old behaviour. ## How was this patch tested? Added new UTs. Closes apache#25029 from peter-toth/SPARK-28228. Authored-by: Peter Toth <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ested WITH clause ### What changes were proposed in this pull request? This is a follow-up for #25029, in this PR we throw an AnalysisException when name conflict is detected in nested WITH clause. In this way, the config `spark.sql.legacy.ctePrecedence.enabled` should be set explicitly for the expected behavior. ### Why are the changes needed? The original change might risky to end-users, it changes behavior silently. ### Does this PR introduce any user-facing change? Yes, change the config `spark.sql.legacy.ctePrecedence.enabled` as optional. ### How was this patch tested? New UT. Closes #27454 from xuanyuanking/SPARK-28228-follow. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

…ested WITH clause ### What changes were proposed in this pull request? This is a follow-up for #25029, in this PR we throw an AnalysisException when name conflict is detected in nested WITH clause. In this way, the config `spark.sql.legacy.ctePrecedence.enabled` should be set explicitly for the expected behavior. ### Why are the changes needed? The original change might risky to end-users, it changes behavior silently. ### Does this PR introduce any user-facing change? Yes, change the config `spark.sql.legacy.ctePrecedence.enabled` as optional. ### How was this patch tested? New UT. Closes #27454 from xuanyuanking/SPARK-28228-follow. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 3db3e39) Signed-off-by: Dongjoon Hyun <[email protected]>

…ested WITH clause ### What changes were proposed in this pull request? This is a follow-up for apache#25029, in this PR we throw an AnalysisException when name conflict is detected in nested WITH clause. In this way, the config `spark.sql.legacy.ctePrecedence.enabled` should be set explicitly for the expected behavior. ### Why are the changes needed? The original change might risky to end-users, it changes behavior silently. ### Does this PR introduce any user-facing change? Yes, change the config `spark.sql.legacy.ctePrecedence.enabled` as optional. ### How was this patch tested? New UT. Closes apache#27454 from xuanyuanking/SPARK-28228-follow. Authored-by: Yuanjian Li <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

cloud-fan · 2020-04-23T13:09:02Z

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

+        // child might contain an inner CTE that has priority so traverse and substitute inner CTEs
+        // in child first
+        val traversedChild: LogicalPlan = child transformExpressions {
+          case e: SubqueryExpression => e.withNewPlan(traverseAndSubstituteCTE(e.plan, true))


The subquery expression seems not correctly handled.

with t1 as (select 1 i) select * from t1 where i in (with t1 as (select 2 i) select * from t1) returns 1 in Spark, but empty row in pgsql.

Yes, it probably should be transformAllExpressions. Will look into it soon...

Oh. Thanks, @cloud-fan .

I've opened #28318 to fix it.

peter-toth mentioned this pull request Jul 2, 2019

[SPARK-19799][SQL] Support WITH clause in subqueries #24831

Closed

peter-toth changed the title ~~[WIP][SPARK-28228][SQL] Better support for WITH clause~~ [WIP][SPARK-28228][SQL] Fix substitution order of nested WITH clauses Jul 2, 2019

dongjoon-hyun added the SQL label Jul 2, 2019

peter-toth force-pushed the SPARK-28228 branch from 26b351e to ca1c7f0 Compare July 4, 2019 15:09

peter-toth changed the title ~~[WIP][SPARK-28228][SQL] Fix substitution order of nested WITH clauses~~ [SPARK-28228][SQL] Fix substitution order of nested WITH clauses Jul 4, 2019

dongjoon-hyun reviewed Jul 4, 2019

View reviewed changes

docs/sql-migration-guide-upgrade.md Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 5, 2019

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 5, 2019

View reviewed changes

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

peter-toth added 2 commits July 5, 2019 21:03

[SPARK-28228][SQL] Better support for WITH clause

8288f02

rename legacy config

4a913e8

peter-toth force-pushed the SPARK-28228 branch from ca1c7f0 to 4a913e8 Compare July 5, 2019 19:17

dongjoon-hyun reviewed Jul 7, 2019

View reviewed changes

docs/sql-migration-guide-upgrade.md Outdated Show resolved Hide resolved

dongjoon-hyun reviewed Jul 7, 2019

View reviewed changes

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

maropu reviewed Jul 7, 2019

View reviewed changes

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

maropu reviewed Jul 7, 2019

View reviewed changes

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

maropu reviewed Jul 7, 2019

View reviewed changes

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

peter-toth mentioned this pull request Jul 8, 2019

[WIP][SPARK-24497][SQL] Support recursive SQL query with adaptive replanning #23531

Closed

peter-toth added 2 commits July 8, 2019 15:41

add example to guide

e1f32bd

fix review findings

73824bf

peter-toth added 2 commits July 11, 2019 08:32

add more comments

45f0642

add comment about inTraverse

55a01ea

mgaido91 reviewed Jul 11, 2019

View reviewed changes

fix review findings

7d9d96f

peter-toth commented Jul 12, 2019

View reviewed changes

dongjoon-hyun approved these changes Jul 12, 2019

View reviewed changes

dongjoon-hyun closed this in 1a26126 Jul 12, 2019

xuanyuanking mentioned this pull request Feb 4, 2020

[SPARK-28228][SQL] Change the default behavior for name conflict in nested WITH clause #27454

Closed

cloud-fan reviewed Apr 23, 2020

View reviewed changes

LantaoJin mentioned this pull request Jul 22, 2020

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

Closed

[SPARK-28228][SQL] Fix substitution order of nested WITH clauses #25029

[SPARK-28228][SQL] Fix substitution order of nested WITH clauses #25029

Uh oh!

Conversation

peter-toth commented Jul 2, 2019

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

peter-toth commented Jul 2, 2019

Uh oh!

maropu commented Jul 2, 2019

Uh oh!

peter-toth commented Jul 2, 2019

Uh oh!

SparkQA commented Jul 2, 2019

Uh oh!

dongjoon-hyun commented Jul 2, 2019

Uh oh!

SparkQA commented Jul 2, 2019

Uh oh!

maropu commented Jul 2, 2019

Uh oh!

maropu commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

peter-toth commented Jul 3, 2019

Uh oh!

SparkQA commented Jul 3, 2019

Uh oh!

dongjoon-hyun commented Jul 4, 2019

Uh oh!

peter-toth commented Jul 4, 2019

Uh oh!

SparkQA commented Jul 4, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jul 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SparkQA commented Jul 8, 2019

Uh oh!

dongjoon-hyun commented Jul 10, 2019

Uh oh!

dongjoon-hyun commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 10, 2019

Uh oh!

SparkQA commented Jul 11, 2019

Uh oh!

SparkQA commented Jul 11, 2019

Uh oh!

dongjoon-hyun commented Jul 11, 2019

Uh oh!

mgaido91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

peter-toth left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 12, 2019

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

peter-toth commented Jul 12, 2019

Uh oh!

cloud-fan Apr 23, 2020

Choose a reason for hiding this comment