GH-42041: [Swift] Fix nullable type decoder issue #42043
Conversation
|
|
| @@ -173,7 +191,7 @@ private struct ArrowKeyedDecoding<Key: CodingKey>: KeyedDecodingContainerProtoco | |||
| } | |||
|
|
|||
| func decodeNil(forKey key: Key) throws -> Bool { | |||
| return try self.decoder.doDecode(key) == nil | |||
| try self.decoder.isNull(key) | |||
There was a problem hiding this comment.
| try self.decoder.isNull(key) | |
| try self.decoder.isNull(key) |
Hmm. It seems that this file isn't lint target. Could you check it?
There was a problem hiding this comment.
Good catch! The file is being checked by the linter but apparently a space off doesn't cause it to fail :). I will update accordingly.
| if type == Int8?.self || type == Int16?.self || | ||
| type == Int32?.self || type == Int64?.self || | ||
| type == UInt8?.self || type == UInt16?.self || | ||
| type == UInt32?.self || type == UInt64?.self || | ||
| type == String?.self || type == Double?.self || | ||
| type == Float?.self || type == Date?.self { | ||
| defer {increment()} | ||
| return try self.decoder.doDecode(self.currentIndex)! | ||
| } else if type == Int8.self || type == Int16.self || |
There was a problem hiding this comment.
I'm not familiar with Swift but can we unify this if and else if? It seems that body codes of them are same.
There was a problem hiding this comment.
They can be unified. I had separated them to delineate the nullable and non nullable types but I can merge them.
| let stringWNilBuilder = try ArrowArrayBuilders.loadStringArrayBuilder() | ||
| stringWNilBuilder.append(nil, "test1", nil, "test3") | ||
| let resultWNil = RecordBatch.Builder() | ||
| .addColumn("propInt8", arrowArray: try int8Builder.toHolder()) | ||
| .addColumn("propString", arrowArray: try stringWNilBuilder.toHolder()) | ||
| .finish() | ||
| switch resultWNil { | ||
| case .success(let rb): | ||
| let decoder = ArrowDecoder(rb) | ||
| let testData = try decoder.decode([Int8: String?].self) | ||
| var index: Int8 = 0 | ||
| for data in testData { | ||
| let str = data[10 + index] | ||
| if index % 2 == 0 { | ||
| XCTAssertNil(str!) | ||
| } else { | ||
| XCTAssertEqual(str, "test\(index)") | ||
| } | ||
| index += 1 | ||
| } | ||
| case .failure(let err): | ||
| throw err | ||
| } |
There was a problem hiding this comment.
How about creating a new separated test for this?
| let int8WNilBuilder: NumberArrayBuilder<Int8> = try ArrowArrayBuilders.loadNumberArrayBuilder() | ||
| int8WNilBuilder.append(10, nil, 12, nil) | ||
| let resultWNil = RecordBatch.Builder() | ||
| .addColumn("propInt8", arrowArray: try int8WNilBuilder.toHolder()) | ||
| .finish() | ||
| switch resultWNil { | ||
| case .success(let rb): | ||
| let decoder = ArrowDecoder(rb) | ||
| let testData = try decoder.decode(Int8?.self) | ||
| for index in 0..<testData.count { | ||
| let val: Int8? = testData[index] | ||
| if index % 2 == 1 { | ||
| XCTAssertNil(val) | ||
| } else { |
There was a problem hiding this comment.
How about creating a new separated test for this case?
testArrowSingleDecoderWithoutNull() and testArrowSingleDecoderWithNull()?
github-actions
bot
added
awaiting merge
kou
changed the title
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 7aaea3d. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them. |
Rationale for this change
There is an issue when decoding nullable types. The previous method of checking for nil values always returned false for nullable types due too the ArrowArray types being non nullable.
What changes are included in this PR?
This PR adds a IsNull method to the ArrowDecoder to be used for null checks. Also, a check for nullable types has been added to the Unkeyed decode method.
Are these changes tested?
Yes, tests have been added/modified to test this fix.