[SPARK-17440] [SPARK-17441] Fixed Multiple Bugs in ALTER TABLE #15004

gatorsmile · 2016-09-07T22:37:54Z

What changes were proposed in this pull request?

For the following ALTER TABLE DDL, we should issue an exception when the target table is a VIEW:

 ALTER TABLE viewName SET LOCATION '/path/to/your/lovely/heart'

 ALTER TABLE viewName SET SERDE 'whatever'

 ALTER TABLE viewName SET SERDEPROPERTIES ('x' = 'y')

 ALTER TABLE viewName PARTITION (a=1, b=2) SET SERDEPROPERTIES ('x' = 'y')

 ALTER TABLE viewName ADD IF NOT EXISTS PARTITION (a='4', b='8')

 ALTER TABLE viewName DROP IF EXISTS PARTITION (a='2')

 ALTER TABLE viewName RECOVER PARTITIONS

 ALTER TABLE viewName PARTITION (a='1', b='q') RENAME TO PARTITION (a='100', b='p')

In addition, ALTER TABLE RENAME PARTITION is unable to handle data source tables, just like the other ALTER PARTITION commands. We should issue an exception instead.

How was this patch tested?

Added a few test cases.

SparkQA · 2016-09-08T00:39:29Z

Test build #65058 has finished for PR 15004 at commit ea630f8.

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

gatorsmile · 2016-09-08T01:51:03Z

cc @cloud-fan @yhuai

gatorsmile · 2016-09-08T03:36:49Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala

+
+        checkMisuseForAlterTableOrView(
+          s"$oldViewName ADD IF NOT EXISTS PARTITION (a='4', b='8')", isAlterView = false)
+


We also need to test isAlterView = true for ADD PARTITION and DROP PARTITION. Let me add them now...

uh, both are not supported. nvm. I think we already cover all the cases. Code is ready to review. Thanks!

cloud-fan · 2016-09-08T07:15:03Z

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

+      verifyAlterTableType(_, tableIdentifier, isView))
+  }
+
+  def verifyAlterTableType(


I think we only need this one, and we can remove the old one.

gatorsmile · 2016-09-09T06:11:52Z

Accept all the comments. Will address comments now. Thanks! : )

SparkQA · 2016-09-09T08:18:52Z

Test build #65137 has finished for PR 15004 at commit 5d0cffe.

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

cloud-fan · 2016-09-09T14:02:38Z

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

+    createPartitionedTable(tableIdent, isDatasourceTable = false)
    sql("ALTER TABLE dbx.tab1 PARTITION (a='1', b='q') RENAME TO PARTITION (a='100', b='p')")
-    sql("ALTER TABLE dbx.tab1 PARTITION (a='2', b='c') RENAME TO PARTITION (a='200', b='c')")
+    sql("ALTER TABLE dbx.tab1 PARTITION (a='2', b='c') RENAME TO PARTITION (a='20', b='c')")


unnecessary change?

https://github.com/gatorsmile/spark/blob/5d0cffe6b59dc36d63f7d1e055903b53ce2fec42/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala#L878 will be longer than 100 characters... That is why I changed it to avoid adding another line.

ok then it's fine...

SparkQA · 2016-09-09T16:16:21Z

Test build #65151 has finished for PR 15004 at commit 8b2ed94.

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

cloud-fan · 2016-09-14T15:15:11Z

retest this please

cloud-fan · 2016-09-14T15:15:18Z

LGTM

SparkQA · 2016-09-14T17:21:04Z

Test build #65375 has finished for PR 15004 at commit 8b2ed94.

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

cloud-fan · 2016-09-14T18:14:01Z

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


      val e = intercept[AnalysisException] {
-        sql("ALTER TABLE tab1 RENAME TO tab2")
+        sql("ALTER VIEW tab1 RENAME TO tab2")


Sorry I missed this one. So we can't use ALTER TABLE to alter temp view now?

en... You are right. Let me resolve this soon.

Will also add a few test cases for ensuring ALTER TABLE commands can alter temporary views.

The problem is fixed. : )

cloud-fan · 2016-09-15T02:49:56Z

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

   * If the command ALTER VIEW is to alter a table or ALTER TABLE is to alter a view,
-   * issue an exception [[AnalysisException]].
+   * issue an exception [[AnalysisException]]. Temporary views can be altered by both
+   * ALTER VIEW and ALTER TABLE commands.


we may also explain the reason, i.e. temporary views can be created by CREATE TEMPORARY TABLE

and we can remove this trick if we decide to not allow temporary views to be created by CREATE TEMPORARY TABLE.

Yeah. Let me do it now. Thanks!

The comment is added. Thanks!

SparkQA · 2016-09-15T03:43:12Z

Test build #65417 has finished for PR 15004 at commit 159f101.

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

SparkQA · 2016-09-15T06:00:14Z

Test build #65419 has finished for PR 15004 at commit 91c3a28.

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

cloud-fan · 2016-09-15T06:37:18Z

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

+    val table = catalog.getTableMetadata(tableName)
+    if (DDLUtils.isDatasourceTable(table)) {
+      throw new AnalysisException(
+        "ALTER TABLE RENAME PARTITION is not allowed for tables defined using the datasource API")


we can just say data source table, to be consistent with other places.

Sure, will do it. Thanks!

cloud-fan · 2016-09-15T06:43:46Z

thanks, merging to master! you can address my last comment in your other PRs

gatorsmile · 2016-09-15T06:47:35Z

Thank you! : )

### What changes were proposed in this pull request? For the following `ALTER TABLE` DDL, we should issue an exception when the target table is a `VIEW`: ```SQL ALTER TABLE viewName SET LOCATION '/path/to/your/lovely/heart' ALTER TABLE viewName SET SERDE 'whatever' ALTER TABLE viewName SET SERDEPROPERTIES ('x' = 'y') ALTER TABLE viewName PARTITION (a=1, b=2) SET SERDEPROPERTIES ('x' = 'y') ALTER TABLE viewName ADD IF NOT EXISTS PARTITION (a='4', b='8') ALTER TABLE viewName DROP IF EXISTS PARTITION (a='2') ALTER TABLE viewName RECOVER PARTITIONS ALTER TABLE viewName PARTITION (a='1', b='q') RENAME TO PARTITION (a='100', b='p') ``` In addition, `ALTER TABLE RENAME PARTITION` is unable to handle data source tables, just like the other `ALTER PARTITION` commands. We should issue an exception instead. ### How was this patch tested? Added a few test cases. Author: gatorsmile <[email protected]> Closes apache#15004 from gatorsmile/altertable.

gatorsmile added 2 commits September 7, 2016 15:23

fix.

16efafa

fix.

ea630f8

gatorsmile reviewed Sep 8, 2016
View reviewed changes

Merge remote-tracking branch 'upstream/master' into altertable

8748dd0

cloud-fan reviewed Sep 8, 2016
View reviewed changes

address comments

5d0cffe

cloud-fan reviewed Sep 9, 2016
View reviewed changes

address comments

8b2ed94

cloud-fan reviewed Sep 14, 2016

View reviewed changes

fix

159f101

cloud-fan reviewed Sep 15, 2016

View reviewed changes

address comments

91c3a28

cloud-fan reviewed Sep 15, 2016

View reviewed changes

asfgit closed this in 6a6adb1 Sep 15, 2016


		checkMisuseForAlterTableOrView(
		s"$oldViewName ADD IF NOT EXISTS PARTITION (a='4', b='8')", isAlterView = false)

[SPARK-17440] [SPARK-17441] Fixed Multiple Bugs in ALTER TABLE #15004

[SPARK-17440] [SPARK-17441] Fixed Multiple Bugs in ALTER TABLE #15004

Uh oh!

Conversation

gatorsmile commented Sep 7, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Sep 8, 2016

Uh oh!

gatorsmile commented Sep 8, 2016

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!

gatorsmile commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SparkQA commented Sep 9, 2016

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 Sep 9, 2016

Uh oh!

cloud-fan commented Sep 14, 2016

Uh oh!

cloud-fan commented Sep 14, 2016

Uh oh!

SparkQA commented Sep 14, 2016

Uh oh!

cloud-fan Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 15, 2016

Uh oh!

SparkQA commented Sep 15, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Sep 15, 2016

Uh oh!

gatorsmile commented Sep 15, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gatorsmile commented Sep 9, 2016 •

edited

Loading

cloud-fan Sep 14, 2016 •

edited

Loading