kou commented on code in PR #42043:
URL: https://github.com/apache/arrow/pull/42043#discussion_r1632382830
##########
swift/Arrow/Sources/Arrow/ArrowDecoder.swift:
##########
@@ -126,11 +136,19 @@ private struct ArrowUnkeyedDecoding:
UnkeyedDecodingContainer {
mutating func decodeNil() throws -> Bool {
defer {increment()}
- return try self.decoder.doDecode(self.currentIndex) == nil
+ return try self.decoder.isNull(self.currentIndex)
}
mutating func decode<T>(_ type: T.Type) throws -> T where T: Decodable {
- if type == Int8.self || type == Int16.self ||
+ 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 ||
Review 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.
##########
swift/Arrow/Tests/ArrowTests/CodableTests.swift:
##########
@@ -165,6 +186,30 @@ final class CodableTests: XCTestCase {
case .failure(let err):
throw err
}
- }
+ 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
+ }
Review Comment:
How about creating a new separated test for this?
##########
swift/Arrow/Sources/Arrow/ArrowDecoder.swift:
##########
@@ -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)
Review Comment:
```suggestion
try self.decoder.isNull(key)
```
Hmm. It seems that this file isn't lint target. Could you check it?
##########
swift/Arrow/Tests/ArrowTests/CodableTests.swift:
##########
@@ -134,7 +136,26 @@ final class CodableTests: XCTestCase {
let testData = try decoder.decode(Int8?.self)
for index in 0..<testData.count {
let val: Int8? = testData[index]
- if val != nil {
+ XCTAssertEqual(val!, Int8(index + 10))
+ }
+ case .failure(let err):
+ throw err
+ }
+
+ 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 {
Review Comment:
How about creating a new separated test for this case?
`testArrowSingleDecoderWithoutNull()` and `testArrowSingleDecoderWithNull()`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
