fix: accept masked nulls in non-nullable struct children when writing…#7844
fix: accept masked nulls in non-nullable struct children when writing…#7844mediamana wants to merge 5 commits into
Conversation
github-actions
Bot
added
A-encoding
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: QUIET Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughNullability validation is centralized and now accounts for ancestor struct validity when writing format version 2.1 or later. Legacy validation remains strict, while tests cover nested structs and compaction with null structs and non-nullable children. ChangesNullability validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DatasetCompaction
participant FileWriter
participant verify_field_nullability
DatasetCompaction->>FileWriter: write scanner output with masked child nulls
FileWriter->>verify_field_nullability: validate fields for format version 2.1
verify_field_nullability-->>FileWriter: accept child nulls masked by parent validity
FileWriter-->>DatasetCompaction: complete compaction roundtrip
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-file/src/writer.rs-280-284 (1)
280-284: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winInclude the unmasked null count in the error. The message names the field but drops the diagnostic value that was just computed. Surfacing
unmasked_null_counthelps distinguish "some rows unmasked" from a fully-unmasked column when a write fails.♻️ Proposed message enrichment
- return Err(Error::invalid_input(format!( - "The field `{}` contained null values even though the field is marked non-null in the schema", - field.name - ))); + return Err(Error::invalid_input(format!( + "The field `{}` contained {} null value(s) not masked by a null ancestor struct even though the field is marked non-null in the schema", + field.name, unmasked_null_count + )));As per coding guidelines: "Include full context in error messages such as variable names, values, sizes, types, and indices; avoid generic messages."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-file/src/writer.rs` around lines 280 - 284, Update the error construction in the null-validation path to include the computed unmasked_null_count alongside field.name. Preserve the existing invalid-input error behavior while adding the count value and clear context to the message.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@rust/lance-file/src/writer.rs`:
- Around line 280-284: Update the error construction in the null-validation path
to include the computed unmasked_null_count alongside field.name. Preserve the
existing invalid-input error behavior while adding the count value and clear
context to the message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 270a7a15-0c19-4f54-a742-ccfcd4338e21
📒 Files selected for processing (3)
rust/lance-file/src/previous/writer/mod.rsrust/lance-file/src/writer.rsrust/lance/src/dataset/optimize.rs
7c8c338 to
f57287f
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
rust/lance-file/src/writer.rs (2)
269-284: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a
debug_assert!on the subtraction invariant.
combined.null_count() - mask.null_count()relies onNullBuffer::union's "null if null in either input" semantics (confirmed correct via arrow docs), but this is an unenforced assumption from an external library. A future arrow-rs change or an unexpected code path here (e.g. mismatched buffer lengths) would panic in debug builds or silently wrap to a hugeusizein release builds, causing spurious rejection of valid writes rather than corrupting data — but still a hard-to-diagnose failure mode.🛡️ Proposed defensive assertion
(Some(nulls), Some(mask)) => NullBuffer::union(Some(nulls), Some(mask)) - .map(|combined| combined.null_count() - mask.null_count()) + .map(|combined| { + debug_assert!( + combined.null_count() >= mask.null_count(), + "union of nulls must be a superset of the mask" + ); + combined.null_count() - mask.null_count() + }) .unwrap_or(0),As per coding guidelines, "Prefer
debug_assert!overassert!for non-safety invariants" and "Do not silently guard against impossible conditions; usedebug_assert!, return an explicit error, or remove the check."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-file/src/writer.rs` around lines 269 - 284, Add a debug_assert! before subtracting null counts in the unmasked-null calculation within the writer’s non-null field validation, verifying combined.null_count() is at least mask.null_count(). Keep the existing union-based count and error behavior unchanged.Source: Coding guidelines
1680-1818: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of single-level masking/gating; consider adding a nested (struct-in-struct) case.
This test thoroughly covers masked-vs-unmasked nulls and the 2.0/2.1 gate for one level of struct nesting. The recursive offset-alignment logic in
verify_field_nullability(child_arr.slice(arr.offset(), arr.len())compounding across levels) is exactly the part that a depth-≥2 nested struct (e.g.Struct<Struct<non-nullable f64>>, both levels null-masked) would exercise but isn't tested anywhere in this PR.As per coding guidelines, "Every bugfix and feature must have corresponding tests," and multi-level nesting is the untested edge of this new recursive logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-file/src/writer.rs` around lines 1680 - 1818, Extend test_masked_nulls_in_non_nullable_struct_child with a depth-two Struct<Struct<non-nullable Float64>> case where nulls are masked at both parent levels, including roundtrip assertions for validity and values. Ensure the test uses the same V2.1 path and exercises recursive offset alignment in verify_field_nullability, while preserving the existing V2.0 and unmasked-null coverage.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rust/lance-file/src/writer.rs`:
- Around line 269-284: Add a debug_assert! before subtracting null counts in the
unmasked-null calculation within the writer’s non-null field validation,
verifying combined.null_count() is at least mask.null_count(). Keep the existing
union-based count and error behavior unchanged.
- Around line 1680-1818: Extend test_masked_nulls_in_non_nullable_struct_child
with a depth-two Struct<Struct<non-nullable Float64>> case where nulls are
masked at both parent levels, including roundtrip assertions for validity and
values. Ensure the test uses the same V2.1 path and exercises recursive offset
alignment in verify_field_nullability, while preserving the existing V2.0 and
unmasked-null coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 8c160853-5832-447e-811c-c6e53ee33b78
📒 Files selected for processing (3)
rust/lance-file/src/previous/writer/mod.rsrust/lance-file/src/writer.rsrust/lance/src/dataset/optimize.rs
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
rust/lance-file/src/writer.rs-1944-1952 (1)
1944-1952: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the error variant, not just the message.
The negative case only checks
err.contains("non-null")on the stringified error. Assert the concrete variant (Error::InvalidInput) as well so a future error-message reword or a different failure path (e.g. a build/IO error also containing that substring) doesn't silently pass this test. Same applies to the sibling assertions intest_masked_nulls_in_non_nullable_struct_child.As per coding guidelines: "Assert on both the error variant and the message content in tests; do not check only
is_err()."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/lance-file/src/writer.rs` around lines 1944 - 1952, Update the negative assertions in the current test and its sibling test test_masked_nulls_in_non_nullable_struct_child to preserve the full error value, assert that it is the concrete Error::InvalidInput variant, and separately verify the message contains “non-null”. Replace the current string-only unwrap_err flow without weakening the existing message check.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@rust/lance-file/src/writer.rs`:
- Around line 1944-1952: Update the negative assertions in the current test and
its sibling test test_masked_nulls_in_non_nullable_struct_child to preserve the
full error value, assert that it is the concrete Error::InvalidInput variant,
and separately verify the message contains “non-null”. Replace the current
string-only unwrap_err flow without weakening the existing message check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 1ded9ba4-bc9a-4f1a-ab08-2c2fc14ddf9b
📒 Files selected for processing (1)
rust/lance-file/src/writer.rs
1 participant
Closes #7826
Problem
compact_files()(and any other read→write roundtrip) fails withInvalid user input: The field 'x' contained null values even though the field is marked non-null in the schemaon any 2.1 dataset containing at least one null struct whose children are non-nullable.
The write pipeline is self-contradictory for this shape:
verify_field_nullabilityrejects any null in a non-nullable child, even when it is masked by the parent struct's validity.StructStructuralEncoder::maybe_encode, callspushdown_nulls(), which pushes the parent's validity into the children — producing exactly the masked-null shape the validation just refused.StructArray::try_new, which accepts masked nulls.So Lance's own scanner output for a null struct with non-nullable children can never be written back: compaction breaks permanently once a single such row exists. This hits every GeoArrow-style dataset (the spec mandates
struct<x: double not null, y: double not null>with validity carried by the struct) that has any row without a geometry.Fix
verify_field_nullabilitynow accepts nulls in a non-nullable struct child when every null is masked by a null ancestor struct — the same "unmasked nulls" semantics as Arrow'sStructArray::try_new. Unmasked nulls are rejected exactly as before.The relaxation is gated to file versions that encode struct validity (2.1+):
SimpleStructheader, no validity buffer), so a masked child null would read back unmasked and failStructArray::try_newat decode time. Masked nulls stay rejected there (verified by test).No encoder/decoder change is needed: the 2.1 structural encoding already handles this shape end-to-end (repdef layers via
pushdown_nullson write, masked-null children fromunravel_validityon read).Tests
lance-file:test_masked_nulls_in_non_nullable_struct_child— masked nulls accepted and roundtripped on 2.1, rejected on 2.0, unmasked nulls still rejected.lance:test_compact_null_structs_with_non_nullable_children— end-to-end repro ofcompact_files()fails on any dataset containing null structs with non-nullable children — scanner null propagation breaks the read→write roundtrip #7826: a 4-fragment dataset with null structs (validity-first layout, GeoArrow-style) compacts successfully and the data survives intact.Notes
cargo fmt --all -- --checkandcargo clippy -p lance-file -p lance --tests --benches -- -D warningsrun clean.critical-fix: while the error itself is explicit, any writer that catches it (as compaction schedulers typically do) silently stops compacting the whole dataset — we ran 6 weeks in production before noticing.