[SPARK-51708] Add CaseInsensitiveDictionary
#40
Conversation
|
Could you review this PR when you have some time, @LuciferYang ? |
|
Sure, I'll take a look at it later. |
|
Thank you so much. Take your time~ |
|
|
||
| dict["key3"] = 2025 | ||
| #expect(dict.count == 3) | ||
| #expect(dict["key3"] as! Int == 2025) |
There was a problem hiding this comment.
Is the scenario where dict["key3"] = nil is supported as expected? I tested it, and the result seems to be correct.
There was a problem hiding this comment.
Yes, the removal of keys are working as you expected.
Initially, I thought you are saying that supporting nil as a valid value. That is not supported because we are using [String: Sendable] type. To support nil as a value, we need to use [String: Sendable?].
Anyway, you are right in terms of the removal of keys. I accepted all the other code suggestions too.
| } | ||
|
|
||
| public func toStringDictionary() -> [String: String] { | ||
| var dict = [String: String]() |
There was a problem hiding this comment.
return originalDictionary.mapValues { String(describing: $0) } appears to be more concise, but it might be slower than the current implementation?
There was a problem hiding this comment.
Thank you. I updated it with your suggestion.
| return keyLowerCasedDictionary[key.lowercased()] | ||
| } | ||
| set { | ||
| var newMap = originalDictionary.filter { $0.key.caseInsensitiveCompare(key) != .orderedSame } |
There was a problem hiding this comment.
how about
set {
let lowerKey = key.lowercased()
if let newValue = newValue {
keyLowerCasedDictionary[lowerKey] = newValue
} else {
keyLowerCasedDictionary.removeValue(forKey: lowerKey)
}
originalDictionary = originalDictionary.filter { $0.key.lowercased() != lowerKey }
if let newValue = newValue {
originalDictionary[key] = newValue
}
}
There was a problem hiding this comment.
Actually, the original code pattern roughly follows Apache Spark code pattern which creates a new map.
There was a problem hiding this comment.
Let me update with yours.
|
Thank you for review, @LuciferYang . The updated PR looks much better to me. 😄 |
|
Thank you again, @LuciferYang . Merged to main. |
What changes were proposed in this pull request?
This PR aims to add
CaseInsensitiveDictionaryand use it in the following classes.DataFrameReaderDataFrameWriterWhy are the changes needed?
For feature parity.
Does this PR introduce any user-facing change?
No, this is a new addition to the unreleased version.
How was this patch tested?
Pass the CIs.
Was this patch authored or co-authored using generative AI tooling?
No.