feat(python): expose DataOverlay commit operation#7540
Conversation
d1e3be6 to
d1fa12b
Compare
3ba338b to
a7d4508
Compare
d1fa12b to
43cc06a
Compare
a7cfcf0 to
72cc6d2
Compare
43cc06a to
91b3987
Compare
| shared_offsets: Optional[List[int]] = None | ||
| field_offsets: Optional[List[List[int]]] = None |
There was a problem hiding this comment.
note: this could certainly be more efficient. I filed a follow up: #7695
| shared_offsets: Optional[List[int]] = None | ||
| field_offsets: Optional[List[List[int]]] = None |
There was a problem hiding this comment.
suggestion: it might be more pythonic if we had one field and accepted either type signature. Then we differentiate which it is based on the type.
91b3987 to
ecfb29c
Compare
📝 WalkthroughWalkthroughAdds cell-level data overlay operations, fragment metadata persistence, Python-Rust interoperability, and end-to-end coverage for dense, sparse, precedence, round-trip, and validation behavior. ChangesData Overlay Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Dataset
participant DataOverlay
participant PyO3Bindings
participant RustOperation
participant FragmentMetadata
Dataset->>DataOverlay: create overlay groups
DataOverlay->>PyO3Bindings: convert groups and coverage
PyO3Bindings->>RustOperation: construct DataOverlay operation
RustOperation->>FragmentMetadata: preserve overlay metadata
FragmentMetadata->>PyO3Bindings: serialize fragment overlays
PyO3Bindings->>Dataset: reconstruct overlay metadata
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
| // Python's FragmentMetadata does not carry overlays; they are added | ||
| // to a fragment via the DataOverlay commit operation, not through | ||
| // fragment metadata round-trips. | ||
| overlays: Vec::new(), |
There was a problem hiding this comment.
Wouldn't we still want it to round-trip? Most of our functions don't take Fragment as input so it's hard to come up with a good example.
Probably the only example I could think of would be a Merge operation. If I built a "deleting" Merge by taking a set of fragments and dropping one of them then the resulting commit would drop all the overlays from the fragments I keep too?
There was a problem hiding this comment.
I've got it so it can roundtrip now.
Addresses review feedback on #7540: - Reword the DataOverlayFile/DataOverlay docstrings: offset "list" not "set", drop the redundant "no row-offset key column" note, and clarify that the latest group wins when multiple groups target the same data. - Round-trip a fragment's overlays through FragmentMetadata so operations that pass existing fragments back (a manual Delete/Update/Merge commit) no longer silently drop them. FragmentMetadata gains an `overlays` field wired through to_json/from_json and both PyO3 conversions; DataOverlayFile gains `committed_version` (None on commit input, populated on read) so overlay precedence survives the round-trip. Adds test_data_overlay_round_trips_through_fragment_metadata covering the metadata, JSON, and commit round-trips. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
4bb5e39 to
5ff87ce
Compare
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
Adds LanceOperation.DataOverlay (with DataOverlayFile and DataOverlayGroup) so Python can commit data overlay files, mirroring the existing DataReplacement binding. A DataOverlayFile carries the value DataFile plus exactly one of shared_offsets (dense coverage) or field_offsets (sparse, per-field coverage); the commit stamps committed_version. Also fills in the overlays field on the Python FragmentMetadata -> Fragment conversion, which OSS-1322 left unset (overlays are committed via DataOverlay, not carried through fragment metadata). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review feedback on #7540: - Reword the DataOverlayFile/DataOverlay docstrings: offset "list" not "set", drop the redundant "no row-offset key column" note, and clarify that the latest group wins when multiple groups target the same data. - Round-trip a fragment's overlays through FragmentMetadata so operations that pass existing fragments back (a manual Delete/Update/Merge commit) no longer silently drop them. FragmentMetadata gains an `overlays` field wired through to_json/from_json and both PyO3 conversions; DataOverlayFile gains `committed_version` (None on commit input, populated on read) so overlay precedence survives the round-trip. Adds test_data_overlay_round_trips_through_fragment_metadata covering the metadata, JSON, and commit round-trips. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
6d52845 to
968d024
Compare
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/transaction.rs`:
- Around line 210-249: The PyLance<DataOverlayFile>::extract implementation must
reject unsorted offsets before constructing OverlayCoverage, rather than
allowing RoaringBitmap::from_iter to reorder them. Validate that the dense
offsets list and every sparse per-field list are strictly ascending, return a
clear ValueError on failure, and preserve the existing dense/sparse parsing
behavior otherwise. The corresponding dataset.py site requires no direct change;
it is covered by this Rust-side validation.
🪄 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: 0c7ed629-7e5b-4d27-bb03-c03381753098
📒 Files selected for processing (5)
python/python/lance/dataset.pypython/python/lance/fragment.pypython/python/tests/test_dataset.pypython/src/fragment.rspython/src/transaction.rs
The overlay `offsets` list maps positionally to value rows in `data_file`, but `RoaringBitmap::from_iter` silently sorts and dedups the offsets, so a non-ascending or duplicated list would corrupt that mapping without error. Validate that each dense/sparse offset list is strictly ascending and raise a clear ValueError otherwise. A proper RoaringBitmap Python binding (#7695) will supersede this. Also update `test_fragment_meta` to expect the new `overlays=[]` field in the `FragmentMetadata` repr, which was failing Python CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 (2)
python/python/lance/dataset.py (1)
5994-6062: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd examples and API links for the new public overlay operations.
DataOverlayFile,DataOverlayGroup, andDataOverlayare public APIs but lack usage examples and links to related types orLanceDataset.commit.As per coding guidelines, “Document all public APIs with examples and links 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 `@python/python/lance/dataset.py` around lines 5994 - 6062, Add documentation examples and cross-references to the public APIs DataOverlayFile, DataOverlayGroup, and DataOverlay. Show how to construct and use overlay operations, link related types such as DataFile and FragmentMetadata, and reference LanceDataset.commit for applying DataOverlay, while preserving the existing attribute and behavior descriptions.Source: Coding guidelines
python/src/transaction.rs (1)
234-247: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReport out-of-range offsets as a value error.
offsets=[-1]and values aboveu32::MAXfall through both extraction branches and end up with the generic “must be a list of ints” message. Reject them with a bounds-specific error and add coverage for those cases.🤖 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/transaction.rs` around lines 234 - 247, Update the offsets parsing flow around the dense and sparse `offsets.extract` branches to detect integer offsets outside the u32 range and return a PyValueError with a bounds-specific message instead of the generic type error. Preserve valid dense and sparse coverage handling, and add tests covering negative offsets and values above u32::MAX.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 `@python/python/lance/dataset.py`:
- Around line 5994-6062: Add documentation examples and cross-references to the
public APIs DataOverlayFile, DataOverlayGroup, and DataOverlay. Show how to
construct and use overlay operations, link related types such as DataFile and
FragmentMetadata, and reference LanceDataset.commit for applying DataOverlay,
while preserving the existing attribute and behavior descriptions.
In `@python/src/transaction.rs`:
- Around line 234-247: Update the offsets parsing flow around the dense and
sparse `offsets.extract` branches to detect integer offsets outside the u32
range and return a PyValueError with a bounds-specific message instead of the
generic type error. Preserve valid dense and sparse coverage handling, and add
tests covering negative offsets and values above u32::MAX.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1afbc6a3-a664-4ce5-91d1-6167dff20438
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/tests/test_dataset.pypython/python/tests/test_fragment.pypython/src/transaction.rs
… API `DataOverlayFile` takes a single `offsets` param, not `shared_offsets`, and the release-build opt-in env var is `LANCE_ENABLE_UNSTABLE_DATA_OVERLAY_FILES` (not `LANCE_ENABLE_DATA_OVERLAY_FILES`). Both diverged from the benchmark helper after PR #7540 landed the real Python bindings; commit()'s returned handle skipped flag validation so this only surfaced on a fresh dataset open.
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) <noreply@anthropic.com> (cherry picked from commit 9681621)
… API `DataOverlayFile` takes a single `offsets` param, not `shared_offsets`, and the release-build opt-in env var is `LANCE_ENABLE_UNSTABLE_DATA_OVERLAY_FILES` (not `LANCE_ENABLE_DATA_OVERLAY_FILES`). Both diverged from the benchmark helper after PR #7540 landed the real Python bindings; commit()'s returned handle skipped flag validation so this only surfaced on a fresh dataset open.
Exposes the
DataOverlaycommit 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 existingDataReplacementbinding.What's here
LanceOperation.DataOverlaywithDataOverlayFileandDataOverlayGroup. ADataOverlayFilecarries the valueDataFileplus exactly one ofshared_offsets(dense coverage shared by every field) orfield_offsets(sparse, one offset set per field). The commit stampscommitted_version; passing both/neither coverage is rejected with a clear error.python/src/transaction.rs.overlaysfield on the PythonFragmentMetadata -> Fragmentconversion, which OSS-1322 left unset (Python metadata doesn't carry overlays — they're committed viaDataOverlay).Tests
test_data_overlay_*intest_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.
🤖 Generated with Claude Code
Summary by CodeRabbit
DataOverlayoperations to append fragment overlays without rewriting entire fragments.reprexpectations and improved defaultmax_bytes_per_filefor fragment writing.