[SPARK-18183] [SPARK-18184] Fix INSERT [INTO|OVERWRITE] TABLE ... PARTITION for Datasource tables #15705

ericl · 2016-10-31T23:42:57Z

What changes were proposed in this pull request?

There are a couple issues with the current 2.1 behavior when inserting into Datasource tables with partitions managed by Hive.

(1) OVERWRITE TABLE ... PARTITION will actually overwrite the entire table instead of just the specified partition.
(2) INSERT|OVERWRITE does not work with partitions that have custom locations.

This PR fixes both of these issues for Datasource tables managed by Hive. The behavior for legacy tables or when manageFilesourcePartitions = false is unchanged.

There is one other issue in that INSERT OVERWRITE with dynamic partitions will overwrite the entire table instead of just the updated partitions, but this behavior is pretty complicated to implement for Datasource tables. We should address that in a future release.

How was this patch tested?

Unit tests.

ericl · 2016-10-31T23:43:52Z

cc @cloud-fan @yhuai

SparkQA · 2016-10-31T23:55:08Z

Test build #67846 has finished for PR 15705 at commit 669e6cc.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2016-11-01T02:18:22Z

Test build #67848 has finished for PR 15705 at commit fec7c9e.

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

SparkQA · 2016-11-01T02:32:15Z

Test build #67849 has finished for PR 15705 at commit 07c6787.

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

cloud-fan · 2016-11-01T02:59:16Z

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

+        val path = new Path(p.storage.locationUri.get)
+        val fs = path.getFileSystem(hadoopConf)
+        PartitionPath(
+          p.toRow(partitionSchema), path.makeQualified(fs.getUri, fs.getWorkingDirectory))


why this change? Doesn't new Path qualify the path string?

Apparently not. The unit test actually fails if you do that, since the path seems to be missing the file: prefix and we fail to find the files in the partition.

But we will qualify it before writing to it at here, doesn't it work?

The issue is that the user can store arbitrary string paths with ALTER TABLE PARTITIONS SET LOCATION. Therefore, we must manually qualify the locations that come from the catalog or else they might not necessarily match up with the paths read from the filesystem.

cloud-fan · 2016-11-01T03:12:17Z

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

          "Cannot overwrite a path that is also being read from.")
      }

+      val overwritePartitionPath = if (overwrite.specificPartition.isDefined &&


can we just pass the partition path as outputPath to InsertIntoHadoopFsRelationCommand and set partition columns to Nil？ then we don't need to add an extra parameter to InsertIntoHadoopFsRelationCommand

Yeah, that seems a little cleaner.

SparkQA · 2016-11-01T05:10:40Z

Test build #67864 has finished for PR 15705 at commit 0daff74.

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

SparkQA · 2016-11-01T21:04:39Z

Test build #67911 has finished for PR 15705 at commit ca31f35.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class TaskCommitMessage(obj: Any) extends Serializable
- abstract class FileCommitProtocol
- class HadoopCommitProtocolWrapper(path: String, isAppend: Boolean)
- logInfo(s\"Using user defined output committer class $
- logInfo(s\"Using output committer class $

cloud-fan · 2016-11-02T03:48:30Z

sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala

+        OverwriteOptions(
+          overwrite,
+          if (overwrite && partition.nonEmpty) {
+            Some(partition.map(kv => (kv._1, kv._2.get)))


do we need to consider dynamic partition here?

ericl · 2016-11-02T05:43:37Z

I think you don't have to since this is just the test suite.

On Tue, Nov 1, 2016, 8:49 PM Wenchen Fan [email protected] wrote:

@cloud-fan commented on this pull request.

In
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/parser/PlanParserSuite.scala
#15705 (review):
@@ -180,7 +180,16 @@ class PlanParserSuite extends PlanTest {
partition: Map[String, Option[String]],
overwrite: Boolean = false,
ifNotExists: Boolean = false): LogicalPlan =
 InsertIntoTable(table("s"), partition, plan, overwrite, ifNotExists)
 InsertIntoTable(
   table("s"), partition, plan,
   OverwriteOptions(
     overwrite,
     if (overwrite && partition.nonEmpty) {
       Some(partition.map(kv => (kv._1, kv._2.get)))
do we need to consider dynamic partition here?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#15705 (review),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAA6SpEB-R5k4u55HP0Wt6z9i1OihpNSks5q6AhJgaJpZM4KlpEq
.

cloud-fan · 2016-11-02T06:15:23Z

LGTM, merging to master!

…ITION for Datasource tables There are a couple issues with the current 2.1 behavior when inserting into Datasource tables with partitions managed by Hive. (1) OVERWRITE TABLE ... PARTITION will actually overwrite the entire table instead of just the specified partition. (2) INSERT|OVERWRITE does not work with partitions that have custom locations. This PR fixes both of these issues for Datasource tables managed by Hive. The behavior for legacy tables or when `manageFilesourcePartitions = false` is unchanged. There is one other issue in that INSERT OVERWRITE with dynamic partitions will overwrite the entire table instead of just the updated partitions, but this behavior is pretty complicated to implement for Datasource tables. We should address that in a future release. Unit tests. Author: Eric Liang <[email protected]> Closes #15705 from ericl/sc-4942. (cherry picked from commit abefe2e) Signed-off-by: Reynold Xin <[email protected]>

## What changes were proposed in this pull request? `OverwriteOptions` was introduced in #15705, to carry the information of static partitions. However, after further refactor, this information becomes duplicated and we can remove `OverwriteOptions`. ## How was this patch tested? N/A Author: Wenchen Fan <[email protected]> Closes #15995 from cloud-fan/overwrite.

## What changes were proposed in this pull request? `OverwriteOptions` was introduced in apache#15705, to carry the information of static partitions. However, after further refactor, this information becomes duplicated and we can remove `OverwriteOptions`. ## How was this patch tested? N/A Author: Wenchen Fan <[email protected]> Closes apache#15995 from cloud-fan/overwrite.

…ITION for Datasource tables ## What changes were proposed in this pull request? There are a couple issues with the current 2.1 behavior when inserting into Datasource tables with partitions managed by Hive. (1) OVERWRITE TABLE ... PARTITION will actually overwrite the entire table instead of just the specified partition. (2) INSERT|OVERWRITE does not work with partitions that have custom locations. This PR fixes both of these issues for Datasource tables managed by Hive. The behavior for legacy tables or when `manageFilesourcePartitions = false` is unchanged. There is one other issue in that INSERT OVERWRITE with dynamic partitions will overwrite the entire table instead of just the updated partitions, but this behavior is pretty complicated to implement for Datasource tables. We should address that in a future release. ## How was this patch tested? Unit tests. Author: Eric Liang <[email protected]> Closes apache#15705 from ericl/sc-4942.

## What changes were proposed in this pull request? `OverwriteOptions` was introduced in apache#15705, to carry the information of static partitions. However, after further refactor, this information becomes duplicated and we can remove `OverwriteOptions`. ## How was this patch tested? N/A Author: Wenchen Fan <[email protected]> Closes apache#15995 from cloud-fan/overwrite.

ericl added 8 commits October 27, 2016 14:44

Thu Oct 27 14:44:04 PDT 2016

dee4b9e

Fri Oct 28 14:20:28 PDT 2016

d99afac

Merge branch 'master' into sc-4942

137aeb2

wip

8da8b0f

implement custom locations

d5a7bd3

Mon Oct 31 16:32:15 PDT 2016

a833a19

Mon Oct 31 16:34:47 PDT 2016

587b85e

Mon Oct 31 16:41:40 PDT 2016

669e6cc

ericl added 2 commits October 31, 2016 17:09

Mon Oct 31 17:09:36 PDT 2016

fec7c9e

Mon Oct 31 17:13:37 PDT 2016

07c6787

Mon Oct 31 19:28:16 PDT 2016

0daff74

cloud-fan reviewed Nov 1, 2016

View reviewed changes

Merge branch 'master' into sc-4942

ca31f35

cloud-fan reviewed Nov 2, 2016

View reviewed changes

asfgit closed this in abefe2e Nov 2, 2016

cloud-fan mentioned this pull request Nov 23, 2016

[SPARK-18566][SQL] remove OverwriteOptions #15995

Closed

[SPARK-18183] [SPARK-18184] Fix INSERT [INTO|OVERWRITE] TABLE ... PARTITION for Datasource tables #15705

[SPARK-18183] [SPARK-18184] Fix INSERT [INTO|OVERWRITE] TABLE ... PARTITION for Datasource tables #15705

Uh oh!

Conversation

ericl commented Oct 31, 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!

ericl commented Oct 31, 2016

Uh oh!

SparkQA commented Oct 31, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

cloud-fan Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

ericl Nov 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!

cloud-fan Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

ericl Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

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

ericl Nov 1, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

cloud-fan Nov 2, 2016

Choose a reason for hiding this comment

Uh oh!

ericl commented Nov 2, 2016

@cloud-fan commented on this pull request.

Uh oh!

cloud-fan commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ericl commented Oct 31, 2016 •

edited

Loading

ericl Nov 1, 2016 •

edited

Loading

cloud-fan Nov 1, 2016 •

edited

Loading