[SPARK-14362] [SPARK-14406] [SQL] DDL Native Support: Drop View and Drop Table #12146

gatorsmile · 2016-04-04T05:58:30Z

What changes were proposed in this pull request?

This PR is to provide a native support for DDL DROP VIEW and DROP TABLE. The PR includes native parsing and native analysis.

Based on the HIVE DDL document for [DROP_VIEW_WEB_LINK](https://cwiki.apache.org/confluence/display/Hive/LanguageManual+DDL#LanguageManualDDL-
DropView), DROP VIEW is defined as,
Syntax:

DROP VIEW [IF EXISTS] [db_name.]view_name;

to remove metadata for the specified view.
illegal to use DROP TABLE on a view.
illegal to use DROP VIEW on a table.
this command only works in HiveContext. In SQLContext, we will get an exception.

This PR also handles DROP TABLE.
Syntax:

DROP TABLE [IF EXISTS] table_name [PURGE];

Previously, the DROP TABLE command only can drop Hive tables in HiveContext. Now, after this PR, this command also can drop temporary table, external table, external data source table in SQLContext.
In HiveContext, we will not issue an exception if the to-be-dropped table does not exist and users did not specify IF EXISTS. Instead, we just log an error message. If IF EXISTS is specified, we will not issue any error message/exception.
In SQLContext, we will issue an exception if the to-be-dropped table does not exist, unless IF EXISTS is specified.
Data will not be deleted if the tables are external, unless table type is managed_table.

How was this patch tested?

For verifying command parsing, added test cases in spark/sql/hive/HiveDDLCommandSuite.scala
For verifying command analysis, added test cases in spark/sql/hive/execution/HiveDDLSuite.scala

gatorsmile · 2016-04-08T21:57:12Z

Sure, will do it.

SparkQA · 2016-04-08T22:26:15Z

Test build #55392 has finished for PR 12146 at commit f00cbea.

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

gatorsmile · 2016-04-09T05:17:51Z

@yhuai Actually, when we drop the external table, we do delete the data.

If we do not want to delete the data, we need to change the following value from true to false, which is the @param deleteData (i.e., deletes the underlying data along with metadata)
https://github.com/apache/spark/blob/master/sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala#L345

Update: Sorry, my case is wrong.

create external table $tabName(c1 int COMMENT 'abc', c2 string)
stored as parquet
location '$tmpDir'
as select 1, '3'

Although we use the EXTERNAL keyword, the table is still managed_table. The above statement could be wrong. As you pointed out in another PR, the data in an external table should not be deleted.
https://github.com/apache/hive/blob/release-0.13.1/metastore/src/java/org/apache/hadoop/hive/metastore/HiveMetaStore.java#L2239-L2251

SparkQA · 2016-04-09T07:47:37Z

Test build #55424 has finished for PR 12146 at commit f280ec4.

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

SparkQA · 2016-04-09T15:32:11Z

Test build #55431 has finished for PR 12146 at commit 6c71124.

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

yhuai · 2016-04-09T16:59:20Z

That deleteData flag is confusing. If you look at the metastore server's code, you will see that there is another flag called isExternal generated from the table type. For the query

create external table $tabName(c1 int COMMENT 'abc', c2 string)
stored as parquet
location '$tmpDir'
as select 1, '3'

I am not sure I understand what you mean by Although we use the EXTERNAL keyword, the table is still managed_table. The above statement could be wrong. Why it is still a managed_table?

gatorsmile · 2016-04-09T17:28:11Z

It sounds like Hive ignores and overwrites the type we provided when we attempt to create the table and converts it to Managed. I will do more investigation today. Just added a test case for data source tables, which is EXTERNAL.

You can see I did the table type verification in both test cases.

yhuai · 2016-04-09T18:26:17Z

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

+             |stored as parquet
+             |location '$tmpDir'
+             |as select 1, '3'
+          """.stripMargin)


The table type is silently changed to MANAGED_TABLE at https://github.com/apache/hive/blob/master/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1315-L1326.

For an external table, table type needs to be EXTERNAL_TABLE and its table properties should have an entry EXTERNAL set to TRUE.

So, there is a bug in HiveClientImpl.toHiveTable. I will fix that.

btw, for this test case, I am not sure Hive allows you to provide a column list in a CTAS table. Also, I am not sure if Hive allows you to provide EXTERNAL keyword for a table generated by CTAS query. Can you double check?

Thank you for sharing this with me!

hive> create external table tab1(c1 int, c2 string) stored as parquet as select 1, 3; FAILED: SemanticException [Error 10065]: CREATE TABLE AS SELECT command cannot specify the list of columns for the target table hive> create external table tab1 stored as parquet as select 1, 3; FAILED: SemanticException [Error 10070]: CREATE-TABLE-AS-SELECT cannot create external table

Obviously, Spark SQL is different from Hive. Should we change the behavior to match Hive SQL?

The SQL in the test case is processed by Spark SQL. It is not a native Hive SQL command.

I think we have been supporting it for a long time (maybe by accident). I have created https://issues.apache.org/jira/browse/SPARK-14507 for deciding if we should keep it or drop the support of it.

For this test, can we change to a real managed table (drop the external keyword from the statement)?

Sure, will do it. Thanks!

BTW, it does not work even if without the external keyword. I will also remove the column list.

hive> create table tab11(c1 int, c2 string) stored as parquet as select 1, 3; FAILED: SemanticException [Error 10065]: CREATE TABLE AS SELECT command cannot specify the list of columns for the target table

SparkQA · 2016-04-09T18:57:20Z

Test build #55432 has finished for PR 12146 at commit 2f45c0d.

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

…g Parsing #### What changes were proposed in this pull request? "Not good to slightly ignore all the un-supported options/clauses. We should either support it or throw an exception." A comment from yhuai in another PR #12146 - Can `Explain` be an exception? The `Formatted` clause is used in `HiveCompatibilitySuite`. - Two unsupported clauses in `Drop Table` are handled in a separate PR: #12146 #### How was this patch tested? Test cases are added to verify all the cases. Author: gatorsmile <[email protected]> Closes #12255 from gatorsmile/warningToException.

yhuai · 2016-04-09T21:47:03Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalogSuite.scala

-      catalog.dropTable(TableIdentifier("unknown_table", Some("db2")), ignoreIfNotExists = false)
-    }
+    // If the table does not exist, we do not issue an exception. Instead, we output an error
+    // message to console when ignoreIfNotExists is set to false. This is to make it consistent


Seems we only have a log message?

Yeah, this is not completely consistent. let me rewrite it. Thanks!

yhuai · 2016-04-09T21:53:00Z

@gatorsmile I have finished my review. Can you update the PR to address the following comments?
#12146 (comment)
https://github.com/apache/spark/pull/12146/files#r59121965
https://github.com/apache/spark/pull/12146/files#r59122042
https://github.com/apache/spark/pull/12146/files#r59122053
https://github.com/apache/spark/pull/12146/files#r59122056

gatorsmile · 2016-04-09T21:56:37Z

Sure, will do it soon. Really thank you for your review!

yhuai · 2016-04-09T23:33:24Z

Also update the description if you get a chance?

gatorsmile · 2016-04-10T00:01:34Z

Sure, will do it. Thanks!

SparkQA · 2016-04-10T00:15:01Z

Test build #55451 has finished for PR 12146 at commit 40861a3.

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

yhuai · 2016-04-10T00:37:25Z

LGTM. We can remove isViewSupported in a follow-up.

…iew and Drop Table #### What changes were proposed in this pull request? This PR is to address the comment: #12146 (diff). It removes the function `isViewSupported` from `SessionCatalog`. After the removal, we still can capture the user errors if users try to drop a table using `DROP VIEW`. #### How was this patch tested? Modified the existing test cases Author: gatorsmile <[email protected]> Closes #12284 from gatorsmile/followupDropTable.

gatorsmile and others added 30 commits November 13, 2015 14:50

Merge remote-tracking branch 'upstream/master'

01e4cdf

Merge remote-tracking branch 'upstream/master'

6835704

Merge remote-tracking branch 'upstream/master'

9180687

SPARK-11633

b38a21e

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

d2b84af

Merge remote-tracking branch 'upstream/master'

fda8025

Merge branch 'master' of https://github.com/gatorsmile/spark

ac0dccd

Merge remote-tracking branch 'upstream/master'

6e0018b

converge

0546772

converge

b37a64f

Merge remote-tracking branch 'upstream/master'

c2a872c

Merge remote-tracking branch 'upstream/master'

ab6dbd7

Merge remote-tracking branch 'upstream/master'

4276356

Merge remote-tracking branch 'upstream/master'

2dab708

Merge remote-tracking branch 'upstream/master'

0458770

Merge remote-tracking branch 'upstream/master'

1debdfa

Merge remote-tracking branch 'upstream/master'

763706d

Merge remote-tracking branch 'upstream/master'

4de6ec1

Merge remote-tracking branch 'upstream/master'

9422a4f

Merge remote-tracking branch 'upstream/master'

52bdf48

Merge remote-tracking branch 'upstream/master'

1e95df3

Merge remote-tracking branch 'upstream/master'

fab24cf

Merge remote-tracking branch 'upstream/master'

8b2e33b

Merge remote-tracking branch 'upstream/master'

2ee1876

Merge remote-tracking branch 'upstream/master'

b9f0090

Merge remote-tracking branch 'upstream/master'

ade6f7e

Merge remote-tracking branch 'upstream/master'

9fd63d2

Merge remote-tracking branch 'upstream/master'

5199d49

Merge remote-tracking branch 'upstream/master'

404214c

Merge remote-tracking branch 'upstream/master'

c001dd9

address comments.

f280ec4

update the test case

6c71124

added a test case

2f45c0d

yhuai reviewed Apr 9, 2016
View reviewed changes

address comments.

40861a3

asfgit closed this in dfce966 Apr 10, 2016

gatorsmile mentioned this pull request Apr 10, 2016

[SPARK-14362][SPARK-14406][SQL][Follow-up] DDL Native Support: Drop View and Drop Table #12284

Closed

gatorsmile deleted the dropView branch April 10, 2016 14:43

[SPARK-14362] [SPARK-14406] [SQL] DDL Native Support: Drop View and Drop Table #12146

[SPARK-14362] [SPARK-14406] [SQL] DDL Native Support: Drop View and Drop Table #12146

Uh oh!

Conversation

gatorsmile commented Apr 4, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gatorsmile commented Apr 8, 2016

Uh oh!

SparkQA commented Apr 8, 2016

Uh oh!

gatorsmile commented Apr 9, 2016

Uh oh!

SparkQA commented Apr 9, 2016

Uh oh!

SparkQA commented Apr 9, 2016

Uh oh!

yhuai commented Apr 9, 2016

Uh oh!

gatorsmile commented Apr 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!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yhuai commented Apr 9, 2016

Uh oh!

gatorsmile commented Apr 9, 2016

Uh oh!

yhuai commented Apr 9, 2016

Uh oh!

gatorsmile commented Apr 10, 2016

Uh oh!

SparkQA commented Apr 10, 2016

Uh oh!

yhuai commented Apr 10, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants