Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Jun 13, 2016

What changes were proposed in this pull request?

Currently In DataFrameWriter's insertInto and ResolveRelations of Analyzer, we add additional Project to adjust column ordering. However, it should be using ordering not name for this resolution. This is how Hive does for dynamic partition.

How was this patch tested?

Existing tests.

@SparkQA
Copy link

SparkQA commented Jun 13, 2016

Test build #60380 has finished for PR 13631 at commit 5f4455a.

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

@viirya
Copy link
Member Author

viirya commented Jun 13, 2016

@cloud-fan There is a test ("Detect table partitioning with correct partition order") in InsertIntoHiveTableSuite which is dedicated to test insertInto with this column re-ordering. What you think we should do about it? Remove it?

@SparkQA
Copy link

SparkQA commented Jun 13, 2016

Test build #60403 has finished for PR 13631 at commit c0500c2.

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

}
}

test("Detect table partitioning with correct partition order") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you link to the PR that added this test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is added by PR #12239.

@SparkQA
Copy link

SparkQA commented Jun 14, 2016

Test build #60481 has finished for PR 13631 at commit f8c9ccf.

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

inputPartCols.find(_.name == name).getOrElse(
throw new AnalysisException(s"Cannot find partition column $name"))
tablePartitionNames.filterNot { name =>
child.output.exists(_.name == name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we really need this check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm. indeed. As we use ordering not name, this check is not needed anymore. I will remove it.

@SparkQA
Copy link

SparkQA commented Jun 15, 2016

Test build #60546 has finished for PR 13631 at commit 3030144.

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

@viirya
Copy link
Member Author

viirya commented Jun 16, 2016

@cloud-fan any other thoughts?

@cloud-fan
Copy link
Contributor

hi @viirya , we are auditing the insertion behaviour of spark sql, and will have an agreement this week. How about we revisit this PR after that?

@viirya
Copy link
Member Author

viirya commented Jun 16, 2016

@cloud-fan no problem at all.

@cloud-fan
Copy link
Contributor

please track the process of https://issues.apache.org/jira/browse/SPARK-16032 and update your PR, thanks!

@viirya
Copy link
Member Author

viirya commented Jun 19, 2016

@cloud-fan Looks like a part of this PR is done by some PRs in that JIRA. I will update this.

viirya added 2 commits June 19, 2016 12:48
Conflicts:
	sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
Conflicts:
	sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala
	sql/hive/src/test/scala/org/apache/spark/sql/hive/InsertIntoHiveTableSuite.scala
@viirya
Copy link
Member Author

viirya commented Jun 19, 2016

@cloud-fan Updated. Please take a look. Thanks!

@SparkQA
Copy link

SparkQA commented Jun 19, 2016

Test build #60795 has finished for PR 13631 at commit be60027.

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

@viirya
Copy link
Member Author

viirya commented Jun 20, 2016

ping @cloud-fan

@viirya
Copy link
Member Author

viirya commented Jun 20, 2016

@cloud-fan Looks like the change in this PR is done by #13766. Let me close it now.

@viirya viirya closed this Jun 20, 2016
@viirya viirya deleted the inserttable branch December 27, 2023 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants