[SPARK-32374][SQL] Disallow setting properties when creating temporary views by imback82 · Pull Request #29167 · apache/spark

imback82 · 2020-07-20T19:07:46Z

What changes were proposed in this pull request?

Currently, you can specify properties when creating a temporary view. However, the specified properties are not used and can be misleading.

This PR propose to disallow specifying properties when creating temporary views.

Why are the changes needed?

To avoid confusion by disallowing specifying unused properties.

Does this PR introduce any user-facing change?

Yes, now if you create a temporary view with properties, the operation will fail:

scala> sql("CREATE TEMPORARY VIEW tv TBLPROPERTIES('p1'='v1') AS SELECT 1 AS c1")
org.apache.spark.sql.catalyst.parser.ParseException:
Operation not allowed: CREATE TEMPORARY VIEW ... TBLPROPERTIES (property_name = property_value, ...)(line 1, pos 0)

== SQL ==
CREATE TEMPORARY VIEW tv TBLPROPERTIES('p1'='v1') AS SELECT 1 AS c1
^^^

How was this patch tested?

Added tests

imback82 · 2020-07-20T19:11:04Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala

+      .getOrElse(Map.empty)
+    if (ctx.TEMPORARY != null && !properties.isEmpty) {
+      operationNotAllowed(
+        "CREATE TEMPORARY VIEW ... TBLPROPERTIES (property_name = property_value, ...)", ctx)


This can be moved to CreateViewCommand if that is a better place to check this:

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

Lines 80 to 85 in fe07521

// 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.")

}

I think we should move that check to the parser as well (in a separate PR). If we support view in DS v2, we shouldn't duplicated the check in both v1 and v2 commands.

SparkQA · 2020-07-20T20:36:26Z

Test build #126196 has finished for PR 29167 at commit de95968.

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

dongjoon-hyun · 2020-07-20T20:58:41Z

Could you fix the UT failure?

org.apache.spark.sql.hive.execution.HiveSQLViewSuite.correctly parse CREATE TEMPORARY VIEW statement

dongjoon-hyun · 2020-07-20T20:59:26Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/DDLParserSuite.scala

    intercept(sql2, "Found duplicate clauses: TBLPROPERTIES")
  }

+  test("create temporary view with properties not allowed") {


It would be great if we can have a JIRA ID prefix like SPARK-32374: .

updated. thanks!

viirya · 2020-07-20T21:19:31Z

Hmm, does it mean the specified properties are not used by Spark currently? If users possibly already use it?

imback82 · 2020-07-20T21:39:28Z

Hmm, does it mean the specified properties are not used by Spark currently? If users possibly already use it?

Correct, they are not being used for temporary views. Thus, if you run SHOW TBLPROPERTIES on temp views, you will always get empty results (and it is being addressed in #29127).

Btw, properties are used for DataSource if you do CREATE TEMPORARY VIEW ... USING, but not stored.

SparkQA · 2020-07-21T02:23:30Z

Test build #126202 has finished for PR 29167 at commit 8b35497.

This patch fails PySpark pip packaging tests.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2020-07-21T03:34:49Z

cc @cloud-fan

cloud-fan · 2020-07-21T04:49:43Z

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

+      }
+      assert(e.message.contains(
+        "Operation not allowed: CREATE TEMPORARY VIEW ... " +
+          "TBLPROPERTIES (property_name = property_value, ...)"))


how about: Operation not allowed: TBLPROPERTIES can't coexist with CREATE TEMPORARY VIEW

SparkQA · 2020-07-21T21:06:27Z

Test build #126268 has finished for PR 29167 at commit 7d8439f.

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

SparkQA · 2020-07-21T22:32:21Z

Test build #126269 has finished for PR 29167 at commit 4f725c2.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-23T04:35:12Z

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


+  test("SPARK-32374: disallow setting properties for CREATE TEMPORARY VIEW") {
+    withTempView("myabcdview") {
+      val e = intercept[AnalysisException] {


parser exception?

interesting, the tests pass. Do you know why?

Oh because ParseException extends AnalysisException. I will update this.

SparkQA · 2020-07-23T07:05:02Z

Test build #126380 has finished for PR 29167 at commit 477b696.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

imback82 · 2020-07-23T07:19:24Z

retest this please

SparkQA · 2020-07-23T12:20:31Z

Test build #126395 has finished for PR 29167 at commit 477b696.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2020-07-23T14:32:02Z

thanks, merging to master!

initial commit

de95968

probot-autolabeler bot added the SQL label Jul 20, 2020

imback82 commented Jul 20, 2020

View reviewed changes

dongjoon-hyun reviewed Jul 20, 2020

View reviewed changes

imback82 added 3 commits July 20, 2020 14:02

update tests

48d7464

address comments

5eeed3c

update test comment

8b35497

cloud-fan reviewed Jul 21, 2020

View reviewed changes

imback82 added 2 commits July 21, 2020 10:14

Address PR comments

7d8439f

fix test

4f725c2

cloud-fan reviewed Jul 23, 2020

View reviewed changes

address PR comment

477b696

cloud-fan closed this in 35345e3 Jul 23, 2020

	// 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.")
	}

Conversation

imback82 commented Jul 20, 2020

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 20, 2020

Uh oh!

dongjoon-hyun commented Jul 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

viirya commented Jul 20, 2020

Uh oh!

imback82 commented Jul 20, 2020

Uh oh!

SparkQA commented Jul 21, 2020

Uh oh!

imback82 commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 21, 2020

Uh oh!

SparkQA commented Jul 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

imback82 commented Jul 23, 2020

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

cloud-fan commented Jul 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants