[SPARK-16879][SQL] unify logical plans for CREATE TABLE and CTAS #14482

cloud-fan · 2016-08-03T15:50:10Z

What changes were proposed in this pull request?

we have various logical plans for CREATE TABLE and CTAS: CreateTableUsing, CreateTableUsingAsSelect, CreateHiveTableAsSelectLogicalPlan. This PR unifies them to reduce the complexity and centralize the error handling.

How was this patch tested?

existing tests

cloud-fan · 2016-08-03T15:50:34Z

cc @liancheng @gatorsmile @yhuai @rxin

SparkQA · 2016-08-03T16:06:44Z

Test build #63176 has finished for PR 14482 at commit a11d122.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2016-08-03T16:22:04Z

is it a MIMA issue? Although I moved the SaveMode class, it's still in the same package.

yhuai · 2016-08-03T17:54:19Z

SaveMode is a public API. We cannot move it to catalyst.

hvanhovell · 2016-08-03T18:05:29Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/CheckAnalysis.scala

Move this somewhere we can reuse it.

cloud-fan · 2016-08-04T05:32:16Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+    //   * partition, bucket and sort column names must exist in table definition.
+    //   * partition, bucket and sort column names can't be duplicated.
+    //   * can't use all table columns as partition columns.
+    //   * partition columns' type must be AtomicType.


cc @gatorsmile , I think all this checks are general and can be applied to hive serde tables too.

Yeah, these checking are general to both types. The only issue is here:

partition, bucket and sort column names must exist in table definition.

However, the schema value from the parser combines both. Thus, Hive table creation pass the checks

val schema = StructType(dataCols ++ partitionCols)

because hive CREATE TABLE syntax is special, it will never hit this exception partition column names must exist in table definition. Instead, hive may specify partition column names that exist in table definition, and will hit column names in table definition can't be duplicated.

cloud-fan · 2016-08-04T05:37:00Z

cc @gatorsmile , IIRC, you have some PRs about error handling. After this PR, we can have a centre place for basic error handling, is it good enough for all the error cases you found?

cloud-fan · 2016-08-04T05:38:17Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/rules.scala

+        // Since we are saving table metadata to metastore, we should make sure the table name
+        // and database name don't break some common restrictions, e.g. special chars except
+        // underscore are not allowed.
+        val pattern = Pattern.compile("[\\w_]+")


cc @hvanhovell , I think this is the only place that we need this check, as CreateTable is the only plan that can save a table metadata into metastore. what do you think?

Compiling a pattern can be expensive and the Pattern produced is reusable; that is all.

SparkQA · 2016-08-04T06:36:39Z

Test build #63209 has finished for PR 14482 at commit 108e385.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CreateTable(tableDesc: CatalogTable, mode: SaveMode, query: Option[LogicalPlan])
- case class PreprocessDDL(conf: SQLConf) extends Rule[LogicalPlan]

SparkQA · 2016-08-04T06:45:20Z

Test build #63211 has finished for PR 14482 at commit ec47911.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class CreateTable(tableDesc: CatalogTable, mode: SaveMode, query: Option[LogicalPlan])
- case class PreprocessDDL(conf: SQLConf) extends Rule[LogicalPlan]

hvanhovell · 2016-08-04T07:33:08Z

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

+      tableType = CatalogTableType.MANAGED,
+      storage = CatalogStorageFormat.empty.copy(properties = options),
+      schema = schema.getOrElse(new StructType),
+      provider = Some(provider),


Is it still possible that a provide is None?

well, just look at the codes around, it's possible that provider is null. However, if we also look at the antlr file, the provider must be specified. The previous code doesn't check the null either, and will throw NPE somewhere if it's null. So I think we should use Some here to follow this behavior.

It was a more general question which you have answered, sorry about that.

In this case the provider cannot be null: the grammar requires a provider and the call ctx.tableProvider.qualifiedName.getText would result in a NPE if it were null.

SparkQA · 2016-08-04T08:15:02Z

Test build #63218 has finished for PR 14482 at commit 7c670b7.

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

SparkQA · 2016-08-04T08:31:12Z

Test build #63219 has finished for PR 14482 at commit 2662bf1.

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

SparkQA · 2016-08-04T09:54:22Z

Test build #63221 has finished for PR 14482 at commit 8db79e4.

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

SparkQA · 2016-08-04T13:58:59Z

Test build #63226 has finished for PR 14482 at commit 8f53cf2.

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

SparkQA · 2016-08-04T15:24:27Z

Test build #63227 has finished for PR 14482 at commit 2933c58.

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

gatorsmile · 2016-08-04T16:37:34Z

sql/core/src/main/scala/org/apache/spark/sql/internal/CatalogImpl.scala

+      schema = schema,
+      provider = Some(source)
+    )
+    val plan = CreateTable(tableDesc, SaveMode.ErrorIfExists, None)


This change will have an external impact. The value of source is input from users. That means it could be "hive". If so, users can create a Hive serde Table. If this is desired, we need to document it and test it?

If so, we need to populate the location of storage. Maybe we just disable the option hive and issue an exception when users do it?

good catch! yea we should disable hive for now.

gatorsmile · 2016-08-04T20:08:08Z

@cloud-fan I just went over my related PRs. Two of them are contained, and thus I closed them. The other three are not covered. Please feel free to let me know what are the best way to deal with the remaining three. Thanks!

cloud-fan · 2016-08-05T05:25:14Z

@gatorsmile how about keep working on them after this PR is merged?

gatorsmile · 2016-08-05T05:46:27Z

@cloud-fan Sure, will do. Thanks!

SparkQA · 2016-08-05T07:31:34Z

Test build #63256 has finished for PR 14482 at commit f49ef22.

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

hvanhovell · 2016-08-05T08:49:37Z

LGTM - merging to master. Thanks!

…ake CatalogTable ## What changes were proposed in this pull request? This is kind of a follow-up of #14482 . As we put `CatalogTable` in the logical plan directly, it makes sense to let physical plans take `CatalogTable` directly, instead of extracting some fields of `CatalogTable` in planner and then construct a new `CatalogTable` in physical plan. ## How was this patch tested? existing tests. Author: Wenchen Fan <[email protected]> Closes #14823 from cloud-fan/create-table.

hvanhovell reviewed Aug 3, 2016
View reviewed changes

cloud-fan force-pushed the table branch from a11d122 to 108e385 Compare August 4, 2016 05:26

unify logical plans for CREATE TABLE and CTAS

ec47911

cloud-fan force-pushed the table branch from 108e385 to ec47911 Compare August 4, 2016 05:28

cloud-fan reviewed Aug 4, 2016
View reviewed changes

cloud-fan added 2 commits August 4, 2016 14:56

extends Command

7c670b7

add one more test

2662bf1

hvanhovell reviewed Aug 4, 2016
View reviewed changes

address comments

8db79e4

fix hive...

2933c58

cloud-fan force-pushed the table branch from 8f53cf2 to 2933c58 Compare August 4, 2016 13:25

gatorsmile reviewed Aug 4, 2016
View reviewed changes

address comments

f49ef22

asfgit closed this in 5effc01 Aug 5, 2016

cloud-fan mentioned this pull request Aug 26, 2016

[SPARK-17257][SQL] the physical plan of CREATE TABLE or CTAS should take CatalogTable #14823

Closed

xwu0226 mentioned this pull request Feb 10, 2017

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

Closed

[SPARK-16879][SQL] unify logical plans for CREATE TABLE and CTAS #14482

[SPARK-16879][SQL] unify logical plans for CREATE TABLE and CTAS #14482

Uh oh!

Conversation

cloud-fan commented Aug 3, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Aug 3, 2016

Uh oh!

SparkQA commented Aug 3, 2016

Uh oh!

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

Uh oh!

Uh oh!

yhuai commented Aug 3, 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!

cloud-fan commented Aug 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 4, 2016

Uh oh!

SparkQA commented Aug 4, 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 Aug 4, 2016

Uh oh!

SparkQA commented Aug 4, 2016

Uh oh!

SparkQA commented Aug 4, 2016

Uh oh!

SparkQA commented Aug 4, 2016

Uh oh!

SparkQA commented Aug 4, 2016

Uh oh!

gatorsmile Aug 4, 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!

cloud-fan Aug 5, 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!

gatorsmile commented Aug 4, 2016

Uh oh!

cloud-fan commented Aug 5, 2016

Uh oh!

gatorsmile commented Aug 5, 2016

Uh oh!

SparkQA commented Aug 5, 2016

Uh oh!

hvanhovell commented Aug 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cloud-fan commented Aug 3, 2016 •

edited

Loading

gatorsmile Aug 4, 2016 •

edited

Loading

cloud-fan Aug 5, 2016 •

edited

Loading