Skip to content

Conversation

@imback82
Copy link
Contributor

@imback82 imback82 commented Jul 15, 2020

What changes were proposed in this pull request?

This PR proposes to disallow SHOW TBLPROPERTIES on a temporary view as discussed here: #28375 (comment).

Why are the changes needed?

Creating temporary views with properties is not allowed, and SHOW TBLPROPERTIES on temporary views can also be disabled to be consistent.

Does this PR introduce any user-facing change?

Yes,
Before:

scala> sql("CREATE TEMPORARY VIEW tv TBLPROPERTIES('p1'='v1') AS SELECT 1 AS c1")
res0: org.apache.spark.sql.DataFrame = []

scala> sql("SHOW TBLPROPERTIES tv").show
+---+-----+
|key|value|
+---+-----+
+---+-----+

After:

scala> sql("CREATE TEMPORARY VIEW tv TBLPROPERTIES('p1'='v1') AS SELECT 1 AS c1")
res0: org.apache.spark.sql.DataFrame = []

scala> sql("SHOW TBLPROPERTIES tv").show
org.apache.spark.sql.AnalysisException: tv is a temp view, not a table or permanent view.; line 1 pos 0
  at org.apache.spark.sql.catalyst.analysis.package$AnalysisErrorAt.failAnalysis(package.scala:42)
  at org.apache.spark.sql.catalyst.analysis.Analyzer$ResolveTempViews$$anonfun$apply$7.$anonfun$applyOrElse$42(Analyzer.scala:863)
  at scala.Option.foreach(Option.scala:407)
...

How was this patch tested?

Updated existing tests

@SparkQA
Copy link

SparkQA commented Jul 16, 2020

Test build #125923 has finished for PR 29127 at commit a17696b.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class UnresolvedTableOrPermanentView(multipartIdentifier: Seq[String]) extends LeafNode

val catalog = sparkSession.sessionState.catalog
if (catalog.isTemporaryTable(table)) {
Seq.empty[Row]
throw new AnalysisException(s"SHOW TBLPROPERTIES is not allowed on a temporary view: $table")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will not be hit if you go thru SHOW TBLPROPERTIES command.

@imback82
Copy link
Contributor Author

cc: @cloud-fan @viirya

* Holds the name of a table or a permanent/temporary view that has yet to be looked up in
* a catalog. It will be resolved to [[ResolvedTable]] or [[ResolvedView]] during analysis.
*/
case class UnresolvedTableOrView(multipartIdentifier: Seq[String]) extends LeafNode {
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we simply add an acceptTempView flag here instead of adding UnresolvedTableOrPermanentView?

@cloud-fan
Copy link
Contributor

At first glance, I thought the PR description is confusing, because we can specify properties of temp view but can't display it. Then I realized that we just ignore the properties when creating temp views.

I think we should fail the temp view creating instead of silently ignoring the table properties when specified. @imback82 can you fix this first?

@imback82
Copy link
Contributor Author

I think we should fail the temp view creating instead of silently ignoring the table properties when specified. @imback82 can you fix this first?

Will do.

@cloud-fan
Copy link
Contributor

Now I don't have a strong opinion. Users can't create temp view with properties and it seems fine if SHOW TBLPROPERTIES returns Nil for temp view. cc @maropu @viirya

@SparkQA
Copy link

SparkQA commented Jul 24, 2020

Test build #126446 has finished for PR 29127 at commit 933dc28.

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

@imback82 imback82 changed the title [SPARK-32327][SQL] Introduce UnresolvedTableOrPermanentView for commands that support a table and permanent view, but not a temporary view [SPARK-32327][SQL] SHOW TBLPROPERTIES is not allowed on temporary views Jul 24, 2020
@viirya
Copy link
Member

viirya commented Jul 24, 2020

Now I don't have a strong opinion. Users can't create temp view with properties and it seems fine if SHOW TBLPROPERTIES returns Nil for temp view. cc @maropu @viirya

Sounds ok too.

@imback82
Copy link
Contributor Author

Yes, I agree. I will close this for now. Thanks!

@imback82 imback82 closed this Jul 24, 2020
@maropu
Copy link
Member

maropu commented Jul 25, 2020

Now I don't have a strong opinion. Users can't create temp view with properties and it seems fine if SHOW TBLPROPERTIES returns Nil for temp view. cc @maropu @viirya

I missed that.. yea, the decision sounds okay to me, too. Anyway, thanks for the work, @imback82 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants