feat: introduce exact file format identity and persisted codecs#7879
feat: introduce exact file format identity and persisted codecs#7879Xuanwo wants to merge 2 commits into
Conversation
|
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)
📝 WalkthroughWalkthroughThis change introduces exact ChangesExact format migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
A-encoding
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/lance/src/dataset/transaction.rs (1)
2299-2308: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.expect()panics on a fallible version conversion inbuild_manifest.
new_file.file_version()returnsResult<LanceFileFormat>and errors on an unrecognized(major, minor)combination. Calling.expect(...)here turns a malformedDataFilein aDataReplacementtransaction into a panic instead of a descriptive commit error.build_manifestalready returnsResult, so this should propagate via?.As per coding guidelines: "Never use
.unwrap(),.expect(),panic!(), orassert!()in library code for fallible operations; use?withResultand proper error types."🛡️ Proposed fix
if columns_covered.is_disjoint(&new_file.fields.iter().collect()) { - new_file - .file_version() - .expect("Expected valid file version"); + new_file.file_version()?; new_frag.files.push(new_file.clone()); }🤖 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 2299 - 2308, In build_manifest, replace the fallible file_version() .expect call in the columns_covered disjoint branch with ? so invalid DataFile version combinations return the existing commit error. Preserve the subsequent new_frag.files.push(new_file.clone()) behavior for valid versions.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.
Inline comments:
In `@rust/lance-file/src/writer.rs`:
- Around line 870-877: Change standard_footer_numbers to return a Result,
replacing the unsupported-version panic with the appropriate error. Update
finish to propagate that error with ?, preserving normal footer-number
conversion for supported versions.
In `@rust/lance/src/dataset/optimize/binary_copy.rs`:
- Around line 199-204: Update rewrite_files_binary_copy’s
LanceFileFormat::from_data_file_numbers conversion to propagate conversion
failures with ? instead of unwrap(). Preserve the existing conversion to
LanceFileVersion and the function’s Result<Vec<Fragment>> error flow.
---
Outside diff comments:
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 2299-2308: In build_manifest, replace the fallible file_version()
.expect call in the columns_covered disjoint branch with ? so invalid DataFile
version combinations return the existing commit error. Preserve the subsequent
new_frag.files.push(new_file.clone()) behavior for valid versions.
🪄 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: QUIET
Plan: Pro Plus
Run ID: 5ba2e859-e7fb-4b2f-99cd-d33d8f0854e6
📒 Files selected for processing (23)
rust/lance-file/src/lib.rsrust/lance-file/src/reader.rsrust/lance-file/src/version.rsrust/lance-file/src/writer.rsrust/lance-table/src/format/fragment.rsrust/lance-table/src/format/manifest.rsrust/lance-table/src/io/commit.rsrust/lance-table/src/io/manifest.rsrust/lance/src/dataset.rsrust/lance/src/dataset/fragment.rsrust/lance/src/dataset/fragment/write.rsrust/lance/src/dataset/optimize.rsrust/lance/src/dataset/optimize/binary_copy.rsrust/lance/src/dataset/optimize/tests/binary_copy.rsrust/lance/src/dataset/schema_evolution.rsrust/lance/src/dataset/tests/dataset_io.rsrust/lance/src/dataset/tests/dataset_merge_update.rsrust/lance/src/dataset/transaction.rsrust/lance/src/dataset/updater.rsrust/lance/src/dataset/write.rsrust/lance/src/dataset/write/commit.rsrust/lance/src/io/commit.rsrust/lance/src/io/commit/conflict_resolver.rs
| fn standard_footer_numbers(&self) -> (u16, u16) { | ||
| let version = self.version(); | ||
| let exact_version = LanceFileFormat::from(version); | ||
| if exact_version == crate::version::LanceFileFormat::V1 { | ||
| panic!("Unsupported version: {}", version); | ||
| } | ||
| exact_version.to_standard_footer_numbers() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace panic! with a returned error on the fallible finish() path.
version() derives from the public FileWriterOptions.format_version, so a caller passing Some(LanceFileVersion::Legacy) reaches this panic! through finish() (which returns Result). Unsupported versions should surface as an error, not a panic.
🛡️ Return an error instead of panicking
- fn standard_footer_numbers(&self) -> (u16, u16) {
- let version = self.version();
- let exact_version = LanceFileFormat::from(version);
- if exact_version == crate::version::LanceFileFormat::V1 {
- panic!("Unsupported version: {}", version);
- }
- exact_version.to_standard_footer_numbers()
- }
+ fn standard_footer_numbers(&self) -> Result<(u16, u16)> {
+ let version = self.version();
+ let exact_version = LanceFileFormat::from(version);
+ if exact_version == LanceFileFormat::V1 {
+ return Err(Error::NotSupported {
+ source: format!(
+ "the v2 file writer cannot write legacy (v1) files; requested version was {}",
+ version
+ )
+ .into(),
+ location: location!(),
+ });
+ }
+ Ok(exact_version.to_standard_footer_numbers())
+ }And update the call site in finish():
- let (major, minor) = self.standard_footer_numbers();
+ let (major, minor) = self.standard_footer_numbers()?;📝 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.
| fn standard_footer_numbers(&self) -> (u16, u16) { | |
| let version = self.version(); | |
| let exact_version = LanceFileFormat::from(version); | |
| if exact_version == crate::version::LanceFileFormat::V1 { | |
| panic!("Unsupported version: {}", version); | |
| } | |
| exact_version.to_standard_footer_numbers() | |
| } | |
| fn standard_footer_numbers(&self) -> Result<(u16, u16)> { | |
| let version = self.version(); | |
| let exact_version = LanceFileFormat::from(version); | |
| if exact_version == LanceFileFormat::V1 { | |
| return Err(Error::NotSupported { | |
| source: format!( | |
| "the v2 file writer cannot write legacy (v1) files; requested version was {}", | |
| version | |
| ) | |
| .into(), | |
| location: location!(), | |
| }); | |
| } | |
| Ok(exact_version.to_standard_footer_numbers()) | |
| } |
🤖 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 870 - 877, Change
standard_footer_numbers to return a Result, replacing the unsupported-version
panic with the appropriate error. Update finish to propagate that error with ?,
preserving normal footer-number conversion for supported versions.
Source: Coding guidelines
| let version: LanceFileVersion = LanceFileFormat::from_data_file_numbers( | ||
| fragments[0].files[0].file_major_version, | ||
| fragments[0].files[0].file_minor_version, | ||
| ) | ||
| .unwrap() | ||
| .resolve(); | ||
| .into(); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
.unwrap() panics on an unrecognized persisted file version in rewrite_files_binary_copy.
LanceFileFormat::from_data_file_numbers errors on any unrecognized (major, minor) combination read from fragments[0].files[0]. Panicking here means a corrupt or unexpectedly-versioned data file crashes a compaction task instead of failing with a descriptive error. The function already returns Result<Vec<Fragment>>, so this should propagate via ? — matching the non-panicking is_ok_and pattern used for the same conversion in can_use_binary_copy_impl (rust/lance/src/dataset/optimize.rs) in this same PR.
As per coding guidelines: "Never use .unwrap(), .expect(), panic!(), or assert!() in library code for fallible operations; use ? with Result and proper error types."
🛡️ Proposed fix
- let version: LanceFileVersion = LanceFileFormat::from_data_file_numbers(
+ let version: LanceFileVersion = LanceFileFormat::from_data_file_numbers(
fragments[0].files[0].file_major_version,
fragments[0].files[0].file_minor_version,
- )
- .unwrap()
- .into();
+ )?
+ .into();📝 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 version: LanceFileVersion = LanceFileFormat::from_data_file_numbers( | |
| fragments[0].files[0].file_major_version, | |
| fragments[0].files[0].file_minor_version, | |
| ) | |
| .unwrap() | |
| .resolve(); | |
| .into(); | |
| let version: LanceFileVersion = LanceFileFormat::from_data_file_numbers( | |
| fragments[0].files[0].file_major_version, | |
| fragments[0].files[0].file_minor_version, | |
| )? | |
| .into(); |
🤖 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/optimize/binary_copy.rs` around lines 199 - 204,
Update rewrite_files_binary_copy’s LanceFileFormat::from_data_file_numbers
conversion to propagate conversion failures with ? instead of unwrap(). Preserve
the existing conversion to LanceFileVersion and the function’s
Result<Vec<Fragment>> error flow.
Source: Coding guidelines
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
1 participant
Part of #7877.
LanceFileVersioncurrently combines user-facing selectors (StableandNext) with exact identities that can be persisted. It also exposes one numeric conversion for wire locations whose compatibility mappings are deliberately different.This first independently mergeable step introduces
LanceFileFormatas the exact, unordered file-format identity while leavingLanceFileVersioninlance-encoding. Selectors resolve before persisted metadata is constructed, and manifest strings,DataFilefields, standard footers, and embedded footers use location-specific codecs.Existing wire mappings, legacy empty-manifest recovery, reader compatibility, and mixed-version rejection remain unchanged. Version-specific dispatch and encoding/compression/dataset execution migration are intentionally deferred to later PRs in the stack.