[SPARK-18659] [SQL] Incorrect behaviors in overwrite table for datasource tables #16088

ericl · 2016-11-30T22:04:24Z

What changes were proposed in this pull request?

Two bugs are addressed here

INSERT OVERWRITE TABLE sometime crashed when catalog partition management was enabled. This was because when dropping partitions after an overwrite operation, the Hive client will attempt to delete the partition files. If the entire partition directory was dropped, this would fail. The PR fixes this by adding a flag to control whether the Hive client should attempt to delete files.
The static partition spec for OVERWRITE TABLE was not correctly resolved to the case-sensitive original partition names. This resulted in the entire table being overwritten if you did not correctly capitalize your partition names.

cc @yhuai @cloud-fan

How was this patch tested?

Unit tests. Surprisingly, the existing overwrite table tests did not catch these edge cases.

SparkQA · 2016-12-01T00:39:56Z

Test build #69432 has finished for PR 16088 at commit 4b6a4ee.

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

cloud-fan · 2016-12-01T01:16:54Z

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

    // partial partition spec.
    partSpecs.foreach { p =>
-      if (existingParts.contains(p) && shouldRemovePartitionLocation) {
+      if (existingParts.contains(p) && shouldRemovePartitionLocation && !retainData) {


shall we put the !retainData in the definition of shouldRemovePartitionLocation? which looks more logical

cloud-fan · 2016-12-01T01:18:01Z

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

      ignoreIfNotExists: Boolean,
-      purge: Boolean): Unit = {
+      purge: Boolean,
+      retainData: Boolean): Unit = {


shall we provide a default value? looks like most of the time we want it to be false

It seemed a little safer to make it mandatory to avoid some component forgetting to propagate it.

cloud-fan · 2016-12-01T01:20:05Z

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

      ignoreIfNotExists: Boolean,
-      purge: Boolean): Unit = withClient {
+      purge: Boolean,
+      deleteFiles: Boolean): Unit = withClient {


why change the parameter name here? it's called retainData right?

SparkQA · 2016-12-02T00:35:11Z

Test build #69504 has finished for PR 16088 at commit f4356f0.

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

cloud-fan · 2016-12-02T03:19:36Z

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

              AlterTableDropPartitionCommand(
                l.catalogTable.get.identifier, deletedPartitions.toSeq,
-                ifExists = true, purge = true).run(t.sparkSession)
+                ifExists = true, purge = true,


an unrelated question, when dropping partitions here, do we have to set purge = true?

I think it doesn't matter since we don't delete files.

then can we set it to false? DROP PARTITION ... PURGE is not supported in hive 0.13, setting it to false can make the partition related functions still work in older hive versions.

cloud-fan · 2016-12-02T03:20:44Z

LGTM pending jenkins

SparkQA · 2016-12-02T04:46:48Z

Test build #69532 has finished for PR 16088 at commit b79cef4.

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

cloud-fan · 2016-12-02T04:52:29Z

retest this please

SparkQA · 2016-12-02T06:28:33Z

Test build #69537 has finished for PR 16088 at commit a9f9710.

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

SparkQA · 2016-12-02T07:35:53Z

Test build #69543 has finished for PR 16088 at commit a9f9710.

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

…rce tables ## What changes were proposed in this pull request? Two bugs are addressed here 1. INSERT OVERWRITE TABLE sometime crashed when catalog partition management was enabled. This was because when dropping partitions after an overwrite operation, the Hive client will attempt to delete the partition files. If the entire partition directory was dropped, this would fail. The PR fixes this by adding a flag to control whether the Hive client should attempt to delete files. 2. The static partition spec for OVERWRITE TABLE was not correctly resolved to the case-sensitive original partition names. This resulted in the entire table being overwritten if you did not correctly capitalize your partition names. cc yhuai cloud-fan ## How was this patch tested? Unit tests. Surprisingly, the existing overwrite table tests did not catch these edge cases. Author: Eric Liang <[email protected]> Closes #16088 from ericl/spark-18659. (cherry picked from commit 7935c84) Signed-off-by: Wenchen Fan <[email protected]>

cloud-fan · 2016-12-02T14:00:06Z

thanks, merging to master/2.1!

…rce tables ## What changes were proposed in this pull request? Two bugs are addressed here 1. INSERT OVERWRITE TABLE sometime crashed when catalog partition management was enabled. This was because when dropping partitions after an overwrite operation, the Hive client will attempt to delete the partition files. If the entire partition directory was dropped, this would fail. The PR fixes this by adding a flag to control whether the Hive client should attempt to delete files. 2. The static partition spec for OVERWRITE TABLE was not correctly resolved to the case-sensitive original partition names. This resulted in the entire table being overwritten if you did not correctly capitalize your partition names. cc yhuai cloud-fan ## How was this patch tested? Unit tests. Surprisingly, the existing overwrite table tests did not catch these edge cases. Author: Eric Liang <[email protected]> Closes apache#16088 from ericl/spark-18659.

ericl added 2 commits November 30, 2016 13:59

Wed Nov 30 13:59:17 PST 2016

f3875ae

Wed Nov 30 14:05:58 PST 2016

4b6a4ee

cloud-fan reviewed Dec 1, 2016

View reviewed changes

ericl added 2 commits December 1, 2016 13:18

Thu Dec 1 13:18:41 PST 2016

c68bc81

Merge branch 'master' into spark-18659

f4356f0

Merge branch 'master' into spark-18659

b79cef4

cloud-fan reviewed Dec 2, 2016

View reviewed changes

Thu Dec 1 19:36:07 PST 2016

a9f9710

asfgit closed this in 7935c84 Dec 2, 2016

[SPARK-18659] [SQL] Incorrect behaviors in overwrite table for datasource tables #16088

[SPARK-18659] [SQL] Incorrect behaviors in overwrite table for datasource tables #16088

Uh oh!

Conversation

ericl commented Nov 30, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Dec 1, 2016

Uh oh!

cloud-fan Dec 1, 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!

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 Dec 2, 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 Dec 2, 2016

Uh oh!

SparkQA commented Dec 2, 2016

Uh oh!

cloud-fan commented Dec 2, 2016

Uh oh!

SparkQA commented Dec 2, 2016

Uh oh!

SparkQA commented Dec 2, 2016

Uh oh!

cloud-fan commented Dec 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cloud-fan Dec 1, 2016 •

edited

Loading