[SPARK-17353] [SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command #14531

gatorsmile · 2016-08-08T04:29:53Z

What changes were proposed in this pull request?

The existing CREATE TABLE LIKE command has multiple issues:

The generated table is non-empty when the source table is a data source table. The major reason is the data source table is using the table property path to store the location of table contents. Currently, we keep it unchanged. Thus, we still create the same table with the same location.
The table type of the generated table is EXTERNAL when the source table is an external Hive Serde table. Currently, we explicitly set it to MANAGED, but Hive is checking the table property EXTERNAL to decide whether the table is EXTERNAL or not. (See https://github.com/apache/hive/blob/master/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1407-L1408) Thus, the created table is still EXTERNAL.
When the source table is a VIEW, the metadata of the generated table contains the original view text and view original text. So far, this does not break anything, but it could cause something wrong in Hive. (For example, https://github.com/apache/hive/blob/master/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1405-L1406)
The issue regarding the table comment. To follow what Hive does, the table comment should be cleaned, but the column comments should be still kept.
The INDEX table is not supported. Thus, we should throw an exception in this case.
owner should not be retained. ToHiveTable set it here no matter which value we set in CatalogTable. We set it to an empty string for avoiding the confusing output in Explain.
Add a support for temp tables
Like Hive, we should not copy the table properties from the source table to the created table, especially for the statistics-related properties, which could be wrong in the created table.
unsupportedFeatures should not be copied from the source table. The created table does not have these unsupported features.
When the type of source table is a view, the target table is using the default format of data source tables: spark.sql.sources.default.

This PR is to fix the above issues.

How was this patch tested?

Improve the test coverage by adding more test cases

SparkQA · 2016-08-08T06:18:53Z

Test build #63339 has finished for PR 14531 at commit 1eb40e0.

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

gatorsmile · 2016-08-08T20:14:54Z

cc @yhuai @cloud-fan @rxin @liancheng @andrewor14 @hvanhovell

Question: Should we support CREATE TABLE LIKE on INDEX TABLE?

Hive does support it. However, it sounds like this is not considered in all the DDL statements, e.g., SHOW CREATE TABLE. If we want to support it, we need test cases for verifying what we did is right. Please let me know whether we should do it or not.

Thanks!

yhuai · 2016-08-08T20:23:29Z

We do not support index tables at all (you can not create such a table). Let's not add the support right now.

SparkQA · 2016-08-08T20:33:48Z

Test build #63372 has finished for PR 14531 at commit d0e9217.

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

SparkQA · 2016-08-09T00:07:04Z

Test build #63385 has finished for PR 14531 at commit 29e17a2.

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

cloud-fan · 2016-08-09T00:42:36Z

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

+    // (metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1095-L1105)
+    // Table comment is stored as a table property. To clean it, we also should remove them.
+    val newTableProp =
+      sourceTableDesc.properties.filterKeys(key => key != "EXTERNAL" && key != "comment")


Hive's behaviour is weird, does it prefer the EXTERNAL table property rather than the table type field?

That sounds true! See another fix by @yhuai in toHiveTable:

spark/sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala

Lines 764 to 770 in e679bc3

// For EXTERNAL_TABLE, we also need to set EXTERNAL field in the table properties.

// Otherwise, Hive metastore will change the table to a MANAGED_TABLE.

// (metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1095-L1105)

hiveTable.setTableType(table.tableType match {

case CatalogTableType.EXTERNAL =>

hiveTable.setProperty("EXTERNAL", "TRUE")

HiveTableType.EXTERNAL_TABLE

cloud-fan · 2016-08-09T01:15:50Z

Can we improve the class doc for CreateTableLikeCommand to explain the expected behaviours? thanks!

gatorsmile · 2016-08-09T01:19:14Z

Sure, will do it. Thanks!

SparkQA · 2016-08-09T02:23:57Z

Test build #63393 has finished for PR 14531 at commit 8434777.

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

cloud-fan · 2016-08-09T04:55:18Z

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

+        sourceTableDesc.storage.properties
+      }
+    val newTableDesc =
+      sourceTableDesc.copy(


Is it easier to create a new table? How many fields we need to retain?

Almost the same. The following attributes need to be kept: storage, schema, provider, partitionColumnNames, bucketSpec, properties, and unsupportedFeatures.

Found another bug... owner should be replaced...

which fields in storage we should retain?

I think it's more clear to list all the fields that need to retain in class doc.

Great idea! Just do it in the lines: https://github.com/gatorsmile/spark/blob/6180e80c2bc69d23441b752f78f1ca94071dd0ba/sql/core/src/main/scala/org/apache/spark/sql/execution/command/tables.scala#L64-L66

I'd like to create a new CatalogTable here and copy the fields explicitly, to match the class doc.

Sure, will do it.

SparkQA · 2016-08-09T06:32:26Z

Test build #63413 has finished for PR 14531 at commit b820be8.

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

SparkQA · 2016-08-09T19:20:11Z

Test build #63454 has finished for PR 14531 at commit 45b51d1.

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

SparkQA · 2016-08-09T22:08:38Z

Test build #63457 has finished for PR 14531 at commit 6180e80.

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

cloud-fan · 2016-08-10T05:08:27Z

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

+ * the table comment is always empty but the column comments are identical to the ones defined
+ * in the source table.
+ *
+ * The CatalogTable attributes copied from the source table include storage(inputFormat,


I think these are all of them, we can use are instead of include

rxin · 2016-08-10T05:13:12Z

Why can't the source table be a temp table? Also why not copy the table comment? Is it the same behavior in Hive / Postgres?

SparkQA · 2016-08-31T18:40:16Z

Test build #64725 has finished for PR 14531 at commit ba1b69d.

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

cloud-fan · 2016-09-01T01:22:38Z

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

+
+    // Storage format
+    val newStorage =
+      if (sourceTableType == CatalogTableType.VIEW) {


I'd like to create data source table if the source table is view.

I see. No need to strictly follow Hive.

FYI, we are having two different default formats: #14430.

cloud-fan · 2016-09-01T04:01:00Z

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

+      "the view text and original text in the created table must be empty")
+    // The location of created table should not be empty. Although Spark SQL does not set it,
+    // when creating it, Hive populates it.
+    assert(targetTable.storage.locationUri.nonEmpty,


what are we checking here? a specific behaviour of hive metastore? I mean, other ExternalCatalog may not need to populate it.

Yeah, we can remove these Hive-specific checking.

SparkQA · 2016-09-01T05:27:10Z

Test build #64754 has finished for PR 14531 at commit 4ce96e6.

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

cloud-fan · 2016-09-01T06:08:25Z

LGTM, pending jenkins

SparkQA · 2016-09-01T08:15:49Z

Test build #64763 has finished for PR 14531 at commit 4bcb306.

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

cloud-fan · 2016-09-01T08:39:15Z

merging to master! @gatorsmile can you send a new PR to backport it to 2.0? thanks!

gatorsmile · 2016-09-01T14:55:40Z

Sure, will do it. Thanks!

…le bugs in CREATE TABLE LIKE command ### What changes were proposed in this pull request? This PR is to backport #14531. The existing `CREATE TABLE LIKE` command has multiple issues: - The generated table is non-empty when the source table is a data source table. The major reason is the data source table is using the table property `path` to store the location of table contents. Currently, we keep it unchanged. Thus, we still create the same table with the same location. - The table type of the generated table is `EXTERNAL` when the source table is an external Hive Serde table. Currently, we explicitly set it to `MANAGED`, but Hive is checking the table property `EXTERNAL` to decide whether the table is `EXTERNAL` or not. (See https://github.com/apache/hive/blob/master/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1407-L1408) Thus, the created table is still `EXTERNAL`. - When the source table is a `VIEW`, the metadata of the generated table contains the original view text and view original text. So far, this does not break anything, but it could cause something wrong in Hive. (For example, https://github.com/apache/hive/blob/master/metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1405-L1406) - The issue regarding the table `comment`. To follow what Hive does, the table comment should be cleaned, but the column comments should be still kept. - The `INDEX` table is not supported. Thus, we should throw an exception in this case. - `owner` should not be retained. `ToHiveTable` set it [here](https://github.com/apache/spark/blob/e679bc3c1cd418ef0025d2ecbc547c9660cac433/sql/hive/src/main/scala/org/apache/spark/sql/hive/client/HiveClientImpl.scala#L793) no matter which value we set in `CatalogTable`. We set it to an empty string for avoiding the confusing output in Explain. - Add a support for temp tables - Like Hive, we should not copy the table properties from the source table to the created table, especially for the statistics-related properties, which could be wrong in the created table. - `unsupportedFeatures` should not be copied from the source table. The created table does not have these unsupported features. - When the type of source table is a view, the target table is using the default format of data source tables: `spark.sql.sources.default`. This PR is to fix the above issues. ### How was this patch tested? Improve the test coverage by adding more test cases Author: gatorsmile <gatorsmile@gmail.com> Closes #14946 from gatorsmile/createTableLike20.

sitalkedia · 2016-10-04T18:55:41Z

@rxin, @gatorsmile - This PR breaks our jobs with CREATE TABLE LIKE command since the table properties from the source tables are not propagated to the new table anymore. We have table properties like table-retention which is expected to be copied over when the new table is created. How do you think we can fix this issue?

gatorsmile · 2016-10-04T19:00:08Z

@sitalkedia So far, you can set table properties to the new table by using the DDL command.

@rxin @cloud-fan @yhuai Let me know if you need me to submit a PR to make such a change. I think what @sitalkedia said is valid.

cloud-fan · 2016-10-05T07:35:18Z

IIUC, before 2.0, we use hive to run CREATE TABLE LIKE, and hive doesn't include the table properties. So this PR actually fixed a regression in 2.0, I think we should keep this behaviour.

cc @rxin @yhuai

rxin · 2016-10-05T07:46:28Z

@cloud-fan are you sure Hive doesn't copy the table properties? How would @sitalkedia's case work if it does not copy?

cloud-fan · 2016-10-05T08:14:35Z

I suspect @sitalkedia built his application based on 2.0 and got broken in 2.0.1, @sitalkedia is that true?

@gatorsmile can you double check that Hive doesn't copy the table properties?

gatorsmile · 2016-10-05T15:40:42Z

@cloud-fan Hive does not copy the table properties in CREATE TABLE LIKE

sitalkedia · 2016-10-05T18:22:56Z

@gatorsmile - I looked into Hive code base a bit and it looks like Hive provides a way to specify a whitelisted set of properties which are copied for the newly created table. Spark should also provide similar flexibility to mimic hive behavior - https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/exec/DDLTask.java#L4140

gatorsmile · 2016-10-07T03:54:28Z

@sitalkedia Yeah, I saw it. Thank you for investigation. Normally, we do not want to add many configuration flags. It hurts the usability. Let @rxin make a decision whether we should add another flag or not.

rxin · 2016-10-16T04:45:49Z

@gatorsmile / @sitalkedia that idea sounds good (similar to Hive's)

gatorsmile · 2016-10-16T04:47:53Z

Sure, will submit a PR for it. Thanks!

fix

1eb40e0

gatorsmile changed the title ~~[SPARK-16943] [SPAR] [SQL] Fix multiple bugs in CREATE TABLE LIKE command~~ [SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command Aug 8, 2016

gatorsmile changed the title ~~[SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command~~ [SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command [WIP] Aug 8, 2016

fix the issue in VIEW

d0e9217

fix the issue in comment and index table.

29e17a2

gatorsmile added 2 commits August 8, 2016 17:07

code clean

9d70eb5

code clean

8434777

gatorsmile changed the title ~~[SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command [WIP]~~ [SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command Aug 9, 2016

cloud-fan reviewed Aug 9, 2016
View reviewed changes

add comments and improve the test cases

b820be8

cloud-fan reviewed Aug 9, 2016
View reviewed changes

address comments

45b51d1

fixed the style.

6180e80

cloud-fan reviewed Aug 10, 2016
View reviewed changes

cloud-fan reviewed Sep 1, 2016
View reviewed changes

address comments

4ce96e6

gatorsmile changed the title ~~[SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command~~ [SPARK-17353] [SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command Sep 1, 2016

cloud-fan reviewed Sep 1, 2016
View reviewed changes

address comments.

4bcb306

asfgit closed this in 1f06a5b Sep 1, 2016

gatorsmile mentioned this pull request Oct 15, 2016

[SPARK-17947] [SQL] Add Doc and Comment about spark.sql.debug #15494

Closed

	// For EXTERNAL_TABLE, we also need to set EXTERNAL field in the table properties.
	// Otherwise, Hive metastore will change the table to a MANAGED_TABLE.
	// (metastore/src/java/org/apache/hadoop/hive/metastore/ObjectStore.java#L1095-L1105)
	hiveTable.setTableType(table.tableType match {
	case CatalogTableType.EXTERNAL =>
	hiveTable.setProperty("EXTERNAL", "TRUE")
	HiveTableType.EXTERNAL_TABLE

[SPARK-17353] [SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command #14531

[SPARK-17353] [SPARK-16943] [SPARK-16942] [SQL] Fix multiple bugs in CREATE TABLE LIKE command #14531

Uh oh!

Conversation

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

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 8, 2016

Uh oh!

gatorsmile commented Aug 8, 2016

Uh oh!

yhuai commented Aug 8, 2016

Uh oh!

SparkQA commented Aug 8, 2016

Uh oh!

SparkQA commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

gatorsmile commented Aug 9, 2016

Uh oh!

SparkQA commented Aug 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!

SparkQA commented Aug 9, 2016

Uh oh!

SparkQA commented Aug 9, 2016

Uh oh!

SparkQA commented Aug 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rxin commented Aug 10, 2016

Uh oh!

SparkQA commented Aug 31, 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!

SparkQA commented Sep 1, 2016

Uh oh!

cloud-fan commented Sep 1, 2016

Uh oh!

SparkQA commented Sep 1, 2016

Uh oh!

cloud-fan commented Sep 1, 2016

Uh oh!

gatorsmile commented Sep 1, 2016

Uh oh!

sitalkedia commented Oct 4, 2016

Uh oh!

gatorsmile commented Oct 4, 2016

Uh oh!

gatorsmile commented Aug 8, 2016 •

edited

Loading

cloud-fan commented Aug 9, 2016 •

edited

Loading