[SPARK-19539][SQL] Block duplicate temp table during creation #16878

xwu0226 · 2017-02-10T01:29:30Z

What changes were proposed in this pull request?

Current CREATE TEMPORARY TABLE ... is deprecated and recommend users to use CREATE TEMPORARY VIEW ... And it does not support IF NOT EXISTS clause. However, if there is an existing temporary view defined, it is possible to unintentionally replace this existing view by issuing CREATE TEMPORARY TABLE ... with the same table/view name.

This PR is to disallow CREATE TEMPORARY TABLE ... with an existing view name.
Under the cover, CREATE TEMPORARY TABLE ... will be changed to create temporary view, however, passing in a flag replace=false, instead of currently true. So when creating temporary view under the cover, if there is existing view with the same name, the operation will be blocked.

How was this patch tested?

New unit test case is added and updated some existing test cases to adapt the new behavior

xwu0226 · 2017-02-10T01:30:28Z

@gatorsmile @cloud-fan Thanks!

SparkQA · 2017-02-10T03:53:13Z

Test build #72670 has finished for PR 16878 at commit 133bd99.

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

cloud-fan · 2017-02-10T18:40:36Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

@@ -425,7 +425,9 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {

        logWarning(s"CREATE TEMPORARY TABLE ... USING ... is deprecated, please use " +


can you check when did we forbid this? maybe it's time to fully remove the support

@cloud-fan Thanks for reviewing! I found that this deprecation was added by your PR #14482 back on 8/5/2016. Do you think it is the right time to remove CREATE TEMPORARY TABLE... completely?

no, it should be earlier: https://github.com/apache/spark/pull/14482/files#diff-7253a38df7e111ecf6b1ef71feba383bL425

Are we referring to the same PR? I found the change here https://github.com/apache/spark/pull/14482/files#diff-1bb4f7bd5a2656f48bcd3c857167a11bR362, where this WARN message is added in SparkSQLParser.scala

I mean it was moved from other places, so this warning should have been added earlier.

I see. sorry. I misunderstood. Let me check further.

#13414 deprecated this. Merged on Jun 7th, 2016.

it was merged to 2.0, let's keep it

cloud-fan · 2017-02-12T02:17:42Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

         |CREATE TEMPORARY TABLE carsTable USING csv
         |OPTIONS (path "${testFile(carsFile8859)}", header "true",
         |charset "iso-8859-1", delimiter "þ")
-      """.stripMargin.replaceAll("\n", " "))
+      """.stripMargin.
+          replaceAll("\n", " "))


it can be put in the previous line

yes. thanks!

cloud-fan · 2017-02-12T02:17:58Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala


-    verifyCars(spark.table("carsTable"), numFields = 6, withHeader = true, checkHeader = false)
+        ))


it can be put in the previous line

yes. Thanks!

cloud-fan · 2017-02-12T02:18:57Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

-      s"""
+    withView("carsTable") {
+      spark.sql(
+        s"""
         |CREATE TEMPORARY TABLE carsTable USING csv


as you are fixing this test suite, can you replace all CREATE TEMPORARY TABLE to VIEW?

Will do. Thanks!

@cloud-fan there are test cases that are related to ALTER TABLE for the temp table, should we keep such test cases for a while?

gatorsmile · 2017-02-12T03:19:38Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala

@@ -425,7 +425,9 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {

        logWarning(s"CREATE TEMPORARY TABLE ... USING ... is deprecated, please use " +
          "CREATE TEMPORARY VIEW ... USING ... instead")
-        CreateTempViewUsing(table, schema, replace = true, global = false, provider, options)
+        // Since we don't support IF NOT EXISTS for temp table, we should not allow
+        // replacing existing temp table, that may accidentally remove a temp view in use.


Just got your points... How about update the description to

Unlike CREATE TEMPORARY VIEW USING, CREATE TEMPORARY TABLE USING does not support IF NOT EXISTS. Users are not allowed to replace the existing temp table.

Thanks!

OK. Good point. Thanks!

gatorsmile · 2017-02-12T03:21:30Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

+      val e = intercept[TempTableAlreadyExistsException] {
+        sql("CREATE TEMPORARY TABLE t_temp (c3 int, c4 string) USING JSON")
+      }.getMessage
+      assert(e.contains("already exists"))


Please capture the whole error message.

SparkQA · 2017-02-13T08:02:43Z

Test build #72800 has finished for PR 16878 at commit f7253c5.

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

cloud-fan · 2017-02-13T19:18:40Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

@@ -971,7 +971,7 @@ class DDLSuite extends QueryTest with SharedSQLContext with BeforeAndAfterEach {
        |)
      """.stripMargin)
    assert(catalog.listTables("default") == Seq(TableIdentifier("tab1")))
-    sql("DROP TABLE tab1")
+    sql("DROP view tab1")


nit: upper case "view"

cloud-fan · 2017-02-13T19:20:04Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+    withView("carsTable") {
+      spark.sql(
+        s"""
+         |CREATE TEMPORARY VIEW carsTable USING csv


the indention is wrong

cloud-fan · 2017-02-13T19:20:19Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+    withView("carsTable") {
+      spark.sql(
+        s"""
+         |CREATE TEMPORARY VIEW carsTable USING csv


the indention is wrong

cloud-fan · 2017-02-13T19:20:24Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+    withView("carsTable") {
+      spark.sql(
+        s"""
+         |CREATE TEMPORARY VIEW carsTable


the indention is wrong

cloud-fan · 2017-02-13T19:20:31Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+    withView("carsTable") {
+      spark.sql(
+        s"""
+           |CREATE TEMPORARY VIEW carsTable


the indention is wrong

cloud-fan · 2017-02-13T19:20:38Z

sql/core/src/test/scala/org/apache/spark/sql/execution/datasources/csv/CSVSuite.scala

+    withView("carsTable") {
+      spark.sql(
+        s"""
+           |CREATE TEMPORARY VIEW carsTable


the indention is wrong

cloud-fan · 2017-02-13T23:45:14Z

LGTM, pending jenkins, thanks for working on it!

SparkQA · 2017-02-14T00:53:36Z

Test build #72834 has finished for PR 16878 at commit 6683d8c.

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

SparkQA · 2017-02-14T01:24:58Z

Test build #72837 has finished for PR 16878 at commit 3a989cf.

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

gatorsmile · 2017-02-14T03:46:14Z

Thanks! Merging to master.

## What changes were proposed in this pull request? Current `CREATE TEMPORARY TABLE ... ` is deprecated and recommend users to use `CREATE TEMPORARY VIEW ...` And it does not support `IF NOT EXISTS `clause. However, if there is an existing temporary view defined, it is possible to unintentionally replace this existing view by issuing `CREATE TEMPORARY TABLE ...` with the same table/view name. This PR is to disallow `CREATE TEMPORARY TABLE ...` with an existing view name. Under the cover, `CREATE TEMPORARY TABLE ...` will be changed to create temporary view, however, passing in a flag `replace=false`, instead of currently `true`. So when creating temporary view under the cover, if there is existing view with the same name, the operation will be blocked. ## How was this patch tested? New unit test case is added and updated some existing test cases to adapt the new behavior Author: Xin Wu <xinwu@us.ibm.com> Closes apache#16878 from xwu0226/block_duplicate_temp_table.

cloud-fan reviewed Feb 10, 2017

View reviewed changes

cloud-fan reviewed Feb 12, 2017

View reviewed changes

gatorsmile reviewed Feb 12, 2017

View reviewed changes

xwu0226 added 4 commits February 12, 2017 08:38

don't allow create temporary table to replace existing temp view

0afe3cb

update testcases

290fa15

update testcases

4a3a06a

update testcases

f7253c5

xwu0226 force-pushed the block_duplicate_temp_table branch from 133bd99 to f7253c5 Compare February 13, 2017 05:32

cloud-fan reviewed Feb 13, 2017

View reviewed changes

xwu0226 added 2 commits February 13, 2017 14:09

update upon review

6683d8c

fix indention

3a989cf

asfgit closed this in 1ab9731 Feb 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-19539][SQL] Block duplicate temp table during creation #16878

[SPARK-19539][SQL] Block duplicate temp table during creation #16878

xwu0226 commented Feb 10, 2017

xwu0226 commented Feb 10, 2017

SparkQA commented Feb 10, 2017

cloud-fan Feb 10, 2017

xwu0226 Feb 10, 2017

cloud-fan Feb 10, 2017

xwu0226 Feb 10, 2017

cloud-fan Feb 10, 2017

xwu0226 Feb 10, 2017

xwu0226 Feb 10, 2017

cloud-fan Feb 12, 2017

cloud-fan Feb 12, 2017

xwu0226 Feb 12, 2017

cloud-fan Feb 12, 2017

xwu0226 Feb 12, 2017

cloud-fan Feb 12, 2017

xwu0226 Feb 12, 2017

xwu0226 Feb 12, 2017

gatorsmile Feb 12, 2017 •

edited

Loading

xwu0226 Feb 12, 2017

gatorsmile Feb 12, 2017

xwu0226 Feb 12, 2017

SparkQA commented Feb 13, 2017

cloud-fan Feb 13, 2017

cloud-fan Feb 13, 2017

cloud-fan Feb 13, 2017

cloud-fan Feb 13, 2017

cloud-fan Feb 13, 2017

cloud-fan Feb 13, 2017

cloud-fan commented Feb 13, 2017

SparkQA commented Feb 14, 2017

SparkQA commented Feb 14, 2017

gatorsmile commented Feb 14, 2017

		@@ -425,7 +425,9 @@ class SparkSqlAstBuilder(conf: SQLConf) extends AstBuilder {

		logWarning(s"CREATE TEMPORARY TABLE ... USING ... is deprecated, please use " +


		verifyCars(spark.table("carsTable"), numFields = 6, withHeader = true, checkHeader = false)
		))

[SPARK-19539][SQL] Block duplicate temp table during creation #16878

[SPARK-19539][SQL] Block duplicate temp table during creation #16878

Conversation

xwu0226 commented Feb 10, 2017

What changes were proposed in this pull request?

How was this patch tested?

xwu0226 commented Feb 10, 2017

SparkQA commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gatorsmile Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Feb 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloud-fan commented Feb 13, 2017

SparkQA commented Feb 14, 2017

SparkQA commented Feb 14, 2017

gatorsmile commented Feb 14, 2017

gatorsmile Feb 12, 2017 •

edited

Loading