[SPARK-16119][sql] Support PURGE option to drop table / partition. #13831

vanzin · 2016-06-22T00:34:30Z

This option is used by Hive to directly delete the files instead of
moving them to the trash. This is needed in certain configurations
where moving the files does not work. For non-Hive tables and partitions,
Spark already behaves as if the PURGE option was set, so there's no
need to do anything.

Hive support for PURGE was added in 0.14 (for tables) and 1.2 (for
partitions), so the code reflects that: trying to use the option with
older versions of Hive will cause an exception to be thrown.

The change is a little noisier than I would like, because of the code
to propagate the new flag through all the interfaces and implementations;
the main changes are in the parser and in HiveShim, aside from the tests
(DDLCommandSuite, VersionsSuite).

Tested by running sql and catalyst unit tests, plus VersionsSuite which
has been updated to test the version-specific behavior. I also ran an
internal test suite that uses PURGE and would not pass previously.

This option is used by Hive to directly delete the files instead of moving them to the trash. This is needed in certain configurations where moving the files does not work. For non-Hive tables and partitions, Spark already behaves as if the PURGE option was set, so there's no need to do anything. Hive support for PURGE was added in 0.14 (for tables) and 1.2 (for partitions), so the code reflects that: trying to use the option with older versions of Hive will cause an exception to be thrown. The change is a little noisier than I would like, because of the code to propagate the new flag through all the interfaces and implementations; the main changes are in the parser and in HiveShim, aside from the tests (DDLCommandSuite, VersionsSuite). Tested by running sql and catalyst unit tests, plus VersionsSuite which has been updated to test the version-specific behavior. I also ran an internal test suite that uses PURGE and would not pass previously.

vanzin · 2016-06-22T00:35:20Z

@andrewor14 @yhuai not sure who should review this?

Also, using a parameter with a default value might avoid some of the noise, but I didn't see that approach used anywhere else in these APIs, so didn't go that route.

SparkQA · 2016-06-22T01:55:03Z

Test build #60987 has finished for PR 13831 at commit a53b7fd.

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

…lection.

yhuai · 2016-06-22T04:22:58Z

@vanzin Thank you for the PR. I probably will not be able to review it until we get 2.0 out. Will take a look after the release.

SparkQA · 2016-06-22T06:32:03Z

Test build #61004 has finished for PR 13831 at commit b084081.

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

SparkQA · 2016-06-28T22:19:45Z

Test build #61406 has finished for PR 13831 at commit 3dca099.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class ElementwiseProduct @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)
- class Normalizer @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)
- class PolynomialExpansion @Since(\"1.4.0\") (@Since(\"1.4.0\") override val uid: String)
- public class JavaPackage
- case class ShowFunctionsCommand(
- case class StreamingRelationExec(sourceName: String, output: Seq[Attribute]) extends LeafExecNode

SparkQA · 2016-07-06T19:53:39Z

Test build #61859 has finished for PR 13831 at commit cd0f2bb.

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

squito · 2016-07-08T11:55:20Z

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

@@ -776,21 +778,29 @@ class DDLCommandSuite extends PlanTest {
    val parsed2 = parser.parsePlan(s"DROP TABLE IF EXISTS $tableName1")
    val parsed3 = parser.parsePlan(s"DROP TABLE $tableName2")
    val parsed4 = parser.parsePlan(s"DROP TABLE IF EXISTS $tableName2")
-    assertUnsupported(s"DROP TABLE IF EXISTS $tableName2 PURGE")
+    val parsed5 = parser.parsePlan(s"DROP TABLE $tableName2 PURGE")


for completeness, probably worth also checking IF EXISTS ... PURGE combo

squito · 2016-07-08T12:30:00Z

you could avoid changing all the callsites by adding a default of purge = false, but I'm guessing that is discouraged in the sql code (since eg. there is not default for ignoreIfNotExists).

EDIT: oops, now I see you made the same comment earlier.

squito · 2016-07-08T14:12:33Z

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

@@ -179,7 +179,8 @@ case class DescribeDatabaseCommand(
 case class DropTableCommand(
    tableName: TableIdentifier,
    ifExists: Boolean,
-    isView: Boolean) extends RunnableCommand {
+    isView: Boolean,
+    purge: Boolean) extends RunnableCommand {



minor but it would be nice to have an assert(!isView || !purge, "Cannot purge a view"). presumably guaranteed by the parser anyway, but it would be nice to have it codified here.

I'm not personally against the assert but I can't find anywhere else where the code is asserting something the parser already checks. So I'll pass on this unless someone that plays with this code more says it's ok.

squito · 2016-07-08T14:22:18Z

it would be nice to test using purge actually does a purge, ie nothing is in the trash afterwards. I don't see any easy way to do that, though -- maybe using TrashPolicy.getCurrentTrashDir()?

squito · 2016-07-08T16:47:28Z

minor comments andrequest for some extra tests, but overall looks good

vanzin · 2016-07-08T18:04:23Z

it would be nice to test using purge actually does a purge

None of the tests run on actual HDFS cluster, so I doubt that would be possible without a lot more work...

SparkQA · 2016-07-08T20:08:38Z

Test build #61991 has finished for PR 13831 at commit c108930.

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

vanzin · 2016-07-11T17:39:57Z

I'd like to get this in soon, so pending tests, I'll leave this open until tomorrow and then push, unless I hear otherwise.

SparkQA · 2016-07-11T19:40:16Z

Test build #62104 has finished for PR 13831 at commit f5fcc84.

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

squito · 2016-07-11T19:40:46Z

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

+      dropTableMethod.invoke(hive, dbName, tableName, deleteData: JBoolean,
+        ignoreIfNotExists: JBoolean, purge: JBoolean)
+    } catch {
+      case e: InvocationTargetException => throw e.getCause()


just curious, why include this extra handling here? its not used on any of the other reflective calls. I'm guessing it just makes the exception a little cleaner, so really could be everywhere?

b084081

(See test failures before that commit.)

squito · 2016-07-11T19:41:56Z

it would be nice to test using purge actually does a purge

None of the tests run on actual HDFS cluster, so I doubt that would be possible without a lot more work...

hmm, yeah I was hoping you could fake this with a localfilesystem, but I futzed with it for a little bit and couldn't get it to work. ok then.

lgtm

vanzin · 2016-07-12T19:47:19Z

Merging to master.

Propagate expected exception when calling dropTable/Partition via ref…

b084081

…lection.

Merge branch 'master' into SPARK-16119

3dca099

Marcelo Vanzin added 2 commits July 6, 2016 10:31

Merge branch 'master' into SPARK-16119

ff99635

Update a new call site.

cd0f2bb

squito reviewed Jul 8, 2016
View reviewed changes

Review feedback: more tests.

c108930

Merge branch 'master' into SPARK-16119

f5fcc84

squito reviewed Jul 11, 2016
View reviewed changes

asfgit closed this in 7f96886 Jul 12, 2016

vanzin deleted the SPARK-16119 branch July 12, 2016 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-16119][sql] Support PURGE option to drop table / partition. #13831

[SPARK-16119][sql] Support PURGE option to drop table / partition. #13831

vanzin commented Jun 22, 2016

vanzin commented Jun 22, 2016

SparkQA commented Jun 22, 2016

yhuai commented Jun 22, 2016

SparkQA commented Jun 22, 2016

SparkQA commented Jun 28, 2016

SparkQA commented Jul 6, 2016

squito Jul 8, 2016

squito commented Jul 8, 2016 •

edited

Loading

squito Jul 8, 2016

vanzin Jul 8, 2016

squito commented Jul 8, 2016

squito commented Jul 8, 2016

vanzin commented Jul 8, 2016

SparkQA commented Jul 8, 2016

vanzin commented Jul 11, 2016

SparkQA commented Jul 11, 2016

squito Jul 11, 2016

vanzin Jul 11, 2016

squito commented Jul 11, 2016

vanzin commented Jul 12, 2016

[SPARK-16119][sql] Support PURGE option to drop table / partition. #13831

[SPARK-16119][sql] Support PURGE option to drop table / partition. #13831

Conversation

vanzin commented Jun 22, 2016

vanzin commented Jun 22, 2016

SparkQA commented Jun 22, 2016

yhuai commented Jun 22, 2016

SparkQA commented Jun 22, 2016

SparkQA commented Jun 28, 2016

SparkQA commented Jul 6, 2016

squito Jul 8, 2016

Choose a reason for hiding this comment

squito commented Jul 8, 2016 • edited Loading

squito Jul 8, 2016

Choose a reason for hiding this comment

vanzin Jul 8, 2016

Choose a reason for hiding this comment

squito commented Jul 8, 2016

squito commented Jul 8, 2016

vanzin commented Jul 8, 2016

SparkQA commented Jul 8, 2016

vanzin commented Jul 11, 2016

SparkQA commented Jul 11, 2016

squito Jul 11, 2016

Choose a reason for hiding this comment

vanzin Jul 11, 2016

Choose a reason for hiding this comment

squito commented Jul 11, 2016

vanzin commented Jul 12, 2016

squito commented Jul 8, 2016 •

edited

Loading