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

cloud-fan · 2016-11-23T16:43:00Z

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

cloud-fan · 2016-11-23T16:43:37Z

cc @yhuai @ericl

SparkQA · 2016-11-23T18:14:42Z

Test build #69083 has finished for PR 15995 at commit f52b364.

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

ericl · 2016-11-28T02:51:19Z

Iirc there was an issue where convertStaticPartitions erased the partitioning information during analysis. Was this fixed by the refactoring?

cloud-fan · 2016-11-28T05:46:53Z

@ericl you are right, I pushed a new commit to do convertStaticPartitions right before we convert InsertIntoTable to InsertIntoHadoopFsRelation, so the partitioning information won't be erased.

SparkQA · 2016-11-28T07:33:31Z

Test build #69220 has finished for PR 15995 at commit 354a860.

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

ericl · 2016-11-29T19:21:29Z

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

This seems to be failing in the tests. Is it because the preprocessing rule no longer gets a chance to run now that the static resolution is combined with this rule?

I'm going to submit another PR to tweak the extended analyzer rules execution order, currently these rules like PreProcessCreateTable, PreWriteCheck, DataSourceAnalysis, etc. may have some dependencies and may worth to put them in different batches.

BTW I have retargeted this JIRA ticket to 2.2

SparkQA · 2016-12-02T20:55:05Z

Test build #69579 has finished for PR 15995 at commit 5f9da54.

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

SparkQA · 2016-12-03T14:42:46Z

Test build #69623 has finished for PR 15995 at commit b5f4394.

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

cloud-fan · 2016-12-11T12:58:03Z

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

-      } else {
-        Map.empty
-      }
+      val staticPartitions = parts.filter(_._2.nonEmpty).map { case (k, v) => k -> v.get }


The column names in partition spec are already normalized in PreprocessTableInsertion rule, we don't need to consider case sensitivity here. And the if-else is not needed, because:

staticPartitions is used to get matchingPartitions in this line, and the matchingPartitions is used to decided which partitions need to be added to metastore. Previously if overwrite is false, we will get all partitions as matchingPartitions, and issue a lot of unnecessary ADD PARTITION calls. After removing the if-else, it's fixed.

After we pass staticPartitions to InsertIntoHadoopFsRelationCommand, it will be used only with OverWrite mode, so the if-else is unnecessary.

SparkQA · 2016-12-11T15:16:31Z

Test build #69986 has finished for PR 15995 at commit 323a97c.

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

ericl · 2016-12-12T20:07:02Z

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

    projectList
  }

+  private def hasBeenPreprocessed(


Add a comment about who preprocesses this?

ericl · 2016-12-12T20:07:33Z

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

+      partSpec: Map[String, Option[String]],
+      query: LogicalPlan): Boolean = {
+    val partColNames = partSchema.map(_.name).toSet
+    query.resolved && partSpec.keys.forall(partColNames.contains) && {


Is it necessary to check that the keys are all valid columns?

Ah, is the issue to avoid this running before PreprocessTableInsertion?

ericl · 2016-12-12T20:10:23Z

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

+    query.resolved && partSpec.keys.forall(partColNames.contains) && {
+      val staticPartCols = partSpec.filter(_._2.isDefined).keySet
+      val expectedColumns = tableOutput.filterNot(a => staticPartCols.contains(a.name))
+      expectedColumns.toStructType.sameType(query.schema)


similar question, when is this false?

this is to follow the previous condition: https://github.com/apache/spark/pull/15995/files#diff-d99813bd5bbc18277e4090475e4944cfL166

This can be caused if users issue an invalid command, e.g. INSERT INTO src SELECT 1,2 while table src has 3 columns.

SparkQA · 2016-12-13T08:01:10Z

Test build #70060 has finished for PR 15995 at commit ed548e6.

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

ericl · 2016-12-14T02:18:09Z

Cool, this looks good then

cloud-fan · 2016-12-14T03:30:59Z

thanks for the review, merging to master!

## 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 reviewed Nov 29, 2016

View reviewed changes

remove OverwriteOptions

30e72e3

cloud-fan force-pushed the overwrite branch from 354a860 to 5f9da54 Compare December 2, 2016 18:08

fix test

b5f4394

cloud-fan force-pushed the overwrite branch from 5f9da54 to b5f4394 Compare December 3, 2016 12:15

cloud-fan added 2 commits December 11, 2016 20:20

Merge remote-tracking branch 'origin/master' into overwrite

f671939

simplify

323a97c

cloud-fan commented Dec 11, 2016

View reviewed changes

cloud-fan mentioned this pull request Dec 12, 2016

[SPARK-18567][SQL] Simplify CreateDataSourceTableAsSelectCommand #15996

Closed

ericl reviewed Dec 12, 2016

View reviewed changes

add comment

ed548e6

asfgit closed this in 3e307b4 Dec 14, 2016

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

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

Uh oh!

Conversation

cloud-fan commented Nov 23, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

cloud-fan commented Nov 23, 2016

Uh oh!

SparkQA commented Nov 23, 2016

Uh oh!

ericl commented Nov 28, 2016

Uh oh!

cloud-fan commented Nov 28, 2016

Uh oh!

SparkQA commented Nov 28, 2016

Uh oh!

ericl Nov 29, 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 30, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 2, 2016

Uh oh!

SparkQA commented Dec 3, 2016

Uh oh!

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

SparkQA commented Dec 11, 2016

Uh oh!

ericl Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

ericl Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

ericl Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 13, 2016

Choose a reason for hiding this comment

Uh oh!

ericl Dec 12, 2016

Choose a reason for hiding this comment

Uh oh!

cloud-fan Dec 13, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Dec 13, 2016

Uh oh!

ericl commented Dec 14, 2016

Uh oh!

cloud-fan commented Dec 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ericl Nov 29, 2016 •

edited

Loading

cloud-fan Dec 11, 2016 •

edited

Loading