[SPARK-18173][SQL] data source tables should support truncating partition #15688

cloud-fan · 2016-10-30T17:32:00Z

What changes were proposed in this pull request?

Previously TRUNCATE TABLE ... PARTITION will always truncate the whole table for data source tables, this PR fixes it and improve InMemoryCatalog to make this command work with it.

How was this patch tested?

existing tests

cloud-fan · 2016-10-30T17:32:12Z

cc @ericl @yhuai @gatorsmile

jaceklaskowski · 2016-10-30T18:30:28Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

Mind changing rectangles2 to rectPartitioned or similar? Think the test would get simpler to read (later).

SparkQA · 2016-10-30T19:06:23Z

Test build #67788 has finished for PR 15688 at commit a9eec51.

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

ericl · 2016-10-30T21:57:35Z

This lgtm

gatorsmile · 2016-10-31T21:13:56Z

If we try to truncate a partition that does not exist, we just do nothing?

cloud-fan · 2016-10-31T23:49:37Z

retest this please

SparkQA · 2016-11-01T02:19:59Z

Test build #67847 has finished for PR 15688 at commit a9eec51.

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

cloud-fan · 2016-11-01T04:56:35Z

@gatorsmile that's a good point. I checked with hive, the behaviour is:

if the given partition spec specifies all partition columns, throw exception if the partition not exist
if the given partition spec is partial, do nothing if the partition not exist

I have updated this PR according to this

SparkQA · 2016-11-01T06:28:43Z

Test build #67871 has finished for PR 15688 at commit 9045ccd.

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

cloud-fan · 2016-11-01T08:28:06Z

retest this please

SparkQA · 2016-11-01T09:51:58Z

Test build #67882 has finished for PR 15688 at commit 9045ccd.

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

cloud-fan · 2016-11-01T10:52:10Z

retest this please

SparkQA · 2016-11-01T13:10:37Z

Test build #67887 has finished for PR 15688 at commit 9045ccd.

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

rxin · 2016-11-01T19:38:05Z

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

what does partial mean here?

we should document it.

gatorsmile · 2016-11-01T20:57:35Z

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

If the specified column names are not partitioning columns, we will get a strange error:

key not found: height java.util.NoSuchElementException: key not found: height

I don't think we will/should hit this in the ExternalCatalog level, as most of partition related methods assumes the column names in partition spec are partition columns, e.g. loadPartition, loadDynamicPartitions, dropPartitions, etc.

If we do hit this, we should fix it at the caller side.

The above message was got from the statement

sql("TRUNCATE TABLE partTable PARTITION (height =1)")

gatorsmile · 2016-11-01T21:00:47Z

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

Comparing the size is not enough. We also need to check whether all the columns are partitioning columns.

gatorsmile · 2016-11-01T21:02:29Z

Could we add all the positive and negative cases in ExternalCatalogSuite, like the other APIs? Then, we can easily know whether the behaviors of InMemoryCatalog and HiveExternalCatalog are consistent.

SparkQA · 2016-11-05T08:02:08Z

Test build #68188 has finished for PR 15688 at commit 8941e10.

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

cloud-fan · 2016-11-05T08:15:52Z

retest this please

SparkQA · 2016-11-05T10:48:49Z

Test build #68195 has finished for PR 15688 at commit 8941e10.

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

rxin · 2016-11-05T20:00:09Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala

+
+      // do nothing if no partition is matched for the given non-partial partition spec
+      // TODO: This behaviour is different from Hive, we should decide whether we need to follow
+      // Hive's behaviour or stick with our existing behaviour later.


what's our and hive's behavior?

our behaviour: do nothing if no partition is matched, no matter whether the given partition spec is partial or not.

Hive's behaviour: when no partition is matched, do nothing if the given partition spec is partial, throw exception if the given partition spec is non-partial.

Could we document the behavior difference in the code comments, if we decide to stick with our existing behavior?

I actually think Hive's behavior makes sense here. If I'm giving you an exact match, you should warn me if there is an issue.

The alternative, of course, is to provide "truncate if exists", which doesn't throw exceptions if that's desired.

rxin · 2016-11-05T20:00:36Z

cc @gatorsmile

gatorsmile · 2016-11-06T05:00:35Z

Regarding the current code changes, LGTM

Like what @cloud-fan said in the code comment, we need to decide whether we should follow Hive.

SparkQA · 2016-11-06T17:00:26Z

Test build #68241 has finished for PR 15688 at commit 4726228.

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

rxin · 2016-11-07T02:56:30Z

Thanks - merging in master/branch-2.1.

rxin · 2016-11-07T02:56:46Z

@cloud-fan can you create a follow-up pr to switch over to Hive's behavior?

…tion ## What changes were proposed in this pull request? Previously `TRUNCATE TABLE ... PARTITION` will always truncate the whole table for data source tables, this PR fixes it and improve `InMemoryCatalog` to make this command work with it. ## How was this patch tested? existing tests Author: Wenchen Fan <[email protected]> Closes #15688 from cloud-fan/truncate. (cherry picked from commit 46b2e49) Signed-off-by: Reynold Xin <[email protected]>

…hed for the given non-partial partition spec ## What changes were proposed in this pull request? a follow up of #15688 ## How was this patch tested? updated test in `DDLSuite` Author: Wenchen Fan <[email protected]> Closes #15805 from cloud-fan/truncate. (cherry picked from commit 73feaa3) Signed-off-by: Wenchen Fan <[email protected]>

…hed for the given non-partial partition spec ## What changes were proposed in this pull request? a follow up of apache#15688 ## How was this patch tested? updated test in `DDLSuite` Author: Wenchen Fan <[email protected]> Closes apache#15805 from cloud-fan/truncate.

…tion ## What changes were proposed in this pull request? Previously `TRUNCATE TABLE ... PARTITION` will always truncate the whole table for data source tables, this PR fixes it and improve `InMemoryCatalog` to make this command work with it. ## How was this patch tested? existing tests Author: Wenchen Fan <[email protected]> Closes apache#15688 from cloud-fan/truncate.

…hed for the given non-partial partition spec ## What changes were proposed in this pull request? a follow up of apache#15688 ## How was this patch tested? updated test in `DDLSuite` Author: Wenchen Fan <[email protected]> Closes apache#15805 from cloud-fan/truncate.

jaceklaskowski reviewed Oct 30, 2016

View reviewed changes

rxin reviewed Nov 1, 2016

View reviewed changes

gatorsmile reviewed Nov 1, 2016

View reviewed changes

data source tables should support truncating partition

8941e10

cloud-fan force-pushed the truncate branch from 9045ccd to 8941e10 Compare November 5, 2016 06:21

rxin reviewed Nov 5, 2016

View reviewed changes

minor improve

4726228

asfgit closed this in 46b2e49 Nov 7, 2016

cloud-fan mentioned this pull request Nov 8, 2016

[SPARK-18346][SQL] TRUNCATE TABLE should fail if no partition is matched for the given non-partial partition spec #15805

Closed

[SPARK-18173][SQL] data source tables should support truncating partition #15688

[SPARK-18173][SQL] data source tables should support truncating partition #15688

Uh oh!

Conversation

cloud-fan commented Oct 30, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Oct 30, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 30, 2016

Uh oh!

ericl commented Oct 30, 2016

Uh oh!

gatorsmile commented Oct 31, 2016

Uh oh!

cloud-fan commented Oct 31, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

cloud-fan commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

cloud-fan commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 2016

Uh oh!

cloud-fan commented Nov 1, 2016

Uh oh!

SparkQA commented Nov 1, 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!

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

Uh oh!

Uh oh!

SparkQA commented Nov 5, 2016

Uh oh!

cloud-fan commented Nov 5, 2016

Uh oh!

SparkQA commented Nov 5, 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!

rxin commented Nov 5, 2016

Uh oh!

gatorsmile commented Nov 6, 2016

Uh oh!

SparkQA commented Nov 6, 2016

Uh oh!

rxin commented Nov 7, 2016

Uh oh!

rxin commented Nov 7, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gatorsmile commented Nov 1, 2016 •

edited

Loading