Skip to content

Conversation

@viirya
Copy link
Member

@viirya viirya commented Jun 19, 2016

What changes were proposed in this pull request?

Right now, InsertIntoTable's expectedColumns uses the method of contains to find static partitioning columns. When analyzer is case-insensitive, the initialization of this lazy val will not work as expected.

How was this patch tested?

Existing tests.

@SparkQA
Copy link

SparkQA commented Jun 19, 2016

Test build #60810 has finished for PR 13772 at commit f9b2907.

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

@SparkQA
Copy link

SparkQA commented Jun 19, 2016

Test build #60813 has finished for PR 13772 at commit dba001d.

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

@SparkQA
Copy link

SparkQA commented Jun 20, 2016

Test build #60826 has finished for PR 13772 at commit a472cc2.

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

Conflicts:
	sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala
	sql/core/src/main/scala/org/apache/spark/sql/DataFrameWriter.scala
@viirya
Copy link
Member Author

viirya commented Jun 20, 2016

ping @liancheng @yhuai @cloud-fan

@SparkQA
Copy link

SparkQA commented Jun 20, 2016

Test build #60839 has finished for PR 13772 at commit 468a781.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class TextSocketSource(host: String, port: Int, sqlContext: SQLContext)
    • class TextSocketSourceProvider extends StreamSourceProvider with DataSourceRegister with Logging

@liancheng
Copy link
Contributor

@viirya Could you please help verify that whether #13769 already fixed this issue?

@liancheng
Copy link
Contributor

liancheng commented Jun 20, 2016

Oh, #13769 only solves case-insensitive resolution for partition columns.

This PR also aims to fix case-insensitive resolution of static partition columns, so I think it's already covered by #13769?

@viirya
Copy link
Member Author

viirya commented Jun 20, 2016

@liancheng yes.

@viirya
Copy link
Member Author

viirya commented Jun 20, 2016

@liancheng hmm, for that part, seems so. But the added rule in #13769 seems only cover the case of HadoopFsRelation?

@liancheng
Copy link
Contributor

Yea, but partitioning is only available for HadoopFsRelation.

@yhuai
Copy link
Contributor

yhuai commented Jun 20, 2016

@viirya Thank you for the pr. After I filed the jira, I did some investigation. Actually as I noted at https://github.com/apache/spark/blob/master/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/basicLogicalOperators.scala#L372, the parser will turn the keys to their lowercase forms. So, I think there is nothing really broken assuming case-insensitive resolution is used. How about we close this PR? We need to address the case-sensitivity flag in a holistic way. We can revisit this particular issue later.

@gatorsmile
Copy link
Member

#12993 (comment) This is the comment left by @rxin in my PR. I assume this will not be addressed in this release. Anything is changed?

@viirya
Copy link
Member Author

viirya commented Jun 20, 2016

@yhuai I've run test against current branch. When case-insensitive resolution is used, exepctedColumns is not correct actually. So I am not sure why you said it is not really broken? I will close this first because I think this approach is not good. Maybe I will try alternative one later. Thanks!

@viirya viirya closed this Jun 20, 2016
@yhuai
Copy link
Contributor

yhuai commented Jun 21, 2016

@viirya Can you comment in the jira with your case?

@viirya
Copy link
Member Author

viirya commented Jun 21, 2016

@yhuai What case you meant?

@yhuai
Copy link
Contributor

yhuai commented Jun 21, 2016

Tests that you mentioned in I've run test against current branch. When case-insensitive resolution is used, exepctedColumns is not correct actually

@viirya viirya deleted the fix-expectedcols 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.

5 participants