Skip to content

fix(compaction): preserve row order across parallel tasks - #8400

Open
lance-gatefixer[bot] wants to merge 23 commits into
mainfrom
gatekeeper/fix-3465-1
Open

lance-gatefixer[bot] wants to merge 23 commits into
mainfrom
gatekeeper/fix-3465-1

Conversation

@lance-gatefixer

@lance-gatefixer lance-gatefixer Bot commented Aug 7, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • preserve compaction row order for out-of-order, partial, and gapped result sets while keeping manifests sorted by fragment ID
  • relabel untouched trailing fragments with newly reserved IDs while reusing their data files and preserving deletion/index metadata
  • rebase stale distributed compaction results across metadata-only relabels and retry row-adding concurrent transactions
  • retain the released table format without a new protobuf field, feature flag, or format-spec change

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 -- --check
  • cargo clippy --all --tests --benches -- -D warnings
  • cargo 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)
  • rebuilt the local Python extension with make build from python/
  • focused JsonIndex/FtsIndex upgrade-downgrade compatibility tests (8 passed across Lance 0.36.0, 8.0.1, 9.0.1, and 10.0.0)

Fixes #3465

@github-actions github-actions Bot added the bug Something isn't working label Aug 7, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@github-actions github-actions Bot added the A-python Python bindings label Aug 7, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Important

This PR touches the Lance format specification.

Substantive changes to the format specification — the .proto definitions
and the spec docs under docs/src/format/ — require a PMC vote before merge.
Minor edits such as typo fixes, wording, or formatting are excluded; use your
judgment.

If this is a meaningful format change:

  • Start a vote following the Lance community voting process.
    Format specification modifications need 3 binding +1 votes (excluding the
    proposer), held on GitHub Discussions, with a minimum voting period of 1 week.
  • Once the vote passes, link the completed vote in this PR. It should not be
    merged until the vote is linked.

@github-actions github-actions Bot added the A-format On-disk format: protos and format spec docs label Aug 7, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

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.

lance-gatekeeper[bot]

This comment was marked as outdated.

@github-actions github-actions Bot added the A-java Java bindings + JNI label Aug 7, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

No code change was made at 56ee824. The requested breaking-change label has been added and verified on this PR. The remaining contract choice—strictly bounding all identity changes versus guaranteeing incremental progress, or adopting logical fragment order as a new format feature—requires the maintainer/PMC decision described in the review.

@codecov

codecov Bot commented Aug 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@Xuanwo Xuanwo added the K-decision Latest Gatekeeper review requires a maintainer decision. label Aug 10, 2026
# Conflicts:
#	rust/lance/src/dataset/optimize.rs
@lance-gatekeeper lance-gatekeeper Bot removed the K-decision Latest Gatekeeper review requires a maintainer decision. label Aug 11, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added the K-decision Latest Gatekeeper review requires a maintainer decision. label Aug 11, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

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
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

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.

@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Blocked: PR #8400 still requires an explicit maintainer/PMC decision on the bounded-compaction contract before this repair can advance.

At remote head 58b67b44105ae2306db5670fb4c6fe1661796b16, current-head Gatekeeper Review PRR_kwDOHn_KdM8AAAABLfBhtw requires maintainers to choose among strict affected-fragment bounds, oldest-first progress, and a logical-order format feature. The newer #8986 cross-reference IC_kwDOHn_KdM8AAAABSf5PFQ independently confirms that prefix splitting reorders rows or stalls at [200, 300], and that source Fragment ID reuse is not compatible.

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.

@lance-gatekeeper lance-gatekeeper Bot removed the K-decision Latest Gatekeeper review requires a maintainer decision. label Sep 14, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 14, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

Blocked: PR #8400 remains blocked by four failed current-head Rust jobs that belong to the separate CI-failure obligation.

At remote head 848e002661dea1d450117c783120a66ada225ef9, Rust workflow run 34863669640 completed with failures in linux-build (job 104041990111), linux-arm (104041990493), mac-build (stable) (104041990509), and windows-build (104041990570). The current main tip c10fd798ce2f636f795abb0fa9c49deefe0ef28e is already an ancestor of this repair head.

The review-feedback workflow re-read all reviews, PR comments, and inline threads. Current-head Gatekeeper Review PRR_kwDOHn_KdM8AAAABNfAIgg recommends approval with a non-blocking risk, every prior actionable external contribution retains its verified App disposition, and the newer inline activity is reconciliation rather than a new request. No review code change, duplicate acknowledgement, thread resolution, or approval-only completion line is appropriate.

The smallest unblock is to run the independent ci_failure workflow against those job logs, repair any attributable failure, validate, and push this same branch. If the logs instead establish a flaky or infrastructure-only failure, rerunning the affected Rust workflow after that determination is the practical alternative.

@lance-gatekeeper lance-gatekeeper Bot removed K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 27, 2026
lance-gatekeeper[bot]

This comment was marked as outdated.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 27, 2026
@lance-gatekeeper lance-gatekeeper Bot removed K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 27, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Gate recommendation: approve with a non-blocking risk.

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.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 27, 2026
@lance-gatefixer

Copy link
Copy Markdown
Contributor Author

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 4f9a19fbcaa6fcfb3d2a5a261e9b004d85efbc6f, Gatekeeper Review PRR_kwDOHn_KdM8AAAABPZc8zg says “approve with a non-blocking risk.” I re-read the reviews, PR comments, and inline threads: each current actionable thread has an earlier App disposition published after its latest actionable contribution, the recent Gatekeeper replies only reconcile those findings, and the test-only head retains the addressed behavior. There is no further code change or new feedback item to acknowledge.

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.

@lance-gatekeeper lance-gatekeeper Bot removed K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 29, 2026

@lance-gatekeeper lance-gatekeeper Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Gate recommendation: approve with a non-blocking risk.

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.

@lance-gatekeeper lance-gatekeeper Bot added K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk. labels Sep 29, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-format On-disk format: protos and format spec docs A-java Java bindings + JNI A-python Python bindings breaking-change bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance. K-risk Latest Gatekeeper recommendation includes a non-blocking risk.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG](python): The order of the table was changed after executing the compact_files operation

2 participants