feat: data overlay files — model, feature flag, and write/commit path#7535
Merged
Conversation
github-actions
Bot
added
A-encoding
Contributor
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
This was referenced Jun 30, 2026
This was referenced Jun 30, 2026
Contributor
Author
Benchmark results: manifest size and write costNumbers from the data-overlay benchmark suite (#7544). All on a local filesystem, 1M-row base, int32 #1 — Manifest sizeEach committed overlay adds a value-file pointer plus a serialized coverage bitmap to the fragment's metadata in the manifest (read on every dataset open). Base manifest is 453 B.
#2 — Updating one column for 1% of rows
Takeaways:
Caveats: the overlay write arm is hand-rolled (there's no |
wjones127
force-pushed
the
will/oss-1322-write-data-overlay-and-scan-data
branch
from
July 6, 2026 20:44
e2f98f5 to
efab63a
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
wjones127
force-pushed
the
will/oss-1322-write-data-overlay-and-scan-data
branch
from
July 7, 2026 22:25
efab63a to
6a39624
Compare
wjones127
commented
Jul 8, 2026
wjones127
marked this pull request as ready for review
July 8, 2026 17:07
westonpace
approved these changes
Jul 9, 2026
westonpace
left a comment
westonpace
left a comment
Member
There was a problem hiding this comment.
Some thoughts but nothing blocking
Adds the in-memory + commit machinery for data overlay files (per the spec in #7381), the foundation the scanner/take/index/compaction work builds on. - `DataOverlayFile` / `OverlayCoverage` (dense `shared_offset_bitmap` and sparse per-field) with protobuf round-trip, attached to `Fragment.overlays`. - Reader feature flag 64 (`FLAG_DATA_OVERLAY_FILES`): set whenever any fragment carries overlays, so a reader that does not understand them refuses the dataset instead of returning stale base values. - `Operation::DataOverlay` transaction op: appends overlays to a fragment's list (preserving concurrently-written overlays) and stamps each overlay's `committed_version` to the new dataset version at commit time (re-stamped on retry). Conflict rules mirror DataReplacement — permissive against appends, deletes, column rewrites, index builds, and other overlays; conflicts only with row-rewriting compaction of the same fragment. Scan-side merge, take, and end-to-end write+read tests follow in the same PR branch. Part of the Data Overlay Files feature (OSS-1322). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Now that overlays can be committed, a scan or take over a fragment that has overlays would silently return stale base values, since the read-path merge is not implemented yet. Refuse such reads at `FileFragment::open` with a clear error instead of serving incorrect data. Lifted once the scan/take merge lands (rest of OSS-1322 / OSS-1324). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…flag OverlayCoverage now holds parsed Arc<RoaringBitmap>s, deserialized once when a fragment is loaded and shared cheaply on clone, instead of re-deserializing the serialized bytes on every coverage_for_field call. Treat the data overlay feature flag (64) as unknown in release builds unless LANCE_ENABLE_DATA_OVERLAY_FILES is set, so the unreleased feature is neither emitted by release writers nor accepted by release readers; debug builds understand it so tests exercise the path. Stable-sort a fragment's overlays by committed_version (newest last) on load so resolution can assume the ordering. Adds tests for the missing-coverage/data_file errors, the release gating, and the load-time sort. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Three correctness fixes in the data-overlay write/commit path, each with tests (this path had ~0% coverage): - build_manifest dropped overlays when two DataOverlayGroups targeted the same fragment (HashMap collect kept only the last). Merge groups by fragment_id so all overlays are appended in order. - check_data_overlay_txn did not conflict when a concurrent Update/Delete removed an overlaid fragment, leaving the overlay orphaned and hard-erroring on retry. Mirror check_data_replacement_txn: retryable conflict when removed/deleted_fragment_ids intersect our fragments; fragments merely updated in place stay compatible. - impl PartialEq for Operation had the DataOverlay arm backwards: it reported DataOverlay == Rewrite as true and DataOverlay == DataOverlay as false (conflict semantics copied into the equality impl). Compare groups for the same variant; not equal to other variants. Tests: build_manifest append/stamp, duplicate-group merge, unknown-fragment error; the conflict matrix incl. remove-fragment conflict; Operation equality; OverlayCoverage serde JSON round-trip; coverage_for_field out-of-bounds; apply_feature_flags overlay arm. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The overlay data model (`OverlayCoverage`, `DataOverlayFile`, their serde/ protobuf/`DeepSizeOf` impls, the Roaring (de)serialization helpers, and `sort_overlays_newest_last`) lived interleaved with fragment code in `fragment.rs`. Move it to its own `format/overlay.rs` submodule so the small external interface (two types, `dense`/`sparse`, `coverage_for_field`) is visible and the serde/proto/rank machinery is hidden as module-private implementation. A module doc consolidates the coverage, rank, parse-once, and newest-last ordering invariants that were previously scattered across item docs. Pure code move: `format::DataOverlayFile`/`OverlayCoverage` paths are preserved via re-export, so callers are unchanged. Pure-overlay unit tests move with the module; the fragment-carrier round-trip/sort tests stay in `fragment.rs`. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The overlay-direction handler `check_data_overlay_txn` treated a concurrent `UpdateMemWalState` as compatible, but the data overlay spec lists it as a hard conflict and the reverse-direction handler `check_update_mem_wal_state_txn` already treats a concurrent `DataOverlay` as incompatible. The pair therefore conflicted or not depending on commit order. Move `UpdateMemWalState` into the incompatible arm so both directions agree and match the spec. Extend `test_data_overlay_conflicts` to cover `UpdateMemWalState` and `Overwrite`, the two hard-conflict cases the matrix previously missed. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…c drift
Self-review polish before requesting review.
Tests:
- Add test_rewrite_conflicts_with_data_overlay: the reverse direction of the
overlay conflict matrix (our Rewrite vs. a committed DataOverlay), which had
real logic in check_rewrite_txn but no test.
- Add test_data_overlay_build_manifest_multi_fragment: overlays on two distinct
fragments plus an untargeted fragment passed through unchanged (the pass-through
branch was previously uncovered).
- Make the overlay-flag assertions in feature_flags read/write checks
profile-independent (assert readable iff data_overlay_files_enabled), so they
no longer fail under a release build without LANCE_ENABLE_DATA_OVERLAY_FILES.
Docs/comments:
- check_data_overlay_txn doc now reflects the actual rules (fragment-removing
Delete/Update and UpdateMemWalState conflict; in-place ops are tolerated).
- Operation::DataOverlay doc says "(physical offset, field)", matching overlay.rs.
- coverage_for_field error message uses Rust vocabulary ("per-field coverage")
instead of the protobuf field name.
- Remove a redundant in-function import in test_data_overlay_operation_roundtrips.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…ield tombstoning - Make `mod overlay` public and drop the format re-export (import via `format::overlay::…`). - Rename the feature flag/env var to mark the feature unstable (FLAG_UNSTABLE_DATA_OVERLAY_FILES, LANCE_ENABLE_UNSTABLE_DATA_OVERLAY_FILES). - Distinguish update modes in conflict resolution: a row-moving Update (RewriteRows) relocates rows out of an overlaid fragment, so it retryably conflicts with a DataOverlay in both directions; an in-place RewriteColumns update preserves physical offsets and stays compatible, and Delete stays compatible (deletion vectors preserve offsets). - When a DataReplacement or RewriteColumns update writes new base values for a field, tombstone that field in any overlay on the fragment (field id -> -2) so the fresh base is not shadowed; drop overlays left with no live fields. - Consolidate the single-fragment build_manifest test into the multi-fragment one; add reverse-direction conflict and tombstone tests. - Document the tombstone/mode rules in the transaction spec. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…uard Row-level resolution for concurrent Update vs DataOverlay, replacing the fragment-granular conflict. When an Update rebases onto a committed overlay, intersect the update's affected rows with the overlay coverage in memory. When an overlay rebases onto a committed row-moving Update, defer to finish_data_overlay, which diffs the update's deletion vectors against the read-time pre-image and conflicts only when moved rows intersect coverage. A concurrent Delete on the same fragment may over-conflict in the rare both-present case (retry, never data loss). Also from review: - DeepSizeOf marks shared overlay Arc bitmaps to avoid double-counting. - feature_flags gains a mark_supported helper for chaining unstable flags. - build_manifest verifies overlays are newest-last at the write boundary. - overlay existence check in build_manifest uses a set (was O(N^2)). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
wjones127
force-pushed
the
will/oss-1322-write-data-overlay-and-scan-data
branch
from
July 10, 2026 22:38
dec272a to
b82ac8d
Compare
Contributor
📝 WalkthroughWalkthroughData overlay files are represented in fragments, serialized through protobuf, applied by a new transaction operation, checked during conflict resolution, and gated by build-supported feature flags. Rewrite and replacement operations tombstone affected overlay fields. ChangesData overlay support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant TransactionRebase
participant DeletionVectors
participant Manifest
Client->>TransactionRebase: rebase DataOverlay transaction
TransactionRebase->>DeletionVectors: read current deletion bitmap
DeletionVectors-->>TransactionRebase: return moved-row bitmap
TransactionRebase->>TransactionRebase: compare moved rows with overlay coverage
TransactionRebase->>Manifest: accept or retry commit
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@docs/src/format/table/transaction.md`:
- Around line 508-513: Update the Merge documentation to state that row-moving
Update conflicts are determined at the row level, not merely by sharing a
fragment: conflict occurs only when the update’s relocated rows intersect the
overlay’s covered offsets, as determined by diffing deletion vectors at
commit-finish time; disjoint relocated rows on the same fragment must succeed.
Describe this retry condition and ordering explicitly in the relevant bullet.
In `@rust/lance-table/src/format/overlay.rs`:
- Around line 342-359: Refactor test_overlay_coverage_serde_json_roundtrip to
use rstest parameterized cases instead of looping over an array. Add named
#[case::...] entries for each dense and sparse coverage input, pass each case
into the test function, and retain the existing JSON round-trip assertions for
clearer failure identification.
- Around line 149-259: Add Rustdoc `# Examples` code blocks to the public APIs
`OverlayCoverage::dense`, `OverlayCoverage::sparse`,
`DataOverlayFile::coverage_for_field`, `sort_overlays_newest_last`,
`verify_overlays_newest_last`, and `tombstone_overlay_fields`. Use compile-valid
examples that demonstrate each API’s basic usage and reference relevant types or
methods through rustdoc links where appropriate.
- Around line 88-93: Replace the bare unwrap in serialize_roaring with expect
and a concise message explaining that writing the bitmap into a Vec is
infallible.
- Around line 80-86: Change deserialize_roaring to return Error::corrupt_file
instead of Error::invalid_input when RoaringBitmap::deserialize_from fails,
preserving the existing contextual error message. This reflects that the
function loads persisted overlay coverage through OverlayCoverageBytes and
pb::DataOverlayFile, so failures indicate corrupt or mismatched on-disk data.
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 2348-2394: Extract the DataOverlay merging, fragment validation,
and version-stamping logic from the build_manifest match arm into a private
helper such as apply_data_overlay(groups, existing_fragments, new_version) ->
Result<Vec<Fragment>>. Move the overlays_by_fragment construction, existence
checks, and fragment cloning/appending into that helper, then invoke it from
build_manifest and extend final_fragments with the returned fragments. Preserve
ordering, retry version stamping, and invalid-input behavior.
In `@rust/lance/src/io/commit/conflict_resolver.rs`:
- Around line 1777-1841: Parallelize the per-fragment deletion-vector reads in
finish_data_overlay using the
futures::stream::iter(...).buffered(io_parallelism) pattern already used by
finish_delete_update. Preserve each fragment’s conflict validation and error
behavior, collecting or processing the buffered results so current and initial
deletion vectors are read concurrently while respecting the configured I/O
parallelism.
🪄 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: 29cdadb4-ea2c-4a98-a535-5ddce0952d86
📒 Files selected for processing (16)
docs/src/format/table/transaction.mdrust/lance-table/src/feature_flags.rsrust/lance-table/src/format.rsrust/lance-table/src/format/fragment.rsrust/lance-table/src/format/manifest.rsrust/lance-table/src/format/overlay.rsrust/lance/src/dataset/files.rsrust/lance/src/dataset/fragment.rsrust/lance/src/dataset/optimize.rsrust/lance/src/dataset/schema_evolution.rsrust/lance/src/dataset/transaction.rsrust/lance/src/dataset/write.rsrust/lance/src/dataset/write/commit.rsrust/lance/src/io/commit.rsrust/lance/src/io/commit/conflict_resolver.rsrust/lance/src/utils/test.rs
Comment on lines
+508
to
+513
| - Merge (always). | ||
| - A row-moving Update that touches an overlaid fragment — a delete-and-reinsert | ||
| update (any update that is not a `REWRITE_COLUMNS` column rewrite) relocates the | ||
| updated rows into new fragments, so the overlay's physical offsets no longer | ||
| address them; the writer must re-read and retry. | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify that the row-moving Update conflict is row-level, not fragment-level.
As written, this reads as "any row-moving Update touching an overlaid fragment conflicts." In practice the conflict resolver only retries when the specific rows the update relocated intersect the overlay's covered offsets (checked by diffing deletion vectors at commit-finish time); a row-moving update on the same fragment whose moved rows are disjoint from the overlay's coverage succeeds without conflict.
As per coding guidelines: "Describe algorithms in full detail, including parameters, precision, ordering, normalization bounds, and implementation steps; do not reference an algorithm by name alone."
✏️ Suggested wording
-- A row-moving Update that touches an overlaid fragment — a delete-and-reinsert
- update (any update that is not a `REWRITE_COLUMNS` column rewrite) relocates the
- updated rows into new fragments, so the overlay's physical offsets no longer
- address them; the writer must re-read and retry.
+- A row-moving Update (any update that is not a `REWRITE_COLUMNS` column rewrite)
+ whose relocated rows intersect the overlay's covered offsets on the same
+ fragment — the moved rows are re-created from the pre-overlay base, so the
+ overlay's values for those specific offsets are lost; conflict detection
+ compares the update's moved-row set against the overlay's coverage bitmap
+ (via the deletion-vector delta at commit time), so a row-moving update whose
+ moved rows don't overlap the overlay's coverage does not conflict.📝 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.
Suggested change
| - Merge (always). | |
| - A row-moving Update that touches an overlaid fragment — a delete-and-reinsert | |
| update (any update that is not a `REWRITE_COLUMNS` column rewrite) relocates the | |
| updated rows into new fragments, so the overlay's physical offsets no longer | |
| address them; the writer must re-read and retry. | |
| - A row-moving Update (any update that is not a `REWRITE_COLUMNS` column rewrite) | |
| whose relocated rows intersect the overlay's covered offsets on the same | |
| fragment — the moved rows are re-created from the pre-overlay base, so the | |
| overlay's values for those specific offsets are lost; conflict detection | |
| compares the update's moved-row set against the overlay's coverage bitmap | |
| (via the deletion-vector delta at commit time), so a row-moving update whose | |
| moved rows don't overlap the overlay's coverage does not conflict. |
🤖 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 `@docs/src/format/table/transaction.md` around lines 508 - 513, Update the
Merge documentation to state that row-moving Update conflicts are determined at
the row level, not merely by sharing a fragment: conflict occurs only when the
update’s relocated rows intersect the overlay’s covered offsets, as determined
by diffing deletion vectors at commit-finish time; disjoint relocated rows on
the same fragment must succeed. Describe this retry condition and ordering
explicitly in the relevant bullet.
Source: Coding guidelines
Comment on lines
+88
to
+93
| fn serialize_roaring(bitmap: &RoaringBitmap) -> Vec<u8> { | ||
| let mut bytes = Vec::with_capacity(bitmap.serialized_size()); | ||
| // Writing to a Vec is infallible. | ||
| bitmap.serialize_into(&mut bytes).unwrap(); | ||
| bytes | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use .expect("reason") instead of bare .unwrap() in production code.
The comment already documents why this is infallible; per guideline this should be an .expect("...") rather than .unwrap().
As per coding guidelines, "Avoid bare .unwrap()... If unavoidable, use .expect("reason")."
🛡️ Proposed fix
let mut bytes = Vec::with_capacity(bitmap.serialized_size());
// Writing to a Vec is infallible.
- bitmap.serialize_into(&mut bytes).unwrap();
+ bitmap
+ .serialize_into(&mut bytes)
+ .expect("writing to a Vec is infallible");
bytes📝 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.
Suggested change
| fn serialize_roaring(bitmap: &RoaringBitmap) -> Vec<u8> { | |
| let mut bytes = Vec::with_capacity(bitmap.serialized_size()); | |
| // Writing to a Vec is infallible. | |
| bitmap.serialize_into(&mut bytes).unwrap(); | |
| bytes | |
| } | |
| fn serialize_roaring(bitmap: &RoaringBitmap) -> Vec<u8> { | |
| let mut bytes = Vec::with_capacity(bitmap.serialized_size()); | |
| // Writing to a Vec is infallible. | |
| bitmap | |
| .serialize_into(&mut bytes) | |
| .expect("writing to a Vec is infallible"); | |
| bytes | |
| } |
🤖 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-table/src/format/overlay.rs` around lines 88 - 93, Replace the
bare unwrap in serialize_roaring with expect and a concise message explaining
that writing the bitmap into a Vec is infallible.
Source: Coding guidelines
Comment on lines
+149
to
+259
| impl OverlayCoverage { | ||
| /// Build a dense coverage from a single bitmap shared across every field. | ||
| pub fn dense(bitmap: RoaringBitmap) -> Self { | ||
| Self::Shared(Arc::new(bitmap)) | ||
| } | ||
|
|
||
| /// Build a sparse coverage from one bitmap per field. | ||
| pub fn sparse(bitmaps: Vec<RoaringBitmap>) -> Self { | ||
| Self::PerField(bitmaps.into_iter().map(Arc::new).collect()) | ||
| } | ||
| } | ||
|
|
||
| /// An overlay file supplies new values for a subset of `(physical offset, field)` | ||
| /// cells within a fragment, without rewriting the fragment's base data files. See | ||
| /// the [module documentation](self) for the coverage, rank, and versioning rules. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, DeepSizeOf)] | ||
| pub struct DataOverlayFile { | ||
| /// The data file storing the overlay's new cell values. | ||
| pub data_file: DataFile, | ||
| /// Which cells this overlay provides values for. | ||
| pub coverage: OverlayCoverage, | ||
| /// The dataset version at which this overlay became effective (the version of | ||
| /// the commit that introduced it, stamped at commit time and re-stamped on | ||
| /// retry). Higher wins when two overlays cover the same `(offset, field)`. | ||
| pub committed_version: u64, | ||
| } | ||
|
|
||
| impl DataOverlayFile { | ||
| /// The parsed coverage bitmap that applies to the field stored at | ||
| /// `field_pos` within `data_file.fields`. | ||
| /// | ||
| /// For a dense overlay the same shared bitmap is returned for every field; | ||
| /// for a sparse overlay the per-field bitmap at `field_pos` is returned. The | ||
| /// bitmap is already parsed, so this is a cheap `Arc` clone. | ||
| pub fn coverage_for_field(&self, field_pos: usize) -> Result<Arc<RoaringBitmap>> { | ||
| match &self.coverage { | ||
| OverlayCoverage::Shared(bitmap) => Ok(bitmap.clone()), | ||
| OverlayCoverage::PerField(bitmaps) => { | ||
| bitmaps.get(field_pos).cloned().ok_or_else(|| { | ||
| Error::invalid_input(format!( | ||
| "overlay per-field coverage has {} bitmaps but field position {} was requested", | ||
| bitmaps.len(), | ||
| field_pos | ||
| )) | ||
| }) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Stable-sort a fragment's overlays newest-last by `committed_version`. The | ||
| /// stable sort preserves list position as the tiebreak for equal versions, so | ||
| /// resolution can rely on the ordering without re-checking. See the [module | ||
| /// documentation](self) for the ordering invariant. | ||
| pub fn sort_overlays_newest_last(overlays: &mut [DataOverlayFile]) { | ||
| overlays.sort_by_key(|overlay| overlay.committed_version); | ||
| } | ||
|
|
||
| /// Verify a fragment's overlays are stored newest-last (non-decreasing | ||
| /// `committed_version`), the ordering invariant readers rely on for | ||
| /// resolution. Returns an error identifying the first out-of-order pair. | ||
| /// | ||
| /// [`sort_overlays_newest_last`] normalizes on load; this is the write-side | ||
| /// guard that rejects any commit path that assembled overlays out of order. See | ||
| /// the [module documentation](self) for the ordering invariant. | ||
| pub fn verify_overlays_newest_last(overlays: &[DataOverlayFile]) -> Result<()> { | ||
| for pair in overlays.windows(2) { | ||
| if pair[0].committed_version > pair[1].committed_version { | ||
| return Err(Error::invalid_input(format!( | ||
| "overlay files must be stored newest-last, but committed_version {} precedes {}", | ||
| pair[0].committed_version, pair[1].committed_version | ||
| ))); | ||
| } | ||
| } | ||
| Ok(()) | ||
| } | ||
|
|
||
| /// Tombstone `fields` across a fragment's `overlays`, dropping any overlay left | ||
| /// with no live fields. | ||
| /// | ||
| /// Called when new base values are written for those fields (a DataReplacement, | ||
| /// or an in-place column rewrite): the stale overlay values must stop shadowing | ||
| /// the fresh base. Each matching field id is replaced with [`TOMBSTONE_FIELD_ID`] | ||
| /// in place, preserving the overlay's remaining fields and its coverage positions | ||
| /// (a per-field coverage bitmap stays aligned with `data_file.fields`). An overlay | ||
| /// whose fields are now all tombstoned is removed entirely. See the [module | ||
| /// documentation](self) for the tombstone invariant. | ||
| pub fn tombstone_overlay_fields(overlays: &mut Vec<DataOverlayFile>, fields: &[u32]) { | ||
| for overlay in overlays.iter_mut() { | ||
| let tombstoned: Vec<i32> = overlay | ||
| .data_file | ||
| .fields | ||
| .iter() | ||
| .map(|&field| { | ||
| if field >= 0 && fields.contains(&(field as u32)) { | ||
| TOMBSTONE_FIELD_ID | ||
| } else { | ||
| field | ||
| } | ||
| }) | ||
| .collect(); | ||
| overlay.data_file.fields = tombstoned.into(); | ||
| } | ||
| overlays.retain(|overlay| { | ||
| overlay | ||
| .data_file | ||
| .fields | ||
| .iter() | ||
| .any(|&field| field != TOMBSTONE_FIELD_ID) | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Public APIs are missing doc examples.
OverlayCoverage::dense/sparse, DataOverlayFile::coverage_for_field, sort_overlays_newest_last, verify_overlays_newest_last, and tombstone_overlay_fields all have descriptive doc comments with cross-links, but none include an # Examples code block.
As per coding guidelines, "All public APIs must have documentation with examples, and documentation should link to relevant structs and methods."
🤖 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-table/src/format/overlay.rs` around lines 149 - 259, Add Rustdoc
`# Examples` code blocks to the public APIs `OverlayCoverage::dense`,
`OverlayCoverage::sparse`, `DataOverlayFile::coverage_for_field`,
`sort_overlays_newest_last`, `verify_overlays_newest_last`, and
`tombstone_overlay_fields`. Use compile-valid examples that demonstrate each
API’s basic usage and reference relevant types or methods through rustdoc links
where appropriate.
Source: Coding guidelines
Comment on lines
+342
to
+359
| #[test] | ||
| fn test_overlay_coverage_serde_json_roundtrip() { | ||
| // The custom serde impl round-trips through JSON for dense/sparse, | ||
| // including empty bitmaps and a zero-bitmap sparse coverage. | ||
| for coverage in [ | ||
| OverlayCoverage::dense(RoaringBitmap::from_iter([1u32, 5, 100])), | ||
| OverlayCoverage::dense(RoaringBitmap::new()), | ||
| OverlayCoverage::sparse(vec![ | ||
| RoaringBitmap::from_iter([2u32, 3]), | ||
| RoaringBitmap::new(), | ||
| ]), | ||
| OverlayCoverage::sparse(vec![]), | ||
| ] { | ||
| let json = serde_json::to_string(&coverage).unwrap(); | ||
| let back: OverlayCoverage = serde_json::from_str(&json).unwrap(); | ||
| assert_eq!(back, coverage); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider rstest with named cases for the coverage round-trip test.
This loops over 4 distinct coverage inputs; per guideline this pattern is better expressed as #[rstest] #[case::dense(...)]/#[case::sparse(...)] for readable failure output identifying which case failed.
As per coding guidelines, "Use rstest for Rust tests... when cases differ only by inputs; use #[case::{name}(...)] for readable Rust case names."
🤖 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-table/src/format/overlay.rs` around lines 342 - 359, Refactor
test_overlay_coverage_serde_json_roundtrip to use rstest parameterized cases
instead of looping over an array. Add named #[case::...] entries for each dense
and sparse coverage input, pass each case into the test function, and retain the
existing JSON round-trip assertions for clearer failure identification.
Source: Coding guidelines
Comment on lines
+2348
to
+2394
| Operation::DataOverlay { groups } => { | ||
| // Stamp each overlay with the version this commit is producing. | ||
| // build_manifest re-runs on every retry with an updated | ||
| // current_manifest, so this is naturally re-stamped on retry. | ||
| let new_version = current_manifest.map_or(1, |m| m.version + 1); | ||
|
|
||
| let existing_fragments = maybe_existing_fragments?; | ||
| // Multiple groups may target the same fragment; merge them in | ||
| // order rather than letting a HashMap collapse drop all but the | ||
| // last group's overlays. | ||
| let mut overlays_by_fragment: HashMap<u64, Vec<&DataOverlayFile>> = HashMap::new(); | ||
| for group in groups { | ||
| overlays_by_fragment | ||
| .entry(group.fragment_id) | ||
| .or_default() | ||
| .extend(group.overlays.iter()); | ||
| } | ||
|
|
||
| // Every group must target an existing fragment. Build a set of | ||
| // existing ids once so this is O(groups + fragments) rather than | ||
| // O(groups * fragments). | ||
| let existing_fragment_ids: HashSet<u64> = | ||
| existing_fragments.iter().map(|f| f.id).collect(); | ||
| for fragment_id in overlays_by_fragment.keys() { | ||
| if !existing_fragment_ids.contains(fragment_id) { | ||
| return Err(Error::invalid_input(format!( | ||
| "DataOverlay targets fragment {fragment_id}, which does not exist" | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| for fragment in existing_fragments { | ||
| let mut fragment = fragment.clone(); | ||
| if let Some(new_overlays) = overlays_by_fragment.get(&fragment.id) { | ||
| // Appended (not replaced) so concurrently-written overlays | ||
| // survive; later entries are newer. | ||
| fragment | ||
| .overlays | ||
| .extend(new_overlays.iter().map(|&overlay| { | ||
| let mut overlay = overlay.clone(); | ||
| overlay.committed_version = new_version; | ||
| overlay | ||
| })); | ||
| } | ||
| final_fragments.push(fragment); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider extracting the DataOverlay merge/stamp logic into a helper method.
The group-merging, fragment-existence validation, and per-fragment overlay stamping (~45 lines) are inlined directly into the already very large build_manifest match statement. Extracting this into a small private method (e.g. apply_data_overlay(groups, existing_fragments, new_version) -> Result<Vec<Fragment>>) would make it independently testable and keep build_manifest more scannable.
As per coding guidelines: "Extract substantial new logic such as bin packing or scheduling into dedicated submodules instead of inlining it into large files."
🤖 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 2348 - 2394, Extract the
DataOverlay merging, fragment validation, and version-stamping logic from the
build_manifest match arm into a private helper such as
apply_data_overlay(groups, existing_fragments, new_version) ->
Result<Vec<Fragment>>. Move the overlays_by_fragment construction, existence
checks, and fragment cloning/appending into that helper, then invoke it from
build_manifest and extend final_fragments with the returned fragments. Preserve
ordering, retry version stamping, and invalid-input behavior.
Source: Coding guidelines
Comment on lines
+1777
to
+1841
| async fn finish_data_overlay(self, dataset: &Dataset) -> Result<Transaction> { | ||
| let fragments_to_check: HashSet<u64> = self | ||
| .initial_fragments | ||
| .iter() | ||
| .filter_map(|(id, (_, needs_check))| needs_check.then_some(*id)) | ||
| .collect(); | ||
| if fragments_to_check.is_empty() { | ||
| return Ok(Transaction { | ||
| read_version: dataset.manifest.version, | ||
| ..self.transaction | ||
| }); | ||
| } | ||
|
|
||
| // Coverage (physical offsets, unioned across fields) per flagged fragment. | ||
| let Operation::DataOverlay { groups } = &self.transaction.operation else { | ||
| return Err(wrong_operation_err(&self.transaction.operation)); | ||
| }; | ||
| let mut coverage_by_fragment: HashMap<u64, RoaringBitmap> = HashMap::new(); | ||
| for group in groups { | ||
| if !fragments_to_check.contains(&group.fragment_id) { | ||
| continue; | ||
| } | ||
| *coverage_by_fragment.entry(group.fragment_id).or_default() |= | ||
| overlay_group_coverage(group); | ||
| } | ||
|
|
||
| for (fragment_id, coverage) in coverage_by_fragment { | ||
| let Some(current_fragment) = dataset | ||
| .fragments() | ||
| .as_slice() | ||
| .iter() | ||
| .find(|f| f.id == fragment_id) | ||
| else { | ||
| // The fragment is gone entirely; the overlay is orphaned. | ||
| return Err(crate::Error::retryable_commit_conflict_source( | ||
| dataset.manifest.version, | ||
| format!( | ||
| "This {} transaction was preempted: overlaid fragment {} was removed by a concurrent transaction. Please retry.", | ||
| self.transaction.uuid, fragment_id | ||
| ) | ||
| .into(), | ||
| )); | ||
| }; | ||
| let current_deletions = | ||
| read_fragment_deletion_bitmap(dataset, current_fragment).await?; | ||
| let initial_deletions = match self.initial_fragments.get(&fragment_id) { | ||
| Some((initial_fragment, _)) => { | ||
| read_fragment_deletion_bitmap(dataset, initial_fragment).await? | ||
| } | ||
| None => RoaringBitmap::new(), | ||
| }; | ||
| let moved_rows = ¤t_deletions - &initial_deletions; | ||
| let conflicting = &moved_rows & &coverage; | ||
| if !conflicting.is_empty() { | ||
| let sample: Vec<u32> = conflicting.iter().take(5).collect(); | ||
| return Err(crate::Error::retryable_commit_conflict_source( | ||
| dataset.manifest.version, | ||
| format!( | ||
| "This {} transaction was preempted by a concurrent update that moved overlaid rows on fragment {} (offsets {:?}). Please retry.", | ||
| self.transaction.uuid, fragment_id, sample.as_slice() | ||
| ) | ||
| .into(), | ||
| )); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Consider parallelizing the per-fragment deletion-vector reads.
Each flagged fragment reads its current and initial deletion vectors sequentially in a for loop with .await inside. finish_delete_update (same file) already uses a futures::stream::iter(...).buffered(io_parallelism) pattern for equivalent I/O; applying the same pattern here would reduce latency when several overlaid fragments need row-level verification during a retry.
🤖 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/io/commit/conflict_resolver.rs` around lines 1777 - 1841,
Parallelize the per-fragment deletion-vector reads in finish_data_overlay using
the futures::stream::iter(...).buffered(io_parallelism) pattern already used by
finish_delete_update. Preserve each fragment’s conflict validation and error
behavior, collecting or processing the buffered results so current and initial
deletion vectors are read concurrently while respecting the configured I/O
parallelism.
The Python (`python/src/fragment.rs`) and Java JNI (`java/lance-jni/src/fragment.rs`) `FromPyObject`/`FromJObject` conversions construct a `lance_table::format::Fragment` and were missing the new `overlays` field, breaking the maturin build and the JNI clippy check. Overlays are not exposed to Python or Java yet and the reverse conversions do not export them, so these round-trips default to an empty overlay list. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
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 `@python/src/fragment.rs`:
- Around line 828-830: The PyLance<Fragment> conversion currently discards
overlays by assigning an empty vector, causing committed merge/update results to
lose overlay files. Update the conversion implementation and its reverse
conversion to preserve overlays opaquely, or explicitly reject fragments with
non-empty overlays instead of silently dropping them; use the relevant PyLance
and Lance Fragment conversion methods around the overlays field.
🪄 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: beef9d81-7947-4a80-ae80-d37ccbfa9cab
📒 Files selected for processing (2)
java/lance-jni/src/fragment.rspython/src/fragment.rs
Comment on lines
+828
to
+830
| // Overlays are not exposed to Python yet, and the reverse conversion | ||
| // does not export them, so this round-trip is overlay-free. | ||
| overlays: vec![], |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
rg -n -C5 \
'PyLance<Fragment>|FromPyObject.*Fragment|extract.*Fragment|Fragment.*(write|commit|append|update)' \
python rust/lanceRepository: lance-format/lance
Length of output: 27436
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '800,910p' python/src/fragment.rs
printf '\n---\n'
rg -n -C3 'struct Fragment|overlays' rust/lance/src python/src | head -n 80Repository: lance-format/lance
Length of output: 9915
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the Rust Fragment definition and Python conversion code paths.
rg -n -C4 'pub struct Fragment|overlays:' rust/lance/src python/src
printf '\n=== fragment.rs around conversions ===\n'
sed -n '800,910p' python/src/fragment.rsRepository: lance-format/lance
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -eu
# Find where Fragment metadata is converted back to Python and whether overlays are included.
sed -n '876,930p' python/src/fragment.rs
printf '\n---\n'
sed -n '1,140p' rust/lance/src/dataset/fragment.rs
printf '\n---\n'
rg -n -C3 'overlay|overlays|DataOverlay|OverlayCoverage' rust/lance/src/lance_table rust/lance/src | head -n 120Repository: lance-format/lance
Length of output: 17299
🏁 Script executed:
#!/bin/bash
set -eu
# Check whether the Python package exposes overlays on Fragment at all.
rg -n -C3 'class Fragment|_Fragment|overlays|Fragment =' python/python/lance/fragment.py python/src/fragment.rs
printf '\n---\n'
sed -n '1,120p' python/python/lance/fragment.py
printf '\n---\n'
sed -n '876,930p' python/src/fragment.rsRepository: lance-format/lance
Length of output: 9905
🌐 Web query:
Lance Python fragment overlays PyLance Fragment overlays not exposed
💡 Result:
The term "fragment overlays" is not a concept or feature within the Microsoft Pylance extension for Visual Studio Code [1][2]. Based on your query, it is highly likely that you are encountering a misunderstanding of terminology related to the LanceDB project. In the context of the Lance data format, "fragment overlays" (often referred to as "Data Overlay Files") are a technical feature used for data versioning, modifications, and storage [3]. Research indicates that the LanceDB project uses a Python interface, frequently referred to in repository code as pylance or associated with the lance Python package, which manages fragments and overlays [4][5]. If you are experiencing issues where certain features or metadata related to these overlays are not "exposed," it is likely that: 1. You are interacting with the LanceDB Python library (sometimes colloquially called "pylance" in internal development contexts) [5], not the Microsoft Pylance VS Code extension [1]. 2. The specific feature you are looking for may be internal, currently gated by feature flags (such as LANCE_ENABLE_DATA_OVERLAY_FILES), or not yet exposed in the public-facing Python API [3]. To resolve this, you should check the official LanceDB documentation or GitHub repository issues regarding Data Overlay Files [3], rather than searching for Microsoft Pylance settings. If you intended to ask about the Microsoft Pylance extension, be aware that "overlays" is not part of its vocabulary, and any issues with missing symbols are likely related to Python path configuration or namespace package resolution [6][7].
Citations:
- 1: https://marketplace.visualstudio.com/items?itemName=ms-python.vscode-pylance
- 2: https://devblogs.microsoft.com/python/announcing-pylance-fast-feature-rich-language-support-for-python-in-visual-studio-code/
- 3: feat: data overlay files — model, feature flag, and write/commit path #7535
- 4: https://github.com/lancedb/lance/blob/119f87b3/python/src/fragment.rs
- 5: c3218b2
- 6: Pkg_resources namespace packages seem to be overshadowed after editable pip install microsoft/pylance-release#1744
- 7: pylance sometimes looks into excluded folders like "~/.pyenv" microsoft/pylance-release#7204
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect the public Python fragment API around FragmentMetadata and LanceFragment.
sed -n '59,140p' python/python/lance/fragment.py
printf '\n---\n'
sed -n '286,470p' python/python/lance/fragment.py
printf '\n---\n'
rg -n -C2 'overlays|FragmentMetadata|_Fragment' python/python/lance/fragment.py python/src/fragment.rsRepository: lance-format/lance
Length of output: 21405
🏁 Script executed:
#!/bin/bash
set -eu
# Inspect Python entry points that consume fragment collections for commit/write paths.
rg -n -C4 '_write_fragments_transaction|_write_fragments|fragments:|FragmentMetadata|LanceFragment' python/python/lance/fragment.py python/python/lance/dataset.py python/python/lance/transaction.py python/src/transaction.rsRepository: lance-format/lance
Length of output: 41740
Preserve overlays in PyLance<Fragment> conversion.
python/src/fragment.rs:828-830 drops overlays, but LanceFragment.merge/update_columns return fragment metadata that can be committed again. That silently loses overlay files; carry them through opaquely or reject non-empty overlays here.
🤖 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 `@python/src/fragment.rs` around lines 828 - 830, The PyLance<Fragment>
conversion currently discards overlays by assigning an empty vector, causing
committed merge/update results to lose overlay files. Update the conversion
implementation and its reverse conversion to preserve overlays opaquely, or
explicitly reject fragments with non-empty overlays instead of silently dropping
them; use the relevant PyLance and Lance Fragment conversion methods around the
overlays field.
The pre-existing overlays were seeded at committed_version 3 in a manifest left at the default version 1, so build_manifest stamped the new overlay at v2 and the fragment ended up ordered [v3, v2], tripping the newest-last guard. Set the manifest to v3 so the new commit stamps v4, matching how a real pre-existing overlay can never post-date the current manifest. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
The coverage bitmap bytes come from a persisted overlay (the protobuf manifest or a serialized fragment), so a decode failure is on-disk corruption rather than caller input. Switch deserialize_roaring from Error::invalid_input to Error::corrupt_file, threading the overlay's data file path through the protobuf path (empty on the serde path, which deserializes coverage in isolation). Addresses a CodeRabbit review comment. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Contributor
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 (3)
rust/lance-table/src/format/overlay.rs (1)
195-205: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
field_posagainstdata_file.fields.The shared branch returns a bitmap for any index, while the sparse branch only checks bitmap-vector length. An out-of-range position can therefore succeed. Validate the position before matching on coverage.
🤖 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-table/src/format/overlay.rs` around lines 195 - 205, Update OverlayCoverage::coverage_for_field to validate field_pos against data_file.fields before matching on self.coverage, returning the existing invalid-input error for out-of-range positions. Preserve the current Shared and PerField bitmap-selection behavior for valid field positions.rust/lance/src/dataset/transaction.rs (2)
6608-6616: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the error variant as well as its message.
This test only checks a substring, so it would pass if
build_manifestreturned the wrong error kind. Match the error againstError::InvalidInputand then assert the message content.As per coding guidelines, tests must assert both the error variant and message.
🤖 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 6608 - 6616, Update the test around build_manifest to pattern-match the returned error as Error::InvalidInput, extracting its message before asserting it contains “does not exist”. Preserve the existing failure output while ensuring both the error variant and message are validated.Source: Coding guidelines
1948-1960: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTombstone overlays when
RewriteRowsreplaces base values.Existing overlays are copied for every update mode, but invalidated only for
RewriteColumns. A row rewrite can therefore leave an overlay covering the rewritten cell, causing readers to return the stale overlay value instead of the new base value. Apply offset-aware invalidation (or remove all affected overlays when the rewrite replaces the whole fragment) and add a regression test.🤖 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 1948 - 1960, Update the overlay handling around updated.overlays and the RewriteColumns/RewriteRows update modes so RewriteRows also invalidates overlays for cells whose base values are replaced, using offset-aware invalidation or removing all affected overlays when the whole fragment is rewritten. Preserve unrelated overlays, and add a regression test verifying readers return the rewritten base value rather than a stale overlay.
🤖 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-table/src/format/overlay.rs`:
- Around line 195-205: Update OverlayCoverage::coverage_for_field to validate
field_pos against data_file.fields before matching on self.coverage, returning
the existing invalid-input error for out-of-range positions. Preserve the
current Shared and PerField bitmap-selection behavior for valid field positions.
In `@rust/lance/src/dataset/transaction.rs`:
- Around line 6608-6616: Update the test around build_manifest to pattern-match
the returned error as Error::InvalidInput, extracting its message before
asserting it contains “does not exist”. Preserve the existing failure output
while ensuring both the error variant and message are validated.
- Around line 1948-1960: Update the overlay handling around updated.overlays and
the RewriteColumns/RewriteRows update modes so RewriteRows also invalidates
overlays for cells whose base values are replaced, using offset-aware
invalidation or removing all affected overlays when the whole fragment is
rewritten. Preserve unrelated overlays, and add a regression test verifying
readers return the rewritten base value rather than a stale overlay.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d6585683-ec1d-4c09-92b0-89126ba77643
📒 Files selected for processing (2)
rust/lance-table/src/format/overlay.rsrust/lance/src/dataset/transaction.rs
wjones127
added a commit
that referenced
this pull request
Jul 15, 2026
Exposes the `DataOverlay` commit operation to Python so overlays can be created and committed from Python (needed to benchmark and use data overlay files without dropping into Rust). Mirrors the existing `DataReplacement` binding. ## What's here - `LanceOperation.DataOverlay` with `DataOverlayFile` and `DataOverlayGroup`. A `DataOverlayFile` carries the value `DataFile` plus **exactly one** of `shared_offsets` (dense coverage shared by every field) or `field_offsets` (sparse, one offset set per field). The commit stamps `committed_version`; passing both/neither coverage is rejected with a clear error. - PyO3 conversions (both directions) in `python/src/transaction.rs`. - Fills in the `overlays` field on the Python `FragmentMetadata -> Fragment` conversion, which OSS-1322 left unset (Python metadata doesn't carry overlays — they're committed via `DataOverlay`). ## Tests `test_data_overlay_*` in `test_dataset.py`: dense round-trip resolves on read; newest overlay wins; sparse per-field coverage resolves fields independently; and the exactly-one-coverage validation rejects both/neither. ## Stacking Stacked on **#7536** (OSS-1324 read path) — base retargets automatically when it lands. Next PR (benchmarks) stacks on this. > Note: the `python/src/fragment.rs` one-liner arguably belongs in the OSS-1322 PR (#7535), which added `Fragment.overlays` without updating the binding; included here so the stack compiles. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added cell-level `DataOverlay` operations to append fragment overlays without rewriting entire fragments. - Supports dense and sparse overlays, with newest overlapping values taking precedence. - Persists overlay metadata on fragments and carries optional committed version stamps. - **Bug Fixes** - Preserves overlay metadata correctly through JSON serialization and Python↔Rust round-trips. - **Tests** - Added end-to-end coverage for dense/sparse overlays, precedence behavior, metadata round-tripping, and offset validation. - **Documentation/Chores** - Updated fragment metadata `repr` expectations and improved default `max_bytes_per_file` for fragment writing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]>
wjones127
added a commit
that referenced
this pull request
Jul 21, 2026
Exposes the `DataOverlay` commit operation to Python so overlays can be created and committed from Python (needed to benchmark and use data overlay files without dropping into Rust). Mirrors the existing `DataReplacement` binding. ## What's here - `LanceOperation.DataOverlay` with `DataOverlayFile` and `DataOverlayGroup`. A `DataOverlayFile` carries the value `DataFile` plus **exactly one** of `shared_offsets` (dense coverage shared by every field) or `field_offsets` (sparse, one offset set per field). The commit stamps `committed_version`; passing both/neither coverage is rejected with a clear error. - PyO3 conversions (both directions) in `python/src/transaction.rs`. - Fills in the `overlays` field on the Python `FragmentMetadata -> Fragment` conversion, which OSS-1322 left unset (Python metadata doesn't carry overlays — they're committed via `DataOverlay`). ## Tests `test_data_overlay_*` in `test_dataset.py`: dense round-trip resolves on read; newest overlay wins; sparse per-field coverage resolves fields independently; and the exactly-one-coverage validation rejects both/neither. ## Stacking Stacked on **#7536** (OSS-1324 read path) — base retargets automatically when it lands. Next PR (benchmarks) stacks on this. > Note: the `python/src/fragment.rs` one-liner arguably belongs in the OSS-1322 PR (#7535), which added `Fragment.overlays` without updating the binding; included here so the stack compiles. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added cell-level `DataOverlay` operations to append fragment overlays without rewriting entire fragments. - Supports dense and sparse overlays, with newest overlapping values taking precedence. - Persists overlay metadata on fragments and carries optional committed version stamps. - **Bug Fixes** - Preserves overlay metadata correctly through JSON serialization and Python↔Rust round-trips. - **Tests** - Added end-to-end coverage for dense/sparse overlays, precedence behavior, metadata round-tripping, and offset validation. - **Documentation/Chores** - Updated fragment metadata `repr` expectations and improved default `max_bytes_per_file` for fragment writing. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (1M context) <[email protected]> (cherry picked from commit 9681621)
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.
Data model, feature flag, and write/commit foundation for Data Overlay Files (OSS-1322).
Stacked on #7381 (spec/proto). To keep this diff clean, the base is an in-repo mirror of #7381's branch (
will/data-overlay-spec-base); retarget tomainonce #7381 lands. (#7406 / OSS-1323, needed for the sparse overlay sink, has merged.)What's here
lance-table/src/format/overlay.rs) —DataOverlayFile+OverlayCoverageonFragment.overlays, dense (shared_offset_bitmap) and sparse (per-field). Lives in its own module: a small public surface (the two types,dense/sparse,coverage_for_field) with the coverage / rank / parse-once / newest-last invariants documented at the module level and the serde/proto/Roaring plumbing kept private. Coverage bitmaps are parsed once on load intoArc<RoaringBitmap>(not re-deserialized per access), and a fragment's overlays are stable-sorted bycommitted_version(newest last) on load so resolution can rely on the ordering. Protobuf/serde round-trip +coverage_for_fieldtested.FLAG_DATA_OVERLAY_FILES) — set when any fragment has overlays. Release-gated: treated as an unknown flag in release builds (so release readers/writers refuse overlay datasets) unlessLANCE_ENABLE_DATA_OVERLAY_FILESis set; debug builds understand it. Tested (release-gating policy is asserted profile-independently).Operation::DataOverlaytransaction — appends overlays to a fragment (preserving concurrently-written ones) and stampscommitted_versionat commit, re-stamped on retry. Protobuf round-trip + multi-fragmentbuild_manifest(distinct targets + untargeted pass-through) tested.CommitConflictResolver) — permissive likeDataReplacement: compatible with append / column-rewrite / index-build / data-replacement / other overlays, and with deletes/updates that leave the overlaid fragment in place. Retryable when a concurrent op row-rewrites or consumes the overlays on an overlaid fragment (Rewrite/Merge) or removes one (Update/Deleteremoval); incompatible with whole-datasetOverwrite/Restoreand withUpdateMemWalState(matching the spec — both commit directions now agree). Tested (both-direction matrix, including remove-fragment andRewrite×overlay).The fragment read path refuses overlays until the scan merge lands (OSS-1324).
Follow-ups on this stack
DataOverlayFileWriter— streaming dense+sparse sink, used internally behindupdate/merge_insert.validate()overlay checks — value-column length vs. coverage cardinality, dtype vs. schema (I/O-bound; cheap structural invariants are enforced at parse/commit time).Blocks OSS-1324 (take/scan), OSS-1325 (index masking), OSS-1326 (compaction).
🤖 Generated with Claude Code
Summary by CodeRabbit
DataOverlaytransaction support to append per-fragment overlay updates during commits, including newest-last overlay ordering validation.