[SPARK-17990][SPARK-18302][SQL] correct several partition related behaviours of ExternalCatalog #15797

cloud-fan · 2016-11-07T16:04:56Z

What changes were proposed in this pull request?

This PR corrects several partition related behaviors of ExternalCatalog:

default partition location should not always lower case the partition column names in path string(fix HiveExternalCatalog)
rename partition should not always lower case the partition column names in updated partition path string(fix HiveExternalCatalog)
rename partition should update the partition location only for managed table(fix InMemoryCatalog)
create partition with existing directory should be fine(fix InMemoryCatalog)
create partition with non-existing directory should create that directory(fix InMemoryCatalog)
drop partition from external table should not delete the directory(fix InMemoryCatalog)

How was this patch tested?

new tests in ExternalCatalogSuite

cloud-fan · 2016-11-07T16:07:28Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

This method just copies the code in the above if branch: https://github.com/apache/spark/pull/15797/files#diff-159191585e10542f013cb3a714f26075R199

cloud-fan · 2016-11-07T16:09:06Z

cc @mallman this should fix https://issues.apache.org/jira/browse/SPARK-17990
cc @ericl @yhuai @gatorsmile

SparkQA · 2016-11-07T16:59:00Z

Test build #68281 has finished for PR 15797 at commit e0e012c.

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

mallman · 2016-11-07T17:07:40Z

@cloud-fan Thanks for taking this on. I will test out a build from your branch to verify that it fixes the problem I was seeing.

mallman · 2016-11-07T18:03:47Z

I tried your patch and it works for me.

ericl · 2016-11-07T20:55:28Z

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

Do we need to escape these, as in PartitioningUtils.escapePathName?

ericl · 2016-11-07T21:05:36Z

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

Should we pull this into PartitioningUtils?

ericl · 2016-11-07T21:20:44Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

Would this apply to external tables as well?

no, Hive only update the partition location when renaming partitions for managed table.

ericl · 2016-11-07T21:23:01Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

What if it had a custom location?

RENAME PARTITION can only change the partition spec, not partition path. The SQL syntax is :ALTER TABLE table_name PARTITION partition_spec RENAME TO PARTITION partition_spec;, users can't specify partition location

if the partition to be renamed has custom location, we will move the directory too.

Actually there is no such a concept called custom location for partitions. Partitions always have a location, if users didn't specify, we will generate a default one. The generated one and specified one are treated equally.

cloud-fan · 2016-11-08T01:40:06Z

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

+
+import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec
+
+object ExternalCatalogUtils {


@ericl PartitioningUtils is not accessible in catalyst module, so I created this.

SparkQA · 2016-11-08T02:30:17Z

Test build #68309 has finished for PR 15797 at commit aa2536f.

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

cloud-fan · 2016-11-08T03:31:04Z

retest this please

SparkQA · 2016-11-08T05:00:27Z

Test build #68314 has finished for PR 15797 at commit aa2536f.

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

SparkQA · 2016-11-08T07:10:04Z

Test build #68322 has finished for PR 15797 at commit dddee47.

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

gatorsmile · 2016-11-08T07:23:14Z

retest this please

gatorsmile · 2016-11-08T07:29:31Z

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

-    builder.toString()
-  }
-
  def unescapePathName(path: String): String = {


If escapePathName is moved, I think we also need to move unescapePathName to the same file?

gatorsmile · 2016-11-08T07:36:17Z

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

    val catalog = newBasicCatalog()
    val db = catalog.getDatabase("db1")
    val table = CatalogTable(
-      identifier = TableIdentifier("my_table", Some("db1")),


Could we keep the original names unchanged? Now, the table/database names allow alphanumeric characters and underscores. Thus, I think they are named using underscore on purpose?

Actually this test was written by me, and I picked myTable at the first time, but changed to my_table because of this bug.

gatorsmile · 2016-11-08T07:37:52Z

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

    throw new AnalysisException(s"table $identifier did not specify database")
  }

+  def location: String = storage.locationUri.getOrElse {


Like the other functions in this file, add the function descriptions too?

SparkQA · 2016-11-08T08:49:00Z

Test build #68329 has finished for PR 15797 at commit dddee47.

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

SparkQA · 2016-11-08T11:16:47Z

Test build #68336 has finished for PR 15797 at commit f85bb27.

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

SparkQA · 2016-11-08T17:07:10Z

Test build #68343 has finished for PR 15797 at commit d965a56.

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

SparkQA · 2016-11-09T02:23:51Z

Test build #68376 has finished for PR 15797 at commit d939b37.

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

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

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+      val fs = tablePath.getFileSystem(hadoopConf)
+      val newParts = newSpecs.map { spec =>
+        val partition = client.getPartition(db, table, lowerCasePartitionSpec(spec))
+        val wrongPath = new Path(partition.storage.locationUri.get)


How about creating a function location for partitions too, just like what this PR did for table's location?

gatorsmile · 2016-11-09T06:59:23Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+          fs.rename(wrongPath, rightPath)
+        } catch {
+          case e: IOException =>
+            throw new SparkException(s"Unable to rename partition path $wrongPath", e)


Maybe we also need to add rightPath here

SparkQA · 2016-11-09T08:08:55Z

Test build #68392 has finished for PR 15797 at commit 9dbc3f1.

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

ericl · 2016-11-09T23:01:51Z

I just tested this PR with #15814 modified with mixed case partition names in unit tests, and it fixes the issues there too. I'll leave it to @gatorsmile to lgtm since he has a few more comments.

SparkQA · 2016-11-10T04:16:54Z

Test build #68435 has finished for PR 15797 at commit 56f3073.

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

gatorsmile · 2016-11-10T05:17:09Z

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

+        val dir = new Path(tableMeta.location)
        try {
          val fs = dir.getFileSystem(hadoopConfig)
          fs.delete(dir, true)


Since the deletion is recursive and thus it can delete the lcation/directory of a partitioned table. I checked the implementation of delete. It sounds like it does not guaranttee the atomicity. The deletion could be partial. We might need to also put a TODO here, like what we did for dropPartition

it's minor and not related to this PR, I'd like to not touch it.

gatorsmile · 2016-11-10T06:39:38Z

In renamePartition of HiveExternalCatalog, we update the file names to preserve the case after renaming partitions. How about another command loadPartition?

More generally, if we insert new data into a partitioned table, Hive will create a new directory for non-existent partitions. These newly created file path are not case-preserving, right?

gatorsmile · 2016-11-10T06:43:36Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+    val hasUpperCasePartitionColumn = partitionColumnNames.exists(col => col.toLowerCase != col)
+    if (tableMeta.tableType == MANAGED && hasUpperCasePartitionColumn) {
+      val tablePath = new Path(tableMeta.location)
+      val fs = tablePath.getFileSystem(hadoopConf)


In the other functions, the function call getFileSystem is also part of the try-catch block. Do we need to make it consistent with others?

cloud-fan · 2016-11-10T09:31:13Z

In renamePartition of HiveExternalCatalog, we update the file names to preserve the case after renaming partitions. How about another command loadPartition? More generally, if we insert new data into a partitioned table, Hive will create a new directory for non-existent partitions. These newly created file path are not case-preserving, right?

This PR tries its best to fix some partition related behaviours, I think it's OK to leave some hard works for follow-ups.

SparkQA · 2016-11-10T12:11:31Z

Test build #68461 has finished for PR 15797 at commit 0a15edc.

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

gatorsmile · 2016-11-10T17:43:58Z

LGTM

rxin · 2016-11-10T21:42:00Z

Merging in master/branch-2.1.

…aviours of ExternalCatalog ## What changes were proposed in this pull request? This PR corrects several partition related behaviors of `ExternalCatalog`: 1. default partition location should not always lower case the partition column names in path string(fix `HiveExternalCatalog`) 2. rename partition should not always lower case the partition column names in updated partition path string(fix `HiveExternalCatalog`) 3. rename partition should update the partition location only for managed table(fix `InMemoryCatalog`) 4. create partition with existing directory should be fine(fix `InMemoryCatalog`) 5. create partition with non-existing directory should create that directory(fix `InMemoryCatalog`) 6. drop partition from external table should not delete the directory(fix `InMemoryCatalog`) ## How was this patch tested? new tests in `ExternalCatalogSuite` Author: Wenchen Fan <wenchen@databricks.com> Closes #15797 from cloud-fan/partition. (cherry picked from commit 2f7461f) Signed-off-by: Reynold Xin <rxin@databricks.com>

…aviours of ExternalCatalog ## What changes were proposed in this pull request? This PR corrects several partition related behaviors of `ExternalCatalog`: 1. default partition location should not always lower case the partition column names in path string(fix `HiveExternalCatalog`) 2. rename partition should not always lower case the partition column names in updated partition path string(fix `HiveExternalCatalog`) 3. rename partition should update the partition location only for managed table(fix `InMemoryCatalog`) 4. create partition with existing directory should be fine(fix `InMemoryCatalog`) 5. create partition with non-existing directory should create that directory(fix `InMemoryCatalog`) 6. drop partition from external table should not delete the directory(fix `InMemoryCatalog`) ## How was this patch tested? new tests in `ExternalCatalogSuite` Author: Wenchen Fan <wenchen@databricks.com> Closes apache#15797 from cloud-fan/partition.

gatorsmile · 2017-01-28T08:54:16Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+        val rightPath = ExternalCatalogUtils.generatePartitionPath(
+          spec, partitionColumnNames, tablePath)
+        try {
+          tablePath.getFileSystem(hadoopConf).rename(wrongPath, rightPath)


Found an issue here... When we call rename, not all the file systems have the same behaviors. For example, on mac OS, when we doing this .../tbl/a=5/b=6 -> .../tbl/A=5/B=6 . The result is .../tbl/a=5/B=6. Thus, it is not recursive. However, the file system used in Jenkin does not have such an issue

good catch! let me think about how to fix it.

cloud-fan changed the title ~~correct several partition related behaviours of ExternalCatalog~~ [SPARK-17990][SPARK-18302][SQL] correct several partition related behaviours of ExternalCatalog Nov 7, 2016

cloud-fan commented Nov 7, 2016

View reviewed changes

ericl suggested changes Nov 7, 2016

View reviewed changes

correct several partition related behaviours of ExternalCatalog

aa2536f

cloud-fan force-pushed the partition branch from e0e012c to aa2536f Compare November 8, 2016 01:31

cloud-fan commented Nov 8, 2016

View reviewed changes

use Path

dddee47

gatorsmile reviewed Nov 8, 2016

View reviewed changes

cloud-fan added 2 commits November 8, 2016 17:19

address comments

e97c17f

fix bug

f85bb27

work around

d939b37

cloud-fan force-pushed the partition branch from d965a56 to d939b37 Compare November 9, 2016 00:09

fix test

9dbc3f1

gatorsmile reviewed Nov 9, 2016

View reviewed changes

ericl mentioned this pull request Nov 10, 2016

[SPARK-18185] Fix all forms of INSERT / OVERWRITE TABLE for Datasource tables #15814

Closed

fix style

56f3073

gatorsmile reviewed Nov 10, 2016

View reviewed changes

address comments

0a15edc

asfgit closed this in 2f7461f Nov 10, 2016

gatorsmile reviewed Jan 28, 2017

View reviewed changes


		import org.apache.spark.sql.catalyst.catalog.CatalogTypes.TablePartitionSpec

		object ExternalCatalogUtils {

[SPARK-17990][SPARK-18302][SQL] correct several partition related behaviours of ExternalCatalog #15797

[SPARK-17990][SPARK-18302][SQL] correct several partition related behaviours of ExternalCatalog #15797

Uh oh!

Conversation

cloud-fan commented Nov 7, 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!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

SparkQA commented Nov 7, 2016

Uh oh!

mallman commented Nov 7, 2016

Uh oh!

mallman commented Nov 7, 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 Nov 8, 2016

Uh oh!

cloud-fan commented Nov 8, 2016

Uh oh!

SparkQA commented Nov 8, 2016

Uh oh!

SparkQA commented Nov 8, 2016

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 8, 2016

Uh oh!

SparkQA commented Nov 8, 2016

Uh oh!

SparkQA commented Nov 8, 2016

Uh oh!

SparkQA commented Nov 9, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 9, 2016

Uh oh!

ericl commented Nov 9, 2016

Uh oh!

SparkQA commented Nov 10, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

cloud-fan commented Nov 7, 2016 •

edited

Loading

cloud-fan commented Nov 7, 2016 •

edited

Loading

gatorsmile commented Nov 10, 2016 •

edited

Loading

gatorsmile Jan 28, 2017 •

edited

Loading