You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "abandy (via GitHub)" <gi...@apache.org> on 2024/06/09 15:28:15 UTC
[PR] GH-42041: [Swift ] Fix nullable type decoder issue [arrow]
abandy opened a new pull request, #42043:
URL: https://github.com/apache/arrow/pull/42043
### 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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42041: [Swift] Fix nullable type decoder issue [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou merged PR #42043:
URL: https://github.com/apache/arrow/pull/42043
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42041: [Swift ] Fix nullable type decoder issue [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42041: [Swift ] Fix nullable type decoder issue [arrow]
Posted by "abandy (via GitHub)" <gi...@apache.org>.
abandy commented on code in PR #42043:
URL: https://github.com/apache/arrow/pull/42043#discussion_r1632403973
##########
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:
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.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42041: [Swift ] Fix nullable type decoder issue [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #42043:
URL: https://github.com/apache/arrow/pull/42043#issuecomment-2156655354
:warning: GitHub issue #42041 **has been automatically assigned in GitHub** to PR creator.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42041: [Swift ] Fix nullable type decoder issue [arrow]
Posted by "abandy (via GitHub)" <gi...@apache.org>.
abandy commented on code in PR #42043:
URL: https://github.com/apache/arrow/pull/42043#discussion_r1632404098
##########
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:
They can be unified. I had separated them to delineate the nullable and non nullable types but I can merge them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-42041: [Swift] Fix nullable type decoder issue [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #42043:
URL: https://github.com/apache/arrow/pull/42043#issuecomment-2160972530
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 7aaea3d9bb65ad37a17a9d3a52341f0fe2478903.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/26083049070) has more details. It also includes information about 2 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org