-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-29628][SQL] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view #26317
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.
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
[SPARK-29628][SQL] Forcibly create a temporary view in CREATE VIEW if referencing a temporary view #26317
Changes from all commits
6e804e2
ff1174c
cd2833c
4e5228c
d04292b
937da50
18dad1c
3b3aa1d
5d11ddf
cf1b5ee
3f12e09
87557e0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ import org.apache.spark.sql.catalyst.catalog.{CatalogStorageFormat, CatalogTable | |
| import org.apache.spark.sql.catalyst.expressions.{Alias, SubqueryExpression} | ||
| import org.apache.spark.sql.catalyst.plans.QueryPlan | ||
| import org.apache.spark.sql.catalyst.plans.logical.{LogicalPlan, Project, View} | ||
| import org.apache.spark.sql.types.{MetadataBuilder, StructType} | ||
| import org.apache.spark.sql.types.MetadataBuilder | ||
| import org.apache.spark.sql.util.SchemaUtils | ||
|
|
||
|
|
||
|
|
@@ -110,19 +110,25 @@ case class CreateViewCommand( | |
|
|
||
| private def isTemporary = viewType == LocalTempView || viewType == GlobalTempView | ||
|
|
||
| // Disallows 'CREATE TEMPORARY VIEW IF NOT EXISTS' to be consistent with 'CREATE TEMPORARY TABLE' | ||
| if (allowExisting && isTemporary) { | ||
| throw new AnalysisException( | ||
| "It is not allowed to define a TEMPORARY view with IF NOT EXISTS.") | ||
| } | ||
| if (isTemporary) verifyTempView() | ||
|
|
||
| private def verifyTempView(): Unit = { | ||
| // Disallows 'CREATE TEMPORARY VIEW IF NOT EXISTS' | ||
| // to be consistent with 'CREATE TEMPORARY TABLE' | ||
| if (allowExisting) { | ||
srowen marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| throw new AnalysisException( | ||
| "It is not allowed to define a TEMPORARY view with IF NOT EXISTS.") | ||
| } | ||
|
|
||
| // Temporary view names should NOT contain database prefix like "database.table" | ||
| if (isTemporary && name.database.isDefined) { | ||
| val database = name.database.get | ||
| throw new AnalysisException( | ||
| s"It is not allowed to add database prefix `$database` for the TEMPORARY view name.") | ||
| // Temporary view names should NOT contain database prefix like "database.table" | ||
| name.database.foreach { | ||
| database => throw new AnalysisException( | ||
| s"It is not allowed to add database prefix `$database` for the TEMPORARY view name.") | ||
| } | ||
| } | ||
|
|
||
| private var isTempReferred = false | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure about the semantics of this. It's used in a different place from where it's checked. Is this always set to true by a code path before it needs to be used?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes that's not quite my question. How do we know the code path that sets this to true below will execute first?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In |
||
|
|
||
| override def run(sparkSession: SparkSession): Seq[Row] = { | ||
| // If the plan cannot be analyzed, throw an exception and don't proceed. | ||
| val qe = sparkSession.sessionState.executePlan(child) | ||
|
|
@@ -136,12 +142,11 @@ case class CreateViewCommand( | |
| s"specified by CREATE VIEW (num: `${userSpecifiedColumns.length}`).") | ||
| } | ||
|
|
||
| // When creating a permanent view, not allowed to reference temporary objects. | ||
| // This should be called after `qe.assertAnalyzed()` (i.e., `child` can be resolved) | ||
| verifyTemporaryObjectsNotExists(sparkSession) | ||
|
|
||
| val catalog = sparkSession.sessionState.catalog | ||
| if (viewType == LocalTempView) { | ||
| if (viewType == LocalTempView || isTempReferred) { | ||
| val aliasedPlan = aliasPlan(sparkSession, analyzedPlan) | ||
| catalog.createTempView(name.table, aliasedPlan, overrideIfExists = replace) | ||
| } else if (viewType == GlobalTempView) { | ||
|
|
@@ -178,7 +183,8 @@ case class CreateViewCommand( | |
| } | ||
|
|
||
| /** | ||
| * Permanent views are not allowed to reference temp objects, including temp function and views | ||
| * Permanent views are not allowed to reference temp function. When permanent view | ||
| * has a reference of temp view, it will be created as temp view [SPARK-29628]. | ||
| */ | ||
| private def verifyTemporaryObjectsNotExists(sparkSession: SparkSession): Unit = { | ||
| import sparkSession.sessionState.analyzer.AsTableIdentifier | ||
|
|
@@ -192,12 +198,20 @@ case class CreateViewCommand( | |
| // package (e.g., HiveGenericUDF). | ||
| def verify(child: LogicalPlan) { | ||
| child.collect { | ||
| // Disallow creating permanent views based on temporary views. | ||
| // Permanent views will be created as temporary view if based on temporary view. | ||
| case UnresolvedRelation(AsTableIdentifier(ident)) | ||
| if sparkSession.sessionState.catalog.isTemporaryTable(ident) => | ||
| // temporary views are only stored in the session catalog | ||
| throw new AnalysisException(s"Not allowed to create a permanent view $name by " + | ||
| s"referencing a temporary view $ident") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can improve the error message and suggest end users to add the keyword TEMPORARY to create a temporary view instead.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. PR for above suggestion #26731
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gatorsmile also review #26712 |
||
| // Temporary views are only stored in the session catalog | ||
| if (sparkSession.sqlContext.conf.usePostgreSQLDialect) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We are removing this dialect as we discussed in the mailing list. |
||
| logInfo(s"View $name is based on temporary view $ident." | ||
| + s" $name will be created as temporary view") | ||
| verifyTempView() | ||
| isTempReferred = true | ||
| } else { | ||
| throw new AnalysisException(s"Not allowed to create a permanent view $name by " + | ||
| s"referencing a temporary view $ident") | ||
| } | ||
|
|
||
| case other if !other.resolved => other.expressions.flatMap(_.collect { | ||
| // Traverse subquery plan for any unresolved relations. | ||
| case e: SubqueryExpression => verify(e.plan) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.