You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/24 17:50:03 UTC
[GitHub] [spark] linhongliu-db opened a new pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check when storeAnalyzedPlan = true
linhongliu-db opened a new pull request #35653:
URL: https://github.com/apache/spark/pull/35653
### 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
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, we want to skip the cyclic check if the legacy conf `storeAnalyzedPlan` is set. So, we
can keep the legacy use cases working and not affect the new view behavior.
### Why are the changes needed?
fix regression
### Does this PR introduce _any_ user-facing change?
fix regression
### How was this patch tested?
newly added ut
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on pull request #35653:
URL: https://github.com/apache/spark/pull/35653#issuecomment-1055096584
thanks, merging to master/3.2!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] linhongliu-db commented on pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check when storeAnalyzedPlan = true
Posted by GitBox <gi...@apache.org>.
linhongliu-db commented on pull request #35653:
URL: https://github.com/apache/spark/pull/35653#issuecomment-1050116618
cc @cloud-fan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] linhongliu-db commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
linhongliu-db commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815967083
##########
File path: 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)
Review comment:
@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.
1. Permanent view created by SQL DDL
2. Temp view created by SQL DDL (from 3.1, this view now is represented with SQL text)
3. Temp view created by SQL DDL with storeAnalyzedPlanForView = true (behavior before 3.1, same with view 4)
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 3 and 4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815866254
##########
File path: 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)
Review comment:
Maybe removing this check and removing the SubqueryAlias and View from analyzedPlan is a better idea for this condition. Same with [here](https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala#L149).
However, it depends on whether we support user to repalce view in this way.
##########
File path: 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 = {
Review comment:
nit: use `createOrReplaceDatasetView` instead of `createDatasetView`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r816400053
##########
File path: 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)
Review comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] linhongliu-db commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
linhongliu-db commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815887710
##########
File path: 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)
Review comment:
I didn't get this. If we remove the cyclic check, how to detect a recursive view created by SQL.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] linhongliu-db commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
linhongliu-db commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815967083
##########
File path: 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)
Review comment:
@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.
1. Permanent view created by SQL DDL
2. Temp view created by SQL DDL (from 3.1, this view now is represented with SQL text)
3. Temp view created by SQL DDL with storeAnalyzedPlanForView = true (behavior before 3.1, same with #4)
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 #3 and #4
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] linhongliu-db commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
linhongliu-db commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815967083
##########
File path: 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)
Review comment:
@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.
1. Permanent view created by SQL DDL
2. Temp view created by SQL DDL (from 3.1, this view now is represented with SQL text)
3. Temp view created by SQL DDL with storeAnalyzedPlanForView = true (behavior before 3.1, same with view 4)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815918524
##########
File path: 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)
Review comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815886958
##########
File path: 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) {
Review comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815886132
##########
File path: 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)
Review comment:
I don't think we want to support it officially. It's for backward compatibility only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] linhongliu-db commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
linhongliu-db commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815554548
##########
File path: 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) {
Review comment:
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).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan closed pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
cloud-fan closed pull request #35653:
URL: https://github.com/apache/spark/pull/35653
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815918524
##########
File path: 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)
Review comment:
> I didn't get this. If we remove the cyclic check, how to detect a recursive view created by SQL.
It seems not necessary to check cyclic since we want to replace a view. The view in analyzedPlan is an old view, not the new view, even if they have the same name, thus I think maybe remove is better.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] stczwd commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check if view is stored as logical plan
Posted by GitBox <gi...@apache.org>.
stczwd commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r815921265
##########
File path: 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)
Review comment:
> 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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a change in pull request #35653: [SPARK-38318][SQL] Skip view cyclic reference check when storeAnalyzedPlan = true
Posted by GitBox <gi...@apache.org>.
cloud-fan commented on a change in pull request #35653:
URL: https://github.com/apache/spark/pull/35653#discussion_r814468315
##########
File path: 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) {
Review comment:
how about `!conf.storeAnalyzedPlanForView && originalText.nonEmpty`? It also matches the condition below where we decide to store analyzed plan.
##########
File path: 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) {
Review comment:
how about `!conf.storeAnalyzedPlanForView && originalText.nonEmpty`? It also matches the if condition below where we decide to store analyzed plan.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org