fix(file): verify 2.1+ nullability against name-selected columns - #9247
Open
jackylee-ch wants to merge 1 commit into
Open
jackylee-ch wants to merge 1 commit into
jackylee-ch wants to merge 1 commit into
Conversation
`EncodingPipeline::verify_nullability_constraints` zips `batch.columns()` against the schema fields positionally, while `encode_batch` selects each field's array with `batch.column_by_name`. When a batch orders its columns differently from the schema, or carries extra ones, the array that gets validated is not the array that gets encoded. Both directions bite: a legal null in a nullable field is rejected while the error names a field that has no nulls, and a null in a non-nullable field passes preflight and is written into a file whose schema declares that field non-null, so readback hands Arrow an array contradicting its own field. lance-format#8507 fixed this same divergence for 2.0 -- its root cause reads "the original guard existed only in the file-writer wrapper, where positional validation could diverge from name-based encoding" -- and the comment it removed pointed at the 2.1+ counterpart, which kept the positional pairing. Reordered batches are a supported input, covered for 2.0 by `test_v2_0_writer_atomically_rejects_reordered_null_structs`. Select by name in the preflight too. A column the schema names but the batch lacks stays with `encode_batch`, which already reports it, so no other behavior moves. ## Testing `cargo test --release -p lance-file --all-features` (188 passed), plus `cargo fmt` and clippy with `-D warnings`. The new `test_structural_writer_verifies_name_selected_nullability` covers both directions across 2.1, 2.2 and 2.3; all six cases fail on the positional pairing.
github-actions
Bot
added
A-encoding
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
Validation now checks the same name-selected array that encoding writes, preserving reordered-batch behavior while rejecting nulls in non-nullable fields. This is the minimal 2.1–2.3 counterpart to #8507, with focused coverage for both mismatch directions.
lance-gatekeeper
Bot
added
the
K-approved
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.
EncodingPipeline::verify_nullability_constraintszipsbatch.columns()against the schema fields positionally, whileencode_batchselects each field's array withbatch.column_by_name. When a batch orders its columns differently from the schema, the validated array is not the encoded one. Both directions bite: a legal null in a nullable field is rejected while the error names a field that has no nulls, and a null in a non-nullable field passes preflight and lands in a file declaring that field non-null.#8507 fixed the same divergence for 2.0, describing it as "positional validation could diverge from name-based encoding", and the comment it removed pointed at this 2.1+ counterpart, which kept the positional pairing.
Select by name here too. A column the schema names but the batch lacks stays with
encode_batch, which reports it.Testing
cargo test --release -p lance-file --all-features(188 passed), plus fmt and clippy with-D warnings. The newtest_structural_writer_verifies_name_selected_nullabilitycovers both directions across 2.1, 2.2 and 2.3; all six cases fail on the positional pairing.