[SPARK-20412] Throw ParseException from visitNonOptionalPartitionSpec instead of returning null values. #17707
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
cc @cloud-fan |
|
ok to test |
|
Could you add a unit test to the DDLCommandSuite? |
|
Test build #75994 has finished for PR 17707 at commit
|
|
Test build #75997 has finished for PR 17707 at commit
|
|
looks like we need to fix a test |
gatorsmile
reviewed
Apr 21, 2017
| @@ -214,8 +214,11 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging { | |||
| * Create a partition specification map without optional values. | |||
| */ | |||
| protected def visitNonOptionalPartitionSpec( | |||
| ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { | |||
| visitPartitionSpec(ctx).mapValues(_.orNull).map(identity) | |||
| ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { | |||
There was a problem hiding this comment.
Nit: Indentation issues. Need to add extra two spaces
gatorsmile
reviewed
Apr 21, 2017
| visitPartitionSpec(ctx).mapValues(_.orNull).map(identity) | ||
| ctx: PartitionSpecContext): Map[String, String] = withOrigin(ctx) { | ||
| visitPartitionSpec(ctx).map { | ||
| case (key, None) => throw new ParseException(s"Found empty key '$key'.", ctx) |
There was a problem hiding this comment.
How about Found an empty partition key '$key'?
|
Test build #76029 has finished for PR 17707 at commit
|
| val expected5 = AlterTableSerDePropertiesCommand( | ||
| tableIdent, | ||
| None, | ||
| Some(Map("columns" -> "foo,bar", "field.delim" -> ",")), | ||
| Some(Map("test" -> null, "dt" -> "2008-08-08", "country" -> "us"))) |
There was a problem hiding this comment.
BTW: In AlterTableSerDePropertiesCommand that null would have caused a NullPointerExeception later, so not having this as null, but checked as ParseException is also a valid change here.
|
thanks, merging to master/2.2! |
asfgit
pushed a commit
that referenced
this pull request
Apr 21, 2017
… instead of returning null values. ## What changes were proposed in this pull request? If a partitionSpec is supposed to not contain optional values, a ParseException should be thrown, and not nulls returned. The nulls can later cause NullPointerExceptions in places not expecting them. ## How was this patch tested? A query like "SHOW PARTITIONS tbl PARTITION(col1='val1', col2)" used to throw a NullPointerException. Now it throws a ParseException. Author: Juliusz Sompolski <julek@databricks.com> Closes #17707 from juliuszsompolski/SPARK-20412. (cherry picked from commit c9e6035) Signed-off-by: Wenchen Fan <wenchen@databricks.com>
peter-toth
pushed a commit
to peter-toth/spark
that referenced
this pull request
Oct 6, 2018
… instead of returning null values. ## What changes were proposed in this pull request? If a partitionSpec is supposed to not contain optional values, a ParseException should be thrown, and not nulls returned. The nulls can later cause NullPointerExceptions in places not expecting them. ## How was this patch tested? A query like "SHOW PARTITIONS tbl PARTITION(col1='val1', col2)" used to throw a NullPointerException. Now it throws a ParseException. Author: Juliusz Sompolski <julek@databricks.com> Closes apache#17707 from juliuszsompolski/SPARK-20412.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
If a partitionSpec is supposed to not contain optional values, a ParseException should be thrown, and not nulls returned.
The nulls can later cause NullPointerExceptions in places not expecting them.
How was this patch tested?
A query like "SHOW PARTITIONS tbl PARTITION(col1='val1', col2)" used to throw a NullPointerException.
Now it throws a ParseException.