feat(format): read and write sparse structural pages#7889
Merged
Conversation
Co-authored-by: Weston Pace <weston.pace@gmail.com>
Xuanwo
added
enhancement
Contributor
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
📝 WalkthroughWalkthroughChangesSparse structural encoding
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Xuanwo
changed the title
Xuanwo
removed
enhancement
Xuanwo
changed the title
westonpace
approved these changes
Jul 21, 2026
westonpace
left a comment
westonpace
left a comment
Member
There was a problem hiding this comment.
Replaying approval from #7754
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: 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-encoding/src/encodings/logical/primitive/sparse/writer.rs`:
- Around line 1788-1810: Update test_explicit_sparse_rejects_lance_2_2 to assert
that both create_encoder failures return the expected Error variant, while
retaining the existing message-content checks. Apply the same variant assertion
to the regular nullable input and structural_only case, using matches! or the
project’s established error-pattern style.
🪄 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: 668aab78-0b61-4fbd-8335-3ecba15aa5fa
📒 Files selected for processing (19)
docs/src/format/file/encoding.mddocs/src/format/file/versioning.mdprotos/encodings_v2_1.protoprotos/file2.protorust/lance-encoding/src/constants.rsrust/lance-encoding/src/decoder.rsrust/lance-encoding/src/encodings/logical/fixed_size_list.rsrust/lance-encoding/src/encodings/logical/list.rsrust/lance-encoding/src/encodings/logical/map.rsrust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-encoding/src/encodings/logical/primitive/miniblock.rsrust/lance-encoding/src/encodings/logical/primitive/sparse.rsrust/lance-encoding/src/encodings/logical/primitive/sparse/writer.rsrust/lance-encoding/src/encodings/logical/struct.rsrust/lance-encoding/src/encodings/physical/bitpacking.rsrust/lance-encoding/src/repdef.rsrust/lance-encoding/src/testing.rsrust/lance-file/src/reader.rsrust/lance/benches/s3_file_reader_diagnostics.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
rust/lance-encoding/src/repdef.rs (2)
1681-1688: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument sparse validation failures on this public API.
Line 1686 now returns
Result; add# Errorsdescribing malformed sparse structural metadata, plus a synchronized usage example/link to the sparse unraveller contract.As per coding guidelines, public APIs require semantic documentation, examples, and links to relevant methods or structs.
🤖 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-encoding/src/repdef.rs` around lines 1681 - 1688, Update the public RepDef::max_lists documentation to include a # Errors section describing failures from malformed sparse structural metadata, and add a synchronized usage example that links to the sparse unraveller contract and relevant method or struct. Keep the existing validity caveat and implementation behavior unchanged.Source: Coding guidelines
983-992: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the guarded
unwrap()from the serializer planner.Line 992 relies on a separate predicate for safety. Bind
max_visible_levelwithlet Some(...) = ... else { ... }and return the existingWithinBudgetfast path instead.Proposed fix
- let max_visible_level = SerializedRepDefs::max_visible_level(def_meaning); - let should_plan = !self.has_fsl && max_schema_rep > 0 && max_visible_level.is_some(); - - if !should_plan { + if self.has_fsl || max_schema_rep == 0 { self.normalize_specials(); return Ok(MiniBlockRepDefBudget::WithinBudget); } - - let max_visible_level = max_visible_level.unwrap(); + let Some(max_visible_level) = SerializedRepDefs::max_visible_level(def_meaning) else { + self.normalize_specials(); + return Ok(MiniBlockRepDefBudget::WithinBudget); + };As per coding guidelines, never use bare
.unwrap()in library code for fallible operations.🤖 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-encoding/src/repdef.rs` around lines 983 - 992, Replace the separate max_visible_level predicate and later unwrap in the serializer planning flow with a let-else binding around SerializedRepDefs::max_visible_level. In the else branch, call self.normalize_specials() and return MiniBlockRepDefBudget::WithinBudget; retain the existing planning conditions and subsequent logic for Some values.Source: Coding guidelines
rust/lance-file/src/reader.rs (2)
946-956: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClassify malformed on-disk metadata as corrupt-file errors.
These checks reject persisted file-format corruption, not invalid caller parameters. Preserve the detailed context/source but use
Error::corrupt_fileso callers can distinguish damaged files from API-input errors.
rust/lance-file/src/reader.rs#L946-L956: classify invalid global-buffer alignment as corrupt file metadata.rust/lance-file/src/reader.rs#L1162-L1312: classify invalid encoding envelopes, missing metadata, mismatched vectors, and unaligned page offsets as corrupt file metadata.As per coding guidelines, “use
Error::corrupt_filefor format or integrity issues.”🤖 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/reader.rs` around lines 946 - 956, Update the metadata validation errors in reader.rs to use Error::corrupt_file instead of Error::invalid_input_source. Apply this to the global-buffer alignment check near lines 946-956 and all invalid encoding envelopes, missing metadata, mismatched vectors, and unaligned page-offset checks within lines 1162-1312, preserving each error’s existing context and source details.Source: Coding guidelines
2751-2879: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winForce sparse pages to cross a page boundary.
This test uses one six-row batch with default page limits and never asserts multiple pages. Force multiple V2.3 sparse pages and verify a range/take crossing a boundary, otherwise the new per-page
num_slotsaggregation remains untested.As per coding guidelines, “Every bugfix and feature must have corresponding tests.”
🤖 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/reader.rs` around lines 2751 - 2879, Update sparse_file_writer_reader_scan_range_and_take_roundtrip to configure sufficiently small page limits when writing the V2_3 file, ensuring the six-row batch produces multiple sparse pages and asserting that multiple pages were created. Keep the existing full scan, range, and take checks, with the range or take selection crossing a page boundary so per-page num_slots aggregation is exercised.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.
Outside diff comments:
In `@rust/lance-encoding/src/repdef.rs`:
- Around line 1681-1688: Update the public RepDef::max_lists documentation to
include a # Errors section describing failures from malformed sparse structural
metadata, and add a synchronized usage example that links to the sparse
unraveller contract and relevant method or struct. Keep the existing validity
caveat and implementation behavior unchanged.
- Around line 983-992: Replace the separate max_visible_level predicate and
later unwrap in the serializer planning flow with a let-else binding around
SerializedRepDefs::max_visible_level. In the else branch, call
self.normalize_specials() and return MiniBlockRepDefBudget::WithinBudget; retain
the existing planning conditions and subsequent logic for Some values.
In `@rust/lance-file/src/reader.rs`:
- Around line 946-956: Update the metadata validation errors in reader.rs to use
Error::corrupt_file instead of Error::invalid_input_source. Apply this to the
global-buffer alignment check near lines 946-956 and all invalid encoding
envelopes, missing metadata, mismatched vectors, and unaligned page-offset
checks within lines 1162-1312, preserving each error’s existing context and
source details.
- Around line 2751-2879: Update
sparse_file_writer_reader_scan_range_and_take_roundtrip to configure
sufficiently small page limits when writing the V2_3 file, ensuring the six-row
batch produces multiple sparse pages and asserting that multiple pages were
created. Keep the existing full scan, range, and take checks, with the range or
take selection crossing a page boundary so per-page num_slots aggregation is
exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 5e68fb18-23a2-42e7-b005-9d5841b66a57
📒 Files selected for processing (3)
rust/lance-encoding/src/encodings/logical/primitive.rsrust/lance-encoding/src/repdef.rsrust/lance-file/src/reader.rs
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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
2 participants
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.
This PR replays #7754 unchanged against
main.#7754 was accidentally merged into
xuanwo/sparse-stack-2-empty-inline-bitpackedinstead ofmain. This PR only corrects that target mistake and introduces no changes beyond the original PR.All design discussion, review history, approvals, and validation are recorded in #7754.