fix: validate merge schema field bindings - #7703
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds MERGE schema validation in ChangesMerge schema validation
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 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.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 4192-4219: The merge-related tests are using in-body loops for
input-only variations and only asserting on error strings, so refactor
test_merge_rejects_renumbered_field_ids,
test_merge_rejects_dropped_field_id_reuse,
test_merge_allows_id_preserving_schema_change, and
test_merge_allows_dropping_field into rstest #[case::...] cases for each input
set, and keep each case independent. For the rejection paths, assert the
returned Error variant from commit_merge (for example Error::InvalidInput) in
addition to checking the expected message text, rather than relying only on
err.to_string().contains(...).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: aa3eb425-c5c3-453d-a14c-beb547288fa1
📒 Files selected for processing (1)
rust/lance/src/dataset/transaction.rs
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
An external Operation::Merge commit installed the caller-supplied schema without any field id validation. A schema whose field ids were renumbered (e.g. by a lossy round-trip through a format that does not store field ids, after a column drop + add) silently rebinds column data: readers resolve name -> schema id -> DataFile::fields position, so a live column re-points at a dropped column's bytes. Add merge_schema_valid to validate_operation's Merge arm: - Every field id that already exists in the dataset schema must resolve to the same field path in the new schema. - Every field id new to the schema must be greater than Manifest::max_field_id(), so the id of a dropped field (still referenced by data files) is never reused for a different column. Dropping fields by omitting them from the merge schema remains legal, and internal callers (Dataset::merge, add_columns) already assign new ids above the manifest max, so id-preserving merges are unaffected. Fixes lance-format#7700
… validation tests Convert the in-body for loops in test_merge_rejects_renumbered_field_ids, test_merge_allows_id_preserving_schema_change, and test_merge_allows_dropping_field to rstest #[case::...] cases so each input fails independently, and assert Error::InvalidInput on every rejection path in addition to the message content. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
79772a0 to
cc9f74c
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 4201-4202: Replace the temp-directory setup in the new transaction
tests with plain in-memory dataset URIs using memory://, following the
repository test guideline; update the affected test cases around
dataset_with_dropped_column and the other listed scenarios to stop constructing
TempStrDir or any unique filesystem-backed paths, and ensure the helper calls
and assertions still work with in-memory datasets.
- Around line 4243-4244: The test-only inline imports in the transaction tests
should be moved out of the function body and added to the existing module-level
import block. Update the relevant test module around the `StructArray` and
`Fields` usage so the imports are grouped with the other `use` statements at the
top of the file, following the same pattern used elsewhere in `transaction.rs`.
- Line 1864: The next-manifest version calculation in the transaction flow
should not use plain arithmetic because `m.version + 1` can wrap at `u64::MAX`
and produce invalid metadata. Update the logic around the current manifest
handling in the transaction code to use a checked increment when computing the
next version, and return an error if the version would overflow instead of
continuing. Use the existing manifest/version path in the transaction
implementation to locate the change.
- Line 4161: The test helper in transaction.rs is using
RecordBatch::column_by_name, but test code should access columns with batch
indexing instead. Update the helper to use batch["column_name"] style access in
place of the current column_by_name call, keeping the change local to the
test-only logic around the batch variable so it matches the coding guidelines.
- Around line 3892-3907: The merge validation in merge_schema_valid only checks
for field id/path remaps, so shared fields can still change type or nullability
and slip through. Update the shared-field comparison in
rust/lance/src/dataset/transaction.rs to compare each field found in both
prior_schema and new_schema by id, and reject any semantic changes (at least
data type and nullability, and any other field metadata that must remain stable)
before allowing the merge to proceed.
- Around line 3918-3919: The validation error in the transaction flow can
overflow when computing the next field id from max_field_id + 1. Update the
logic in transaction.rs around the max_field_id validation to use checked_add
when deriving the next id, and if max_field_id is i32::MAX return an
exhaustion-style error instead of formatting an overflowing value. Keep the
change localized to the max_field_id validation/error construction path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f7835e8-6b34-4eb9-8231-37c32874d42e
📒 Files selected for processing (1)
rust/lance/src/dataset/transaction.rs
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 4201-4202: Replace the temp-directory setup in the new transaction
tests with plain in-memory dataset URIs using memory://, following the
repository test guideline; update the affected test cases around
dataset_with_dropped_column and the other listed scenarios to stop constructing
TempStrDir or any unique filesystem-backed paths, and ensure the helper calls
and assertions still work with in-memory datasets.
- Around line 4243-4244: The test-only inline imports in the transaction tests
should be moved out of the function body and added to the existing module-level
import block. Update the relevant test module around the `StructArray` and
`Fields` usage so the imports are grouped with the other `use` statements at the
top of the file, following the same pattern used elsewhere in `transaction.rs`.
- Line 1864: The next-manifest version calculation in the transaction flow
should not use plain arithmetic because `m.version + 1` can wrap at `u64::MAX`
and produce invalid metadata. Update the logic around the current manifest
handling in the transaction code to use a checked increment when computing the
next version, and return an error if the version would overflow instead of
continuing. Use the existing manifest/version path in the transaction
implementation to locate the change.
- Line 4161: The test helper in transaction.rs is using
RecordBatch::column_by_name, but test code should access columns with batch
indexing instead. Update the helper to use batch["column_name"] style access in
place of the current column_by_name call, keeping the change local to the
test-only logic around the batch variable so it matches the coding guidelines.
- Around line 3892-3907: The merge validation in merge_schema_valid only checks
for field id/path remaps, so shared fields can still change type or nullability
and slip through. Update the shared-field comparison in
rust/lance/src/dataset/transaction.rs to compare each field found in both
prior_schema and new_schema by id, and reject any semantic changes (at least
data type and nullability, and any other field metadata that must remain stable)
before allowing the merge to proceed.
- Around line 3918-3919: The validation error in the transaction flow can
overflow when computing the next field id from max_field_id + 1. Update the
logic in transaction.rs around the max_field_id validation to use checked_add
when deriving the next id, and if max_field_id is i32::MAX return an
exhaustion-style error instead of formatting an overflowing value. Keep the
change localized to the max_field_id validation/error construction path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f7835e8-6b34-4eb9-8231-37c32874d42e
📒 Files selected for processing (1)
rust/lance/src/dataset/transaction.rs
🛑 Comments failed to post (6)
rust/lance/src/dataset/transaction.rs (6)
1864-1864: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1848,1880p' rust/lance/src/dataset/transaction.rsRepository: lance-format/lance
Length of output: 1569
🏁 Script executed:
sed -n '2288,2365p' rust/lance/src/dataset/transaction.rsRepository: lance-format/lance
Length of output: 3696
🏁 Script executed:
sed -n '3700,4360p' rust/lance/src/dataset/transaction.rsRepository: lance-format/lance
Length of output: 25316
🏁 Script executed:
rg -n "max_field_id|merge_schema_valid|checked_add|version \+ 1|manifest.version" rust/lance/src/dataset/transaction.rsRepository: lance-format/lance
Length of output: 1102
🏁 Script executed:
rg -n "struct Manifest|pub version:|version: .*u64|version: .*i32|type Version" rust/lance/src/dataset rust/lance/src -g '!**/target/**'Repository: lance-format/lance
Length of output: 16358
🏁 Script executed:
sed -n '1,220p' rust/lance/src/dataset/manifest.rsRepository: lance-format/lance
Length of output: 234
🏁 Script executed:
sed -n '1,120p' rust/lance/src/dataset/transaction.rsRepository: lance-format/lance
Length of output: 5710
Use checked_add for the next manifest version.
m.version + 1can wrap atu64::MAX, which would write an invalid version into manifest/WAL metadata. Return an error instead.🤖 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/src/dataset/transaction.rs` at line 1864, The next-manifest version calculation in the transaction flow should not use plain arithmetic because `m.version + 1` can wrap at `u64::MAX` and produce invalid metadata. Update the logic around the current manifest handling in the transaction code to use a checked increment when computing the next version, and return an error if the version would overflow instead of continuing. Use the existing manifest/version path in the transaction implementation to locate the change.Source: Coding guidelines
3892-3907: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Verify whether downstream validation already rejects type/nullability changes for shared ids. rg -nP 'fn schema_fragments_valid|schema_fragments_valid\(|field_by_id|DataType|nullable' rust/lance/src/dataset/transaction.rs rust/lance-core/src/datatypes/schema.rsRepository: lance-format/lance
Length of output: 29945
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the merge/schema validation logic around the cited code and the shared-id validator. sed -n '3753,3925p' rust/lance/src/dataset/transaction.rs printf '\n--- schema.rs relevant validation ---\n' sed -n '860,980p' rust/lance-core/src/datatypes/schema.rsRepository: lance-format/lance
Length of output: 11153
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the merge flow around schema validation and how the new schema is applied. sed -n '3600,3925p' rust/lance/src/dataset/transaction.rs printf '\n--- any schema comparison hooks in transaction.rs ---\n' rg -n 'compare_fields|compare_with_options|SchemaCompareOptions|allow_missing_if_nullable|allow_subschema|merge_schema_valid' rust/lance/src/dataset/transaction.rs rust/lance-core/src/datatypes/schema.rsRepository: lance-format/lance
Length of output: 16356
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect field comparison semantics to see what counts as a schema change. sed -n '1,260p' rust/lance-core/src/datatypes/field.rs printf '\n--- compare-related uses ---\n' rg -n 'compare_with_options|metadata|nullable|data_type' rust/lance-core/src/datatypes/field.rs rust/lance-core/src/datatypes/schema.rsRepository: lance-format/lance
Length of output: 31608
Reject shared-id schema changes that alter field semantics. In
rust/lance/src/dataset/transaction.rs:3889-3907,merge_schema_valid()only blocks id/path remaps, so a merge can keep the same field id and path while changing type or nullability and still pass. Compare shared fields by id before accepting the new schema.🤖 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/src/dataset/transaction.rs` around lines 3892 - 3907, The merge validation in merge_schema_valid only checks for field id/path remaps, so shared fields can still change type or nullability and slip through. Update the shared-field comparison in rust/lance/src/dataset/transaction.rs to compare each field found in both prior_schema and new_schema by id, and reject any semantic changes (at least data type and nullability, and any other field metadata that must remain stable) before allowing the merge to proceed.
3918-3919: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '3890,3945p' rust/lance/src/dataset/transaction.rsRepository: lance-format/lance
Length of output: 2446
🏁 Script executed:
rg -n "fn max_field_id|max_field_id\(" rust/lance/src rust/lance-core/src rust/lance-table/srcRepository: lance-format/lance
Length of output: 3703
🏁 Script executed:
rg -n "field.id|struct Field|type .*FieldId|i32::MAX|max_field_id" rust/lance/src rust/lance-core/src rust/lance-table/srcRepository: lance-format/lance
Length of output: 50374
🏁 Script executed:
sed -n '400,440p' rust/lance-table/src/format/manifest.rsRepository: lance-format/lance
Length of output: 1717
🏁 Script executed:
sed -n '650,710p' rust/lance-core/src/datatypes/schema.rsRepository: lance-format/lance
Length of output: 2536
🏁 Script executed:
sed -n '1,120p' rust/lance-core/src/datatypes/field.rsRepository: lance-format/lance
Length of output: 4464
🏁 Script executed:
sed -n '1,220p' rust/lance-core/src/datatypes/field.rs | rg -n "set_id|max_id|id:"Repository: lance-format/lance
Length of output: 205
🏁 Script executed:
sed -n '720,780p' rust/lance/src/dataset/schema_evolution.rsRepository: lance-format/lance
Length of output: 2566
Avoid overflowing
max_field_id + 1in the validation error.If
manifest.max_field_id()isi32::MAX, this+ 1panics in debug and wraps in release while formatting the error. Compute the next id withchecked_addand return an exhaustion error instead.🤖 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/src/dataset/transaction.rs` around lines 3918 - 3919, The validation error in the transaction flow can overflow when computing the next field id from max_field_id + 1. Update the logic in transaction.rs around the max_field_id validation to use checked_add when deriving the next id, and if max_field_id is i32::MAX return an exhaustion-style error instead of formatting an overflowing value. Keep the change localized to the max_field_id validation/error construction path.Source: Coding guidelines
4161-4161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use batch indexing in tests.
This new test helper uses
column_by_name; test code should usebatch[...]forRecordBatchcolumn access.As per coding guidelines: "Use
column_by_name()forRecordBatchcolumn access in production code; usebatch["column_name"]in tests."Proposed fix
- let col = batch.column_by_name(name).unwrap(); + let col = &batch[*name];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.let col = &batch[*name];🤖 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/src/dataset/transaction.rs` at line 4161, The test helper in transaction.rs is using RecordBatch::column_by_name, but test code should access columns with batch indexing instead. Update the helper to use batch["column_name"] style access in place of the current column_by_name call, keeping the change local to the test-only logic around the batch variable so it matches the coding guidelines.Source: Coding guidelines
4201-4202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use
memory://for these tests.The new tests allocate temp directories; repository guidance prefers plain in-memory URIs for tests.
As per coding guidelines: "Use plain
"memory://"URIs in tests; no atomic counters or unique suffixes are needed."Example fix
- let test_dir = TempStrDir::default(); - let dataset = dataset_with_dropped_column(test_dir.as_str(), dropped).await; + let dataset = dataset_with_dropped_column("memory://", dropped).await;Also applies to: 4218-4219, 4246-4247, 4297-4298, 4330-4331
🤖 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/src/dataset/transaction.rs` around lines 4201 - 4202, Replace the temp-directory setup in the new transaction tests with plain in-memory dataset URIs using memory://, following the repository test guideline; update the affected test cases around dataset_with_dropped_column and the other listed scenarios to stop constructing TempStrDir or any unique filesystem-backed paths, and ensure the helper calls and assertions still work with in-memory datasets.Source: Coding guidelines
4243-4244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Move test imports to the module import block.
These inline
usestatements should be hoisted next to the other test imports.As per coding guidelines: "Place
useimports at the top of the file, not inline within function bodies."Proposed fix
- use arrow_array::{Int32Array, RecordBatch, RecordBatchIterator}; - use arrow_schema::{DataType, Field as ArrowField, Schema as ArrowSchema}; + use arrow_array::{Int32Array, RecordBatch, RecordBatchIterator, StructArray}; + use arrow_schema::{DataType, Field as ArrowField, Fields, Schema as ArrowSchema}; ... - use arrow_array::StructArray; - use arrow_schema::Fields;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🤖 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/src/dataset/transaction.rs` around lines 4243 - 4244, The test-only inline imports in the transaction tests should be moved out of the function body and added to the existing module-level import block. Update the relevant test module around the `StructArray` and `Fields` usage so the imports are grouped with the other `use` statements at the top of the file, following the same pattern used elsewhere in `transaction.rs`.Source: Coding guidelines
|
|
||
| // Remap errors first: a renumbered schema usually violates both clauses. | ||
| for field in new_schema.fields_pre_order() { | ||
| if prior_schema.field_by_id(field.id).is_none() { |
There was a problem hiding this comment.
This only validates ID-to-path bindings for IDs that already exist. It does not validate path-to-ID or the schema semantics attached to a reused ID. For example, a(id=0) can be submitted as a(id=max+1) with unchanged fragments; validation passes, the manifest installs the new ID, and reads synthesize NULL because the files still contain field 0. The same ID and path can also change Int32 to Float32 or nullable to non-nullable, bypassing the existing schema-evolution checks and reinterpreting old files. These cases still permit silent data loss or corruption through Merge.
| } | ||
| } | ||
|
|
||
| let max_field_id = manifest.max_field_id(); |
There was a problem hiding this comment.
On current main, Manifest::max_field_id() scans the schema and base fragment files but not DataOverlayFile.data_file.fields, while Project retains overlays. If an overlay-only field is dropped, the high-water mark can fall, this check can accept the old ID for a new column, and the retained overlay values become bound to that new column. Overlay datasets therefore remain vulnerable to field-ID reuse.
Xuanwo
changed the title
2 participants
External
Operation::Mergecommits install a caller-supplied schema. Without validating that schema against the current manifest, a renumbered or reused field ID can silently rebind a live column to data from another column. The same risk exists when a shared field ID changes its logical type, nullability, storage encoding, or dictionary while old base or overlay files remain.Validate field bindings before accepting a merge commit. Existing field IDs must keep the same field path, new IDs must be greater than
Manifest::max_field_id(), and semantic binding changes are rejected whenever any old field-bearing file is retained.Complete physical rewrites remain supported: a binding change is allowed only when every old base and overlay file carrying the field is replaced and every proposed fragment materializes the field in a base data file. Field drops and metadata-only updates remain legal.
Fixes #7700