fix: count only visible nulls in the 2.1+ writer - #9020
Merged
jackye1995 merged 4 commits intoSep 6, 2026
Conversation
A struct column that this writer produced could not be written back after a round trip. verify_field_nullability walked into a struct's children without regard for the parent's validity, so a null in a non-nullable child failed the write even where a null parent slot masked it -- and a reader hands back exactly that, because a null struct slot leaves its children undefined and they materialize as null. Arrow draws the line where this now does: StructArray::try_new rejects only the nulls in a non-nullable child that the parent's null mask does not cover, and refuses to build the array this check was reporting. The 2.0 writer already documented the split, pointing at a writer::nullability that was never written; that comment now points at the real thing, and 2.0 keeps rejecting masked nulls because its logical encoders genuinely cannot store the slot. Struct children are the only ones that sit one slot per parent slot, so they are the only ones masked; list-like children are addressed through offsets and are checked as before. Children are sliced to the parent's window first, so a sliced batch lines up.
github-actions
Bot
added
A-encoding
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
lance-gatekeeper
Bot
removed
the
K-changes
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
lance-gatekeeper
Bot
added
the
K-changes
The reachability mask was applied to physical validity, which does not carry every null Arrow can see. A dictionary key pointing at a null value is a null with no bit in the validity buffer, so masking physical validity alone let one through into a file Arrow refuses on readback -- the writer would have produced something it could not read. Reading nulls through logical_nulls closes that, and matches what StructArray::try_new validates against, which is the contract this check is meant to keep. Working from typed arrays also removes the manual child slicing: StructArray::columns are already cut to the parent's window. The regression case is the dictionary the gate described -- values [10, null], keys [0, 1, null], parent validity [true, true, false] -- where row 2's physical null is masked and row 1's logical null is not.
lance-gatekeeper
Bot
removed
the
K-changes
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
Both earlier findings are resolved. The 2.1+ writer now evaluates Arrow logical nulls only where struct slots are reachable, preserving masked child nulls across write/read/write while still rejecting visible logical nulls. The focused regressions cover masked, unmasked, sliced-parent, and dictionary cases.
lance-gatekeeper
Bot
added
the
K-approved
This was referenced Sep 6, 2026
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
1 participant
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.
A struct column this writer produced cannot be written back after a round trip.
verify_field_nullabilitywalked into a struct's children without regard for the parent's validity, so a null in a non-nullable child failed the write even where a null parent slot masked it:A reader hands back exactly that. A null struct slot leaves its children undefined, and they materialize as null — so
write(read(write(x)))fails on data this writer had just accepted. It showed up in a filtered UDF refresh, which merges stored values with recomputed ones and therefore re-writes rows it read; a full refresh writes only freshly built arrays and never hit it.Arrow draws the line where this now does.
StructArray::try_newrejects only the nulls in a non-nullable child that the parent's null mask does not cover, and in fact refuses to construct the array this check was reporting:The 2.0 writer already documented the split — "the 2.1+ structural writer counts only visible nulls (
writer::nullability)" — but that module was never written. The comment now points at the real thing, and 2.0 keeps rejecting masked nulls, since its logical encoders genuinely cannot store the slot.Struct children are the only ones that sit one slot per parent slot, so they are the only ones masked; list-like children are addressed through offsets and are checked as before. Children are sliced to the parent's window first so a sliced batch lines up, which the third test covers.