-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GH-39519: [Swift] Fix null count when using reader #39520
Conversation
|
This approach passes arrow/cpp/src/arrow/ipc/reader.cc Line 275 in 1e74ace
If we use this approach in Swift too, we will use API something like the following: diff --git a/swift/Arrow/Sources/Arrow/ArrowReader.swift b/swift/Arrow/Sources/Arrow/ArrowReader.swift
index d9dc1bdb47..c985b37ca6 100644
--- a/swift/Arrow/Sources/Arrow/ArrowReader.swift
+++ b/swift/Arrow/Sources/Arrow/ArrowReader.swift
@@ -88,7 +88,7 @@ public class ArrowReader {
let valueBuffer = loadInfo.recordBatch.buffers(at: loadInfo.bufferIndex + 2)!
let arrowValueBuffer = makeBuffer(valueBuffer, fileData: loadInfo.fileData,
length: UInt(node.length), messageOffset: loadInfo.messageOffset)
- return makeArrayHolder(loadInfo.field, buffers: [arrowNullBuffer, arrowOffsetBuffer, arrowValueBuffer])
+ return makeArrayHolder(loadInfo.field, buffers: [arrowNullBuffer, arrowOffsetBuffer, arrowValueBuffer], UInt(node.nullCount))
} catch let error as ArrowError {
return .failure(error)
} catch {
diff --git a/swift/Arrow/Sources/Arrow/ArrowReaderHelper.swift b/swift/Arrow/Sources/Arrow/ArrowReaderHelper.swift
index fa52160478..13d6209dee 100644
--- a/swift/Arrow/Sources/Arrow/ArrowReaderHelper.swift
+++ b/swift/Arrow/Sources/Arrow/ArrowReaderHelper.swift
@@ -126,7 +126,8 @@ private func makeFixedHolder<T>(
func makeArrayHolder( // swiftlint:disable:this cyclomatic_complexity
_ field: org_apache_arrow_flatbuf_Field,
- buffers: [ArrowBuffer]
+ buffers: [ArrowBuffer],
+ nullCount: UInt
) -> Result<ArrowArrayHolder, ArrowError> {
let type = field.typeType
switch type { What do you think this approach? |
Hi @kou, I have made the requested updates. Please review when you get a chance. Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit e632364. 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. |
Currently the reader is not properly setting the null count when building an array from a stream. This PR adds a fix for this. * Closes: apache#39519 Authored-by: Alva Bandy <abandy@live.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Currently the reader is not properly setting the null count when building an array from a stream. This PR adds a fix for this. * Closes: apache#39519 Authored-by: Alva Bandy <abandy@live.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Currently the reader is not properly setting the null count when building an array from a stream. This PR adds a fix for this. * Closes: apache#39519 Authored-by: Alva Bandy <abandy@live.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Currently the reader is not properly setting the null count when building an array from a stream. This PR adds a fix for this. * Closes: apache#39519 Authored-by: Alva Bandy <abandy@live.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Currently the reader is not properly setting the null count when building an array from a stream. This PR adds a fix for this.