fix(compaction): preserve row order across parallel tasks - #8400
lance-gatefixer[bot] wants to merge 23 commits into
Conversation
|
Important This PR touches the Lance format specification. Substantive changes to the format specification — the If this is a meaningful format change:
|
|
Addressed in 93b75ef. The repair now retains the ID-sorted manifest contract by reserving one ordered replacement suffix and metadata-only relabeling untouched trailing fragments. The logical-order feature bit, protobuf/spec changes, and format-vote dependency are removed; focused JsonIndex/FtsIndex upgrade-downgrade tests pass against the locally available historical releases. |
|
No code change was made at 56ee824. The requested |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
# Conflicts: # rust/lance/src/dataset/optimize.rs
|
No code change was made at d23b189. The current implementation takes the strict affected-fragment-bound option while preserving ID-sorted stable manifests and row order; guaranteeing oldest-first bounded progress as well would require either relaxing that bound or approving the separately reviewed logical-order format feature. Selecting that contract is a maintainer/PMC decision, not an implementation choice this repair can assume. |
# Conflicts: # java/src/main/java/org/lance/compaction/CompactionOptions.java # python/python/lance/dataset.py # rust/lance/src/dataset/optimize.rs
|
No code change was made at 58b67b4. The #8986 evidence confirms the existing contract boundary rather than supplying a compatible implementation: this head preserves ID-sorted manifests and strictly bounds every changed Fragment identity, which can necessarily strand older eligible work, and the #8986 author likewise verified that prefix splitting is unsafe under that representation. Adopting ordered partial commits as a format feature or relaxing the strict budget/progress guarantees still requires explicit maintainer/PMC direction. |
|
Blocked: PR #8400 still requires an explicit maintainer/PMC decision on the bounded-compaction contract before this repair can advance. At remote head The #8986 contribution now has a verified App disposition, all prior actionable threads retain valid dispositions, formatting and all 145 focused compaction tests pass, all 38 current-head checks succeed, and a normal push verified the published head. No code-only change can select among these incompatible compatibility guarantees without maintainer authority. The smallest unblock is for a maintainer to confirm that strict bound and exclusion semantics may sacrifice compaction progress. Alternatively, explicitly permit suffix relabel/remap work beyond the bound, or approve the logical-order format path and its required compatibility and PMC process. |
# Conflicts: # python/python/lance/dataset.py
# Conflicts: # rust/lance/src/io/commit.rs
|
Blocked: PR #8400 remains blocked by four failed current-head Rust jobs that belong to the separate CI-failure obligation. At remote head The review-feedback workflow re-read all reviews, PR comments, and inline threads. Current-head Gatekeeper Review The smallest unblock is to run the independent |
There was a problem hiding this comment.
The index-fixture follow-up confirms the suffix-relabel contract: trailing physical addresses move and are translated, while staged and FM-index segments claim only coverage they can serve. Production code is unchanged; the ordered-suffix repair and row-adding conflict safeguards remain supported.
A strict max_source_fragments bound or exclusion can still leave earlier eligible work uncompacted. This documented trade-off is recoverable by raising or removing the option, as #8986 independently illustrates.
|
Blocked: PR #8400 has no new actionable review request, but its current Gatekeeper Review does not contain the canonical approval recommendation needed to complete this review handoff. At remote head If the stated risk is accepted, the smallest unblock is a current-head Gatekeeper Review with the canonical approve recommendation. Alternatively, a reviewer can give a concrete new request identifying the code change still needed. |
There was a problem hiding this comment.
Merging #9347 adds in-file spilled row lineage. The ordered-suffix repair remains compatible: metadata-only relabels retain each lineage-bearing data file and its metadata, while rewrites preserve row order and lineage. The ID-sorted manifest and row-adding conflict safeguards remain intact.
A strict max_source_fragments bound or exclusion can still leave earlier eligible work uncompacted. Raising or removing that option restores progress; #8986 independently illustrates the bounded-planning trade-off.
Summary
Root cause
Compaction replacement fragments receive fresh IDs above the manifest high-water mark. The commit path globally sorts fragments by ID, so a replacement for an early range moved behind any untouched later fragments. Concurrently completed tasks could also arrive in a different order, and bounded compaction exposed the same defect repeatedly.
Fix
Compaction results are first ordered by their current source positions. Starting at the earliest rewritten range, the commit completes one ordered replacement suffix: real compaction outputs replace planned ranges, while untouched trailing fragments are represented by metadata-only replacements that keep their data files and receive fresh consecutive IDs. Deletion files are copied to the paths implied by the new fragment IDs, physical row-address indices are remapped, and stable-row-ID index coverage follows the relabeled fragments.
Stale distributed tasks recognize prior metadata-only relabels and rebase captured row addresses. Genuine source changes remain retryable conflicts, as do concurrent appends and row-adding updates that would invalidate the reserved suffix ordering. The commit boundary continues to require strictly increasing fragment IDs, so released readers and writers retain their existing representation and compatibility.
Validation
cargo fmt --all -- --checkcargo clippy --all --tests --benches -- -D warningscargo test -p lance dataset::optimize::tests -- --nocapture(117 passed)cargo test -p lance dataset::transaction::tests -- --nocapture(63 passed)cargo test -p lance io::commit::conflict_resolver::tests -- --nocapture(44 passed)cargo test -p lance test_check_fragment_ids_requires_sorted_order -- --nocapture(1 passed)cargo test -p lance test_compact_distributed -- --nocapture(4 passed)cargo test -p lance test_bounded_compaction_preserves_order_across_candidate_gap -- --nocapture(2 passed)make buildfrompython/Fixes #3465