Skip to content

[SPARK-28054][SQL][followup] move the bug fix closer to where causes the issue#25234

Closed
cloud-fan wants to merge 2 commits intoapache:masterfrom
cloud-fan:minor
Closed

[SPARK-28054][SQL][followup] move the bug fix closer to where causes the issue#25234
cloud-fan wants to merge 2 commits intoapache:masterfrom
cloud-fan:minor

Conversation

@cloud-fan
Copy link
Contributor

What changes were proposed in this pull request?

The bug fixed by #24886 is caused by Hive's loadDynamicPartitions. It's better to keep the fix surgical and put it right before we call loadDynamicPartitions.

This also makes the fix safer, instead of analyzing all the callers of saveAsHiveFile and proving that they are safe.

How was this patch tested?

N/A

@cloud-fan
Copy link
Contributor Author

cc @viirya @gatorsmile

@SparkQA
Copy link

SparkQA commented Jul 23, 2019

Test build #108045 has finished for PR 25234 at commit 0284685.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

attr.withName(attr.name.toLowerCase)
}
// scalastyle:on caselocale

Copy link
Member

Choose a reason for hiding this comment

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

This move looks good. And we should use partitionAttributes below.

// with lower cased names. Hive will validate the column names in the partition directories
// during `loadDynamicPartitions`. Spark needs to write partition directories with lower-cased
// column names in order to make `loadDynamicPartitions` work.
attr.withName(name.toLowerCase(Locale.ROOT))
Copy link
Member

Choose a reason for hiding this comment

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

This move looks nicer and also cleaner due to the removal of scalastyle:off caselocale.

@SparkQA
Copy link

SparkQA commented Jul 23, 2019

Test build #108058 has finished for PR 25234 at commit 673fef6.

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

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Merged to the master.

@dongjoon-hyun
Copy link
Member

Thank you, @cloud-fan and @viirya !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants