fix(python): added cleanup_frag_reuse_index#7221
Conversation
f2eaabb to
d90747b
Compare
Xuanwo
left a comment
Xuanwo
left a comment
There was a problem hiding this comment.
- The new Python cleanup entry point can panic when fragment-reuse metadata cannot be loaded. Corrupt or stale metadata can surface as a panic instead of a normal Python exception.
- The current test only exercises the already-caught-up index case. A cleanup implementation that drops all generations would still pass, so the regression can reappear.
…-frag-reuse-index # Conflicts: # python/python/tests/test_optimize.py
Thanks for the feedback @Xuanwo, addressed both issues. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Also moved to |
| Ok(true) => {} | ||
| Ok(false) => all_caught_up = false, | ||
| // An InvalidInput error means the reuse version is likely corrupted; drop it. | ||
| Err(Error::InvalidInput { .. }) => continue 'versions, |
There was a problem hiding this comment.
The cleanup path treats a partially indexed rewrite group as corrupt and drops the reuse generation, while the load path treats the same straddling state as recoverable. Calling the new Python cleanup API on an older dataset can remove the generation that keeps affected indexes safe.
There was a problem hiding this comment.
Good catch, fixed.
| .await | ||
| .unwrap(); | ||
| // Surface corrupt/stale metadata as a normal error instead of panicking on unwrap. | ||
| let frag_reuse_details = load_frag_reuse_index_details(dataset, frag_reuse_index_meta).await?; |
There was a problem hiding this comment.
The cleanup path still reads external fragment-reuse details with unchecked offset/size arithmetic. Malformed metadata can still panic or produce an invalid object-store range instead of returning a Python exception.
There was a problem hiding this comment.
Fixed in load_frag_reuse_index_details. The read now converts offset and size with usize::try_from so overflow errors instead of truncating, computes the end with checked_add so it errors instead of wrapping, and stats the file via reader.size() to reject any range past EOF before calling get_range.
…d bound metadata reads
…-frag-reuse-index
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Python API for pruning obsolete fragment-reuse generations, strengthens Rust cleanup retention and error handling, validates external fragment-reuse detail ranges, and rejects invalid scanner batch readahead values. ChangesFragment-Reuse Cleanup and Validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DatasetOptimizer
participant PythonDataset
participant FragReuseCleanup
participant FragReuseIndex
DatasetOptimizer->>PythonDataset: cleanup_frag_reuse_index()
PythonDataset->>FragReuseCleanup: clone dataset and invoke cleanup
FragReuseCleanup->>FragReuseIndex: evaluate reuse generations against all indexes
FragReuseIndex-->>FragReuseCleanup: retain or remove generations
FragReuseCleanup-->>PythonDataset: cleaned dataset or error
PythonDataset-->>DatasetOptimizer: None or propagated error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance/src/dataset/index/frag_reuse.rs`:
- Around line 336-339: Update the regression test around
cleanup_frag_reuse_index to capture its returned error and assert both the
expected error variant and identifying message content, rather than only calling
is_err(). Use the existing error type and message produced for corrupt
frag-reuse metadata so unrelated failures cannot satisfy the test.
In `@rust/lance/src/index/frag_reuse.rs`:
- Around line 236-285: Refactor
test_load_external_details_malformed_offset_size_errors into an rstest
parameterized test, with named cases #[case::out_of_bounds(...)] for
payload-length-plus-one and #[case::offset_past_eof(...)] for the past-EOF
offset, plus #[case::overflow(...)] for u64::MAX values. Pass each case’s offset
and size to external_details_meta, retain the shared setup and error assertions,
and adjust the expected-message assertion so it applies only to the
out-of-bounds case if needed.
- Around line 56-86: Update the final two malformed-input test cases around the
fragment reuse validation tests to assert the specific error variant and
message, rather than only checking is_err(). Cover the expected errors for the
external file range overflow and out-of-bounds conditions, using the relevant
fragment reuse test function and error construction paths as references.
🪄 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: 0e26bc62-db67-4690-b162-5fc9673f2066
📒 Files selected for processing (6)
python/python/lance/dataset.pypython/python/lance/lance/__init__.pyipython/python/tests/test_optimize.pypython/src/dataset.rsrust/lance/src/dataset/index/frag_reuse.rsrust/lance/src/index/frag_reuse.rs
d7fd04a to
b26e9ac
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 `@rust/lance/src/dataset/index/frag_reuse.rs`:
- Line 44: The retained_versions buffer is initialized without pre-allocating
known capacity. In the relevant fragment-reuse logic, change its initialization
to Vec::with_capacity(frag_reuse_details.versions.len()) before the loop.
🪄 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: 432d9243-56c5-4cd5-b7f6-00773076a5a9
📒 Files selected for processing (6)
python/python/lance/dataset.pypython/python/lance/lance/__init__.pyipython/python/tests/test_optimize.pypython/src/dataset.rsrust/lance/src/dataset/index/frag_reuse.rsrust/lance/src/index/frag_reuse.rs
|
@Xuanwo I think this is ready. |
…-frag-reuse-index
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
python/python/lance/dataset.py-6284-6290 (1)
6284-6290: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-integral
batch_readaheadvalues at this boundary.
int(nbatches)is used only for validation, but the original value is stored. Therefore values such as1.5or"1"can pass this check and fail later when the native scanner expects an integer. Validate integer semantics and store the normalized value.As per coding guidelines, API boundaries must reject invalid values with descriptive errors and must not silently adjust them.
Proposed fix
- if nbatches is not None and int(nbatches) <= 0: - raise ValueError("batch_readahead must be greater than 0") + if nbatches is not None: + if not isinstance(nbatches, int): + raise TypeError("batch_readahead must be an integer") + if nbatches <= 0: + raise ValueError("batch_readahead must be greater than 0")🤖 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 6284 - 6290, Update the batch_readahead validation in the shown setter to reject non-integral values, including numeric strings and fractional numbers, with a descriptive ValueError. Preserve None as allowed, validate positive integer semantics without silently coercing or rounding, and store the normalized integer value after validation.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.
Other comments:
In `@python/python/lance/dataset.py`:
- Around line 6284-6290: Update the batch_readahead validation in the shown
setter to reject non-integral values, including numeric strings and fractional
numbers, with a descriptive ValueError. Preserve None as allowed, validate
positive integer semantics without silently coercing or rounding, and store the
normalized integer value after validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Pro Plus
Run ID: 10a2fa99-8691-4ae8-8045-6bcff990d06a
📒 Files selected for processing (4)
python/python/lance/dataset.pypython/python/lance/lance/__init__.pyipython/python/tests/test_optimize.pypython/src/dataset.rs
2 participants
Closes #7220
Expose the existing Rust frag-reuse pruner to Python as
LanceDataset.cleanup_frag_reuse_index().Deferred-remap compaction appends a generation to the
__lance_frag_reuseindex on every commit, and without pruning these accumulate forever and are loaded on every vector index open. The race-safe pruner from #3836 already exists in Rust but had no Python binding and is never invoked automatically.The binding follows the
migrate_manifest_paths_v2wrapper pattern, the Python method is documented besidecleanup_old_versionsas a periodic maintenance call, and a new test verifies that__lance_frag_reusereports zero versions once all indexes have caught up and the cleanup runs.