-
Notifications
You must be signed in to change notification settings - Fork 28.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-40308][SQL] Allow non-foldable delimiter arguments to str_to_map
function
#37763
Conversation
@@ -606,6 +606,36 @@ class StringFunctionsSuite extends QueryTest with SharedSparkSession { | |||
Seq(Row(Map("a" -> "1", "b" -> "2", "c" -> "3"))) | |||
) | |||
|
|||
checkAnswer( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really a test of my change, but it was a missing test (the case where a literal pair delimiter is given but no key/value delimiter is given).
@cloud-fan I wonder why did you insist in #13990 (comment) that the delimiters must be foldable. |
} else { | ||
super.checkInputDataTypes() | ||
} | ||
super.checkInputDataTypes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you invoke only the parent method, let's remove checkInputDataTypes()
then.
+1, LGTM. Merging to master. |
What changes were proposed in this pull request?
Remove the check for foldable delimiter arguments from
StringToMap#checkInputDataTypes
.Except for
checkInputDataTypes
,StringToMap
is already able to handle non-foldable delimiter arguments (no other changes are required).Why are the changes needed?
Hive 2.3.9 allows non-foldable delimiter arguments, e.g.:
This returns
However, Spark returns an error:
The use-case is more likely to be something like this:
Does this PR introduce any user-facing change?
Yes, users can now specify non-foldable delimiter arguments to
str_to_map
.Literals are still accepted, so the change is backwardly compatible.
How was this patch tested?
New unit tests.