Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
linhongliu-db wants to merge 7 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
52fe1f3
SPARK-38318: Skip view cyclic reference check when storeAnalyzedPlan …
linhongliu-db Feb 24, 2022
66926e9
clean
linhongliu-db Feb 24, 2022
3fb3312
trigger test
linhongliu-db Feb 24, 2022
a8a2725
address comments
linhongliu-db Feb 28, 2022
f413315
Update SQLViewTestSuite.scala
linhongliu-db Feb 28, 2022
c6ff3e1
Update SQLViewTestSuite.scala
linhongliu-db Feb 28, 2022
3238c3d
address comments
linhongliu-db Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/command/views.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,12 +612,17 @@ object ViewHelper extends SQLConfHelper with Logging {
val uncache = getRawTempView(name.table).map { r =>
needsToUncache(r, aliasedPlan)
}.getOrElse(false)
val storeAnalyzedPlanForView = conf.storeAnalyzedPlanForView || originalText.isEmpty
if (replace && uncache) {
logDebug(s"Try to uncache ${name.quotedString} before replacing.")
checkCyclicViewReference(analyzedPlan, Seq(name), name)
Copy link
Contributor

@jackylee-ch jackylee-ch Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@cloud-fan cloud-fan Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@linhongliu-db linhongliu-db Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jackylee-ch jackylee-ch Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jackylee-ch jackylee-ch Feb 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@linhongliu-db linhongliu-db Feb 28, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

@jackylee-ch jackylee-ch Mar 1, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (!storeAnalyzedPlanForView) {
// Skip cyclic check because when stored analyzed plan for view, the depended
// view is already converted to the underlying tables. So no cyclic views.
checkCyclicViewReference(analyzedPlan, Seq(name), name)
}
CommandUtils.uncacheTableOrView(session, name.quotedString)
}
if (!conf.storeAnalyzedPlanForView && originalText.nonEmpty) {
if (!storeAnalyzedPlanForView) {
TemporaryViewRelation(
prepareTemporaryView(
name,
Expand Down
33 changes: 32 additions & 1 deletion sql/core/src/test/scala/org/apache/spark/sql/execution/SQLViewTestSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package org.apache.spark.sql.execution

import scala.collection.JavaConverters._

import org.apache.spark.sql.{AnalysisException, QueryTest, Row}
import org.apache.spark.sql.{AnalysisException, DataFrame, QueryTest, Row}
import org.apache.spark.sql.catalyst.{FunctionIdentifier, TableIdentifier}
import org.apache.spark.sql.catalyst.catalog.CatalogFunction
import org.apache.spark.sql.catalyst.expressions.Expression
Expand Down Expand Up @@ -408,6 +408,9 @@ abstract class SQLViewTestSuite extends QueryTest with SQLTestUtils {
}

abstract class TempViewTestSuite extends SQLViewTestSuite {

def createOrReplaceDatasetView(df: DataFrame, viewName: String): Unit

test("SPARK-37202: temp view should capture the function registered by catalog API") {
val funcName = "tempFunc"
withUserDefinedFunction(funcName -> true) {
Expand Down Expand Up @@ -435,6 +438,28 @@ abstract class TempViewTestSuite extends SQLViewTestSuite {
s"$viewName is a temp view. 'SHOW CREATE TABLE' expects a table or permanent view."))
}
}

test("back compatibility: skip cyclic reference check if view is stored as logical plan") {
val viewName = formattedViewName("v")
withSQLConf(STORE_ANALYZED_PLAN_FOR_VIEW.key -> "false") {
withView(viewName) {
createOrReplaceDatasetView(sql("SELECT 1"), "v")
createOrReplaceDatasetView(sql(s"SELECT * FROM $viewName"), "v")
checkViewOutput(viewName, Seq(Row(1)))
}
}
withSQLConf(STORE_ANALYZED_PLAN_FOR_VIEW.key -> "true") {
withView(viewName) {
createOrReplaceDatasetView(sql("SELECT 1"), "v")
createOrReplaceDatasetView(sql(s"SELECT * FROM $viewName"), "v")
checkViewOutput(viewName, Seq(Row(1)))

createView("v", "SELECT 2", replace = true)
createView("v", s"SELECT * FROM $viewName", replace = true)
checkViewOutput(viewName, Seq(Row(2)))
}
}
}
}

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

class GlobalTempViewTestSuite extends TempViewTestSuite with SharedSparkSession {
Expand All @@ -454,6 +482,9 @@ class GlobalTempViewTestSuite extends TempViewTestSuite with SharedSparkSession
override protected def tableIdentifier(viewName: String): TableIdentifier = {
TableIdentifier(viewName, Some(db))
}
override def createOrReplaceDatasetView(df: DataFrame, viewName: String): Unit = {
df.createOrReplaceGlobalTempView(viewName)
}
}

class OneTableCatalog extends InMemoryCatalog {
Expand Down