Repository navigation
fix(ipc): Write the actual null count to each FieldNode - #974
Open
rustyconover wants to merge 1 commit into
Open
rustyconover wants to merge 1 commit into
rustyconover wants to merge 1 commit into
Conversation
The encoder copied ArrowArrayView.null_count into FieldNode.null_count. That wrote -1 for an array whose null count is unknown in the C data interface, and whatever a producer put there for a null-typed array, which may be 0. FieldNode.null_count is the number of nulls, so compute it when it is unknown, and write the length for a null array. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #974 +/- ##
=======================================
Coverage 78.29% 78.30%
=======================================
Files 106 106
Lines 16914 16919 +5
Branches 2014 2016 +2
=======================================
+ Hits 13243 13248 +5
Misses 2429 2429
Partials 1242 1242 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
Author
|
@paleolimbot sorry for doing a lot of cross language checks and finding these bugs. I know .Net and C# isn't often run against nanoarrow, but I figured why not run the entire suite of arrow language conformance tests and these came out. |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ArrowIpcEncoderCollectArray()copiedArrowArrayView.null_countinto eachFieldNodeas it was. That is wrong in two cases:null_countmay be -1 when it hasn't been computed. A zero-copy slice often looks like this. The IPC format has no such value:Message.fbsdocumentsFieldNode.null_countas "the number of observed nulls", andColumnar.rsttreats the null count as a property of the array that is "as large as the array length". The encoder wrotenull_count: -1.ValidateArrayreports "Null array null_count unequal to its length", andNullArrayforcesnull_count = length). A producer may still leave a null array'snull_countat 0, and the encoder wrote that 0.The encoder now writes the length for a null array. For other types it computes the null count with
ArrowArrayViewComputeNullCount()when the count is negative. The change is inArrowIpcEncoderCollectArray(), which also collects dictionary values, so it applies toDictionaryBatchmessages as well. The top-level record batch check already handled -1.I found this when a DuckDB extension started writing its IPC with nanoarrow and Apache.Arrow for .NET read the stream. Arrow C++ happens to accept both cases: it maps -1 to
kUnknownNullCountand recomputes it, andNullArrayoverwrites the count. Apache.Arrow 23.0.0 (.NET) does not:null_count: -1throwsInvalidDataException: Null count of -1 must be >= 0 and <= 2147483647. Older versions read the column as having no nulls, so NULLs were silently lost.null_count: 0throwsArgumentException: Length must equal null countfromNullArray..ctor.With this change, both batches read correctly in .NET.
Tests:
NanoarrowIpcEncoderWritesActualNullCountsencodesstruct<i: int32, n: null>withi = [1, null, 3],i.null_count = -1andn.null_count = 0. It decodes the message and checks the field-node null counts, which must be 1 and 3. Onmainthe test fails with -1 and 0.ctestpasses (Linux aarch64, Debug, 393 tests), including under ASan/UBSan with clang. The changed files are formatted with clang-format 16.0.6, the pre-commit version.This PR's code and description were written by AI (Claude Code) and reviewed by a human maintainer.
🤖 Generated with Claude Code