[SPARK-19724][SQL]create a managed table with an existed default table should throw an exception #20886

gengliangwang · 2018-03-22T22:52:48Z

What changes were proposed in this pull request?

This PR is to finish #17272

This JIRA is a follow up work after SPARK-19583

As we discussed in that PR

The following DDL for a managed table with an existed default location should throw an exception:

CREATE TABLE ... (PARTITIONED BY ...) AS SELECT ...
CREATE TABLE ... (PARTITIONED BY ...)
Currently there are some situations which are not consist with above logic:

CREATE TABLE ... (PARTITIONED BY ...) succeed with an existed default location
situation: for both hive/datasource(with HiveExternalCatalog/InMemoryCatalog)

CREATE TABLE ... (PARTITIONED BY ...) AS SELECT ...
situation: hive table succeed with an existed default location

This PR is going to make above two situations consist with the logic that it should throw an exception
with an existed default location.

How was this patch tested?

unit test added

SparkQA · 2018-03-22T23:39:36Z

Test build #88530 has finished for PR 20886 at commit d584c9b.

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

SparkQA · 2018-03-25T02:05:26Z

Test build #88564 has finished for PR 20886 at commit ff57889.

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

SparkQA · 2018-03-25T02:11:31Z

Test build #88565 has finished for PR 20886 at commit 3b882fa.

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

SparkQA · 2018-03-30T10:49:00Z

Test build #88742 has finished for PR 20886 at commit 95bfb7b.

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

gengliangwang · 2018-03-30T11:23:48Z

@gatorsmile This PR is ready for review.

SparkQA · 2018-03-30T13:47:17Z

Test build #88749 has finished for PR 20886 at commit 0a35a84.

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

gengliangwang · 2018-03-30T23:26:47Z

retest this please.

SparkQA · 2018-03-31T03:15:43Z

Test build #88769 has finished for PR 20886 at commit 0a35a84.

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

SparkQA · 2018-04-01T17:32:43Z

Test build #88796 has finished for PR 20886 at commit f1de70c.

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

gatorsmile · 2018-04-01T22:09:11Z

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

Move them to DDLSuite? Change the format based on isUsingHiveMetastore?

gatorsmile · 2018-04-01T22:16:46Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

We always check defaultTablePath ? Is that possible, the table location points to the different location?

I think managed tables are always created on default path. So the check here should be correct.

gatorsmile · 2018-04-01T22:17:20Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

CatalogTable already contains TableIdentifier . What is the reason you do not use the one directly?

gatorsmile · 2018-04-01T22:17:54Z

This is a behavior change, we need to make it configurable and also document it in the migration guide.

SparkQA · 2018-04-02T02:40:01Z

Test build #88799 has finished for PR 20886 at commit f26c08c.

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

…ration guide

SparkQA · 2018-04-02T08:53:03Z

Test build #88807 has started for PR 20886 at commit 4d2cc31.

gengliangwang · 2018-04-03T04:11:15Z

retest this please.

gengliangwang · 2018-04-03T14:19:29Z

retest this please.

SparkQA · 2018-04-03T15:20:05Z

Test build #88846 has finished for PR 20886 at commit 13b6633.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2018-04-04T01:45:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

    .createWithDefault(false)

+  val ALLOW_NONEMPTY_MANAGED_TABLE_LOCATION =
+    buildConf("spark.sql.allowNonemptyManagedTableLocation")


spark.sql.allowCreateManagedTableUsingNonemptyLocation

Also this should be an internal conf

gatorsmile · 2018-04-04T01:46:05Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/SessionCatalog.scala

+      val fs = tableLocation.getFileSystem(hadoopConf)
+
+      if (fs.exists(tableLocation) && fs.listStatus(tableLocation).nonEmpty) {
+        throw new AnalysisException(s"Can not create the managed table('${table.identifier}')" +


Can not -> Not allowed to

SparkQA · 2018-04-04T02:46:33Z

Test build #88860 has finished for PR 20886 at commit 7a3311c.

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

SparkQA · 2018-04-04T07:05:01Z

Test build #88874 has finished for PR 20886 at commit 2b2973a.

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

gengliangwang · 2018-04-04T07:06:29Z

retest this please.

SparkQA · 2018-04-04T11:04:07Z

Test build #88878 has finished for PR 20886 at commit 2b2973a.

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

gatorsmile · 2018-04-06T03:18:58Z

LGTM

Thanks! Merged to master

…le should throw an exception ## What changes were proposed in this pull request? This PR is to finish apache#17272 This JIRA is a follow up work after SPARK-19583 As we discussed in that PR The following DDL for a managed table with an existed default location should throw an exception: CREATE TABLE ... (PARTITIONED BY ...) AS SELECT ... CREATE TABLE ... (PARTITIONED BY ...) Currently there are some situations which are not consist with above logic: CREATE TABLE ... (PARTITIONED BY ...) succeed with an existed default location situation: for both hive/datasource(with HiveExternalCatalog/InMemoryCatalog) CREATE TABLE ... (PARTITIONED BY ...) AS SELECT ... situation: hive table succeed with an existed default location This PR is going to make above two situations consist with the logic that it should throw an exception with an existed default location. ## How was this patch tested? unit test added Author: Gengliang Wang <[email protected]> Closes apache#20886 from gengliangwang/pr-17272.

…noreIfExists is true ## What changes were proposed in this pull request? In the PR apache#20886, I mistakenly check the table location only when `ignoreIfExists` is false, which was following the original deprecated PR. That was wrong. When `ignoreIfExists` is true and the target table doesn't exist, we should also check the table location. In other word, **`ignoreIfExists` has nothing to do with table location validation**. This is a follow-up PR to fix the mistake. ## How was this patch tested? Add one unit test. Author: Gengliang Wang <[email protected]> Closes apache#21001 from gengliangwang/SPARK-19724-followup.

gengliangwang force-pushed the pr-17272 branch from d584c9b to ff57889 Compare March 24, 2018 23:19

gengliangwang force-pushed the pr-17272 branch from 3b882fa to 95bfb7b Compare March 30, 2018 07:10

gengliangwang changed the title ~~[WIP][SPARK-19724][SQL]create a managed table with an existed default table should throw an exception~~ [SPARK-19724][SQL]create a managed table with an existed default table should throw an exception Mar 30, 2018

gengliangwang force-pushed the pr-17272 branch from 0a35a84 to f1de70c Compare April 1, 2018 13:52

gatorsmile reviewed Apr 1, 2018

View reviewed changes

gengliangwang added 2 commits April 2, 2018 07:37

validate managed table location

31f2efa

address comments

f26c08c

gengliangwang force-pushed the pr-17272 branch from f1de70c to f26c08c Compare April 2, 2018 00:37

gengliangwang added 2 commits April 2, 2018 16:22

fix test failure

9780438

add config spark.sql.allowNonemptyManagedTableLocation and modify mig…

4d2cc31

…ration guide

fix indent

13b6633

fix scala style

7a3311c

gatorsmile reviewed Apr 4, 2018

View reviewed changes

address comments

2b2973a

asfgit closed this in 249007e Apr 6, 2018

gengliangwang mentioned this pull request Apr 7, 2018

[SPARK-19724][SQL][FOLLOW-UP]Check location of managed table when ignoreIfExists is true #21001

Closed

[SPARK-19724][SQL]create a managed table with an existed default table should throw an exception #20886

[SPARK-19724][SQL]create a managed table with an existed default table should throw an exception #20886

Uh oh!

Conversation

gengliangwang commented Mar 22, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Mar 22, 2018

Uh oh!

SparkQA commented Mar 25, 2018

Uh oh!

SparkQA commented Mar 25, 2018

Uh oh!

SparkQA commented Mar 30, 2018

Uh oh!

gengliangwang commented Mar 30, 2018

Uh oh!

SparkQA commented Mar 30, 2018

Uh oh!

gengliangwang commented Mar 30, 2018

Uh oh!

SparkQA commented Mar 31, 2018

Uh oh!

SparkQA commented Apr 1, 2018

Uh oh!

gatorsmile Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

gatorsmile Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

gengliangwang Apr 2, 2018

Choose a reason for hiding this comment

Uh oh!

gatorsmile Apr 1, 2018

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Apr 1, 2018

Uh oh!

SparkQA commented Apr 2, 2018

Uh oh!

SparkQA commented Apr 2, 2018

Uh oh!

gengliangwang commented Apr 3, 2018

Uh oh!

gengliangwang commented Apr 3, 2018

Uh oh!

SparkQA commented Apr 3, 2018

Uh oh!

gatorsmile Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

gatorsmile Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Apr 4, 2018

Uh oh!

SparkQA commented Apr 4, 2018

Uh oh!

gengliangwang commented Apr 4, 2018

Uh oh!

SparkQA commented Apr 4, 2018

Uh oh!

gatorsmile commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gatorsmile commented Apr 6, 2018 •

edited

Loading