[SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan #35653

linhongliu-db · 2022-02-24T17:49:51Z

What changes were proposed in this pull request?

In 3.2, we unified the representation of dataset view and SQL view, i.e., we wrap both
of them with View. This causes a regression that below case works in 3.1 but failed in 3.2

sql("select 1").createOrReplaceTempView("v")
sql("select * from v").createOrReplaceTempView("v")
-- in 3.1 it works well, and select will output 1
-- in 3.2 it failed with error: "AnalysisException: Recursive view v detected (cycle: v -> v)"

The root cause is in 3.1 we actually never did view cyclic check for dataset view. Because they
are wrapped by SubqueryAlias instead of View
In this PR, we want to skip the cyclic check if the view is stored as a logical plan.
i.e., storeAnalyzedPlanForView = true or view is created by Dataset API.

Why are the changes needed?

fix regression

Does this PR introduce any user-facing change?

No

How was this patch tested?

newly added ut

…= true

linhongliu-db · 2022-02-24T18:01:20Z

cc @cloud-fan

cloud-fan · 2022-02-25T04:34:46Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

@@ -614,7 +614,11 @@ object ViewHelper extends SQLConfHelper with Logging {
    }.getOrElse(false)
    if (replace && uncache) {
      logDebug(s"Try to uncache ${name.quotedString} before replacing.")
-      checkCyclicViewReference(analyzedPlan, Seq(name), name)
+      if (!conf.storeAnalyzedPlanForView) {


how about !conf.storeAnalyzedPlanForView && originalText.nonEmpty? It also matches the if condition below where we decide to store analyzed plan.

Sure, I just want to be conservative in the first version. Because !conf.storeAnalyzedPlanForView && originalText.nonEmpty means dataset view will always skip cyclic check. It's consistent with 3.1 which is good. But I thought we should always block recursive views unless the user explicitly changes the flag (even if the recursive views work well).

jackylee-ch · 2022-02-28T12:41:13Z

sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala

@@ -443,6 +468,9 @@ class LocalTempViewTestSuite extends TempViewTestSuite with SharedSparkSession {
  override protected def tableIdentifier(viewName: String): TableIdentifier = {
    TableIdentifier(viewName)
  }
+  override def createDatasetView(df: DataFrame, viewName: String): Unit = {


nit: use createOrReplaceDatasetView instead of createDatasetView.

jackylee-ch · 2022-02-28T12:58:04Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

@@ -614,7 +614,11 @@ object ViewHelper extends SQLConfHelper with Logging {
    }.getOrElse(false)
    if (replace && uncache) {
      logDebug(s"Try to uncache ${name.quotedString} before replacing.")
-      checkCyclicViewReference(analyzedPlan, Seq(name), name)


Maybe removing this check and removing the SubqueryAlias and View from analyzedPlan is a better idea for this condition. Same with here.
However, it depends on whether we support user to repalce view in this way.

I don't think we want to support it officially. It's for backward compatibility only.

I didn't get this. If we remove the cyclic check, how to detect a recursive view created by SQL.

I didn't get this. If we remove the cyclic check, how to detect a recursive view created by SQL.

Oh, I got it, thanks.

I don't think we want to support it officially. It's for backward compatibility only.

If we won't suport this, then why we change it for backward compatibility? It is not a valid behavior. What about leading people to use different name?

@stczwd, the community did a lot of work in view implementation in 3.1 and 3.2. The view's behavior makes more sense now. But it's also a little bit more confusing now because we have several kinds of view implementations now.

Permanent view created by SQL DDL

Temp view created by SQL DDL (from 3.1, this view now is represented with SQL text)

Temp view created by SQL DDL with storeAnalyzedPlanForView = true (behavior before 3.1, same with view 4)

Temp view created by Dataset API (until now, this view is still represented with analyzed logical plan)

To answer your question

If we won't suport this, then why we change it for backward compatibility?

We don't support recursive view for view created by SQL DDL, but we DO support this for view created by dataset API.
That's why we add this condition here: !conf.storeAnalyzedPlanForView && originalText.nonEmpty.
It means only check the cyclic reference for view 1 and 2

We don't support recursive view for view created by SQL DDL, but we DO support this for view created by dataset API.

Ah, I see, thanks for the explanation.

cloud-fan · 2022-02-28T13:27:28Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala

@@ -614,7 +614,11 @@ object ViewHelper extends SQLConfHelper with Logging {
    }.getOrElse(false)
    if (replace && uncache) {
      logDebug(s"Try to uncache ${name.quotedString} before replacing.")
-      checkCyclicViewReference(analyzedPlan, Seq(name), name)
+      if (!conf.storeAnalyzedPlanForView && originalText.nonEmpty) {


maybe it's better to have a val storeAnalyzedPlanForView = conf.storeAnalyzedPlanForView || originalText.nonEmpty at the beginning, then we can just write if (!storeAnalyzedPlanForView) here and below.

cloud-fan · 2022-03-01T07:08:37Z

thanks, merging to master/3.2!

… as logical plan In 3.2, we unified the representation of dataset view and SQL view, i.e., we wrap both of them with `View`. This causes a regression that below case works in 3.1 but failed in 3.2 ```sql sql("select 1").createOrReplaceTempView("v") sql("select * from v").createOrReplaceTempView("v") -- in 3.1 it works well, and select will output 1 -- in 3.2 it failed with error: "AnalysisException: Recursive view v detected (cycle: v -> v)" ``` The root cause is in 3.1 we actually never did view cyclic check for dataset view. Because they are wrapped by `SubqueryAlias` instead of `View` In this PR, we want to skip the cyclic check if the view is stored as a logical plan. i.e., `storeAnalyzedPlanForView = true` or view is created by Dataset API. fix regression No newly added ut Closes #35653 from linhongliu-db/SPARK-38318. Authored-by: Linhong Liu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 1d068ce) Signed-off-by: Wenchen Fan <[email protected]>

ydh1000 · 2022-04-14T04:26:49Z

thanks for fixing it. do you have a new build?

… as logical plan In 3.2, we unified the representation of dataset view and SQL view, i.e., we wrap both of them with `View`. This causes a regression that below case works in 3.1 but failed in 3.2 ```sql sql("select 1").createOrReplaceTempView("v") sql("select * from v").createOrReplaceTempView("v") -- in 3.1 it works well, and select will output 1 -- in 3.2 it failed with error: "AnalysisException: Recursive view v detected (cycle: v -> v)" ``` The root cause is in 3.1 we actually never did view cyclic check for dataset view. Because they are wrapped by `SubqueryAlias` instead of `View` In this PR, we want to skip the cyclic check if the view is stored as a logical plan. i.e., `storeAnalyzedPlanForView = true` or view is created by Dataset API. fix regression No newly added ut Closes apache#35653 from linhongliu-db/SPARK-38318. Authored-by: Linhong Liu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 1d068ce) Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit fb1218d) Signed-off-by: Dongjoon Hyun <[email protected]>

… as logical plan In 3.2, we unified the representation of dataset view and SQL view, i.e., we wrap both of them with `View`. This causes a regression that below case works in 3.1 but failed in 3.2 ```sql sql("select 1").createOrReplaceTempView("v") sql("select * from v").createOrReplaceTempView("v") -- in 3.1 it works well, and select will output 1 -- in 3.2 it failed with error: "AnalysisException: Recursive view v detected (cycle: v -> v)" ``` The root cause is in 3.1 we actually never did view cyclic check for dataset view. Because they are wrapped by `SubqueryAlias` instead of `View` In this PR, we want to skip the cyclic check if the view is stored as a logical plan. i.e., `storeAnalyzedPlanForView = true` or view is created by Dataset API. fix regression No newly added ut Closes apache#35653 from linhongliu-db/SPARK-38318. Authored-by: Linhong Liu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 1d068ce) Signed-off-by: Wenchen Fan <[email protected]>

SPARK-38318: Skip view cyclic reference check when storeAnalyzedPlan …

52fe1f3

…= true

github-actions bot added the SQL label Feb 24, 2022

linhongliu-db added 2 commits February 25, 2022 01:51

clean

66926e9

trigger test

3fb3312

cloud-fan reviewed Feb 25, 2022

View reviewed changes

linhongliu-db changed the title ~~[SPARK-38318][SQL] Skip view cyclic reference check when storeAnalyzedPlan = true~~ [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan Feb 28, 2022

address comments

a8a2725

jackylee-ch reviewed Feb 28, 2022

View reviewed changes

linhongliu-db added 2 commits February 28, 2022 21:24

Update SQLViewTestSuite.scala

f413315

Update SQLViewTestSuite.scala

c6ff3e1

cloud-fan reviewed Feb 28, 2022

View reviewed changes

address comments

3238c3d

cloud-fan closed this in 1d068ce Mar 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan #35653

[SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan #35653

linhongliu-db commented Feb 24, 2022 •

edited

Loading

linhongliu-db commented Feb 24, 2022

cloud-fan Feb 25, 2022 •

edited

Loading

linhongliu-db Feb 28, 2022

jackylee-ch Feb 28, 2022

jackylee-ch Feb 28, 2022

cloud-fan Feb 28, 2022

linhongliu-db Feb 28, 2022

jackylee-ch Feb 28, 2022 •

edited

Loading

jackylee-ch Feb 28, 2022

linhongliu-db Feb 28, 2022 •

edited

Loading

jackylee-ch Mar 1, 2022

cloud-fan Feb 28, 2022

cloud-fan commented Mar 1, 2022

ydh1000 commented Apr 14, 2022

[SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan #35653

[SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan #35653

Conversation

linhongliu-db commented Feb 24, 2022 • 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?

linhongliu-db commented Feb 24, 2022

cloud-fan Feb 25, 2022 • 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

Choose a reason for hiding this comment

jackylee-ch Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linhongliu-db Feb 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Mar 1, 2022

ydh1000 commented Apr 14, 2022

linhongliu-db commented Feb 24, 2022 •

edited

Loading

cloud-fan Feb 25, 2022 •

edited

Loading

jackylee-ch Feb 28, 2022 •

edited

Loading

linhongliu-db Feb 28, 2022 •

edited

Loading