[SPARK-26071][SQL] disallow map as map key #23045
Closed
Conversation
This file contains hidden or 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
Contributor
Author
|
Test build #98866 has finished for PR 23045 at commit
|
viirya
reviewed
Nov 15, 2018
| - The `ADD JAR` command previously returned a result set with the single value 0. It now returns an empty result set. | ||
|
|
||
| - In Spark version 2.4 and earlier, users can create map values with map type key via built-in function like `CreateMap`, `MapFromArrays`, etc. Since Spark 3.0, it's not allowed to create map values with map type key. | ||
|
|
Member
There was a problem hiding this comment.
I think should we also add this note?
Note that, maps with map type key still exist, via reading from parquet files, converting from scala/java map, etc. Spark is not to completely forbid map as map key, but to avoid creating it by Spark itself.
|
|
||
| def checkForMapKeyType(keyType: DataType): TypeCheckResult = { | ||
| if (keyType.existsRecursively(_.isInstanceOf[MapType])) { | ||
| TypeCheckResult.TypeCheckFailure("The key of map cannot be/contains map.") |
|
Test build #98901 has finished for PR 23045 at commit
|
|
Test build #98922 has finished for PR 23045 at commit
|
|
Test build #98955 has finished for PR 23045 at commit
|
ueshin
reviewed
Nov 19, 2018
| if (sameTypeCheck.isFailure) { | ||
| sameTypeCheck | ||
| } else { | ||
| TypeUtils.checkForMapKeyType(dataType.keyType) |
Member
There was a problem hiding this comment.
I don't think we need this. The children already should not have map type keys?
Contributor
Author
There was a problem hiding this comment.
see https://github.com/apache/spark/pull/23045/files#diff-3f19ec3d15dcd8cd42bb25dde1c5c1a9R20 . The child may be read from parquet files, so map of map is still possible.
Member
|
LGTM. |
Contributor
Author
|
thanks, merging to master! |
jackylee-ch
pushed a commit
to jackylee-ch/spark
that referenced
this pull request
Feb 18, 2019
## What changes were proposed in this pull request? Due to implementation limitation, currently Spark can't compare or do equality check between map types. As a result, map values can't appear in EQUAL or comparison expressions, can't be grouping key, etc. The more important thing is, map loop up needs to do equality check of the map key, and thus can't support map as map key when looking up values from a map. Thus it's not useful to have map as map key. This PR proposes to stop users from creating maps using map type as key. The list of expressions that are updated: `CreateMap`, `MapFromArrays`, `MapFromEntries`, `MapConcat`, `TransformKeys`. I manually checked all the places that create `MapType`, and came up with this list. Note that, maps with map type key still exist, via reading from parquet files, converting from scala/java map, etc. This PR is not to completely forbid map as map key, but to avoid creating it by Spark itself. Motivation: when I was trying to fix the duplicate key problem, I found it's impossible to do it with map type map key. I think it's reasonable to avoid map type map key for builtin functions. ## How was this patch tested? updated test Closes apache#23045 from cloud-fan/map-key. Authored-by: Wenchen Fan <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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?
Due to implementation limitation, currently Spark can't compare or do equality check between map types. As a result, map values can't appear in EQUAL or comparison expressions, can't be grouping key, etc.
The more important thing is, map loop up needs to do equality check of the map key, and thus can't support map as map key when looking up values from a map. Thus it's not useful to have map as map key.
This PR proposes to stop users from creating maps using map type as key. The list of expressions that are updated:
CreateMap,MapFromArrays,MapFromEntries,MapConcat,TransformKeys. I manually checked all the places that createMapType, and came up with this list.Note that, maps with map type key still exist, via reading from parquet files, converting from scala/java map, etc. This PR is not to completely forbid map as map key, but to avoid creating it by Spark itself.
Motivation: when I was trying to fix the duplicate key problem, I found it's impossible to do it with map type map key. I think it's reasonable to avoid map type map key for builtin functions.
How was this patch tested?
updated test