[SPARK-32364][SQL][2.4] Use CaseInsensitiveMap for DataFrameReader/Writer options #29209
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
…options
When a user have multiple options like `path`, `paTH`, and `PATH` for the same key `path`, `option/options` is non-deterministic because `extraOptions` is `HashMap`. This PR aims to use `CaseInsensitiveMap` instead of `HashMap` to fix this bug fundamentally.
Like the following, DataFrame's `option/options` have been non-deterministic in terms of case-insensitivity because it stores the options at `extraOptions` which is using `HashMap` class.
```scala
spark.read
.option("paTh", "1")
.option("PATH", "2")
.option("Path", "3")
.option("patH", "4")
.load("5")
...
org.apache.spark.sql.AnalysisException:
Path does not exist: file:/.../1;
```
Also, this PR adds the following.
1. Add an explicit document to `DataFrameReader/DataFrameWriter`.
2. Add `toMap` to `CaseInsensitiveMap` in order to return `originalMap: Map[String, T]` because it's more consistent with the existing `case-sensitive key names` behavior for the existing code pattern like `AppendData.byName(..., extraOptions.toMap)`. Previously, it was `HashMap.toMap`.
3. During (2), we need to change the following to keep the original logic using `CaseInsensitiveMap.++`.
```scala
- val params = extraOptions.toMap ++ connectionProperties.asScala.toMap
+ val params = extraOptions ++ connectionProperties.asScala
```
4. Additionally, use `.toMap` in the following because `dsOptions.asCaseSensitiveMap()` is used later.
```scala
- val options = sessionOptions ++ extraOptions
+ val options = sessionOptions.filterKeys(!extraOptions.contains(_)) ++ extraOptions.toMap
val dsOptions = new CaseInsensitiveStringMap(options.asJava)
```
`extraOptions.toMap` is used in several places (e.g. `DataFrameReader`) to hand over `Map[String, T]`. In this case, `CaseInsensitiveMap[T] private (val originalMap: Map[String, T])` had better return `originalMap`.
No.
Pass the Jenkins or GitHub Action with the existing tests and newly add test case at `JDBCSuite`.
Closes #29191 from dongjoon-hyun/SPARK-32364-3.
Authored-by: Dongjoon Hyun <dongjoon@apache.org>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit aed8dba)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
a
|
Test build #126430 has finished for PR 29209 at commit
|
|
LGTM. Thanks. |
dongjoon-hyun
commented
Jul 23, 2020
dongjoon-hyun
added a commit
that referenced
this pull request
Jul 23, 2020
…iter options ### What changes were proposed in this pull request? This PR is a backport of SPARK-32364 (#29160, #29191). When a user have multiple options like `path`, `paTH`, and `PATH` for the same key `path`, `option/options` is indeterministic because `extraOptions` is `HashMap`. This PR aims to use `CaseInsensitiveMap` instead of `HashMap` to fix this bug fundamentally. Like the following, DataFrame's `option/options` have been non-deterministic in terms of case-insensitivity because it stores the options at `extraOptions` which is using `HashMap` class. ```scala spark.read .option("paTh", "1") .option("PATH", "2") .option("Path", "3") .option("patH", "4") .load("5") ... org.apache.spark.sql.AnalysisException: Path does not exist: file:/.../1; ``` Also, this PR adds the following. 1. Add an explicit document to `DataFrameReader/DataFrameWriter`. 2. Add `toMap` to `CaseInsensitiveMap` in order to return `originalMap: Map[String, T]` because it's more consistent with the existing `case-sensitive key names` behavior for the existing code pattern like `AppendData.byName(..., extraOptions.toMap)`. Previously, it was `HashMap.toMap`. 3. During (2), we need to change the following to keep the original logic using `CaseInsensitiveMap.++`. ```scala - val params = extraOptions.toMap ++ connectionProperties.asScala.toMap + val params = extraOptions ++ connectionProperties.asScala ``` 4. Additionally, use `.toMap` in the following because `dsOptions.asCaseSensitiveMap()` is used later. ```scala - val options = sessionOptions ++ extraOptions + val options = sessionOptions.filterKeys(!extraOptions.contains(_)) ++ extraOptions.toMap val dsOptions = new CaseInsensitiveStringMap(options.asJava) ``` `extraOptions.toMap` is used in several places (e.g. `DataFrameReader`) to hand over `Map[String, T]`. In this case, `CaseInsensitiveMap[T] private (val originalMap: Map[String, T])` had better return `originalMap`. ### Why are the changes needed? This will fix indeterministic behavior. ### Does this PR introduce _any_ user-facing change? Yes. ### How was this patch tested? Pass the Jenkins with the existing tests and newly add test cases. Closes #29209 from dongjoon-hyun/SPARK-32364-2.4. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
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?
This PR is a backport of SPARK-32364 (#29160, #29191).
When a user have multiple options like
path,paTH, andPATHfor the same keypath,option/optionsis indeterministic becauseextraOptionsisHashMap. This PR aims to useCaseInsensitiveMapinstead ofHashMapto fix this bug fundamentally.Like the following, DataFrame's
option/optionshave been non-deterministic in terms of case-insensitivity because it stores the options atextraOptionswhich is usingHashMapclass.Also, this PR adds the following.
Add an explicit document to
DataFrameReader/DataFrameWriter.Add
toMaptoCaseInsensitiveMapin order to returnoriginalMap: Map[String, T]because it's more consistent with the existingcase-sensitive key namesbehavior for the existing code pattern likeAppendData.byName(..., extraOptions.toMap). Previously, it wasHashMap.toMap.During (2), we need to change the following to keep the original logic using
CaseInsensitiveMap.++..toMapin the following becausedsOptions.asCaseSensitiveMap()is used later.extraOptions.toMapis used in several places (e.g.DataFrameReader) to hand overMap[String, T]. In this case,CaseInsensitiveMap[T] private (val originalMap: Map[String, T])had better returnoriginalMap.Why are the changes needed?
This will fix indeterministic behavior.
Does this PR introduce any user-facing change?
Yes.
How was this patch tested?
Pass the Jenkins with the existing tests and newly add test cases.