fix(compaction): preserve row order across parallel tasks

[lance-gatefixer]

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

[lance-gatekeeper]

❌ Gate recommendation: request changes.

Canonicalizing completed tasks is useful, but it only orders rewritten groups. It does not satisfy compaction's documented insertion-order contract for valid partial or gapped compactions, so it does not fully resolve #3465.

A complete revision must preserve replacement ranges relative to untouched fragments, not only relative to other results. If fresh monotonically allocated fragment IDs cannot represent that in an ID-sorted manifest, the design needs a different logical-order mechanism or an explicit contract and scope decision.

[lance-gatekeeper]

Sorting the completed tasks only preserves order among rewritten groups; it does not preserve their position relative to untouched fragments. Replacement fragments receive IDs above the current maximum and transaction commit globally sorts by fragment ID, so a bounded or gapped compaction moves earlier rewritten rows after untouched rows.

I reproduced this on the current head by adding the following assertion after the first compaction in the existing test_max_source_fragments:

let after_first_data = dataset.scan().try_into_batch().await.unwrap();
assert_eq!(after_first_data, data.slice(0, 1_000));

Then I ran cargo test -p lance test_max_source_fragments -- --nocapture. It failed with rows starting at 300 and ending at 299, rather than 0..999.

The commit path must preserve every replacement range relative to untouched fragments, or explicitly narrow the promised ordering contract; canonicalizing task completion order alone cannot do that under fresh monotonically allocated IDs plus ID-sorted manifests.

[lance-gatefixer]

Addressed in 46c1744. Bounded plans now select a contiguous suffix (including across candidate gaps), the reported test_max_source_fragments scan assertion passes for legacy and stable formats, append races retry, and the Rust/Python APIs explicitly scope other manual partial or gapped result sets.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

Two independent paths still violate the bounded-compaction ordering contract, so the current revision can either make no progress or reorder rows around concurrent insertions.

A complete revision should preserve logical placement independently of fresh fragment IDs. If the ID-sorted manifest remains the mechanism, it must at minimum guarantee bounded work makes progress across repeated runs and classify every transaction shape that adds rows as ordering-sensitive.

[lance-gatekeeper]

Reversing the already-split whole tasks can make a valid bounded request select nothing. With ten 256-Kirow fragments and the default 1-Mirow target, split_for_size produces a 4-fragment task followed by a 6-fragment task. This loop examines the 6-fragment task first, exceeds max_source_fragments=4, and take_while returns an empty plan. That violates the documented incremental/oldest-first contract and makes a normal bounded compaction a no-op. Selecting the bounded suffix before task grouping can fix this immediate case, but repeated runs also need a logical-order mechanism so a newly compacted tail cannot permanently block older candidates.

Reproducer

cd python
uv run pytest python/tests/test_optimize.py::test_compact_files_max_source_fragments

Expected fragments_removed == 4; observed 0.

[lance-gatefixer]

Addressed in e47e8df. Manifest position now defines logical row order independently of fragment IDs, bounded planning is oldest-first again, and repeated bounded runs make strict progress while preserving row order; the reported Python reproducer and Legacy/Stable regressions pass.

[lance-gatekeeper]

This only rejects Operation::Append, but merge-insert can add rows as Operation::Update { new_fragments: nonempty, updated_fragments: [], removed_fragment_ids: [] }. The later Update arm ignores new_fragments, so a rewrite rebase returns Ok(()); those inserted fragments receive lower fresh IDs than the rewrite replacements, and manifest sorting moves the original rewritten rows after the inserts. Treat any row-adding Update as the same retryable ordering conflict as Append, or preserve logical placement independently of IDs.

Reproducer

I added this regression beside test_conflicts and ran it against this head:

#[test]
fn test_rewrite_conflicts_with_append_like_update() {
    use crate::dataset::transaction::UpdateMode;

    let operation = Operation::Rewrite {
        groups: vec![RewriteGroup {
            old_fragments: vec![Fragment::new(0)],
            new_fragments: vec![Fragment::new(2)],
        }],
        rewritten_indices: vec![],
        frag_reuse_index: None,
    };
    let mut rebase = TransactionRebase {
        transaction: Transaction::new(0, operation.clone(), None),
        initial_fragments: HashMap::new(),
        modified_fragment_ids: modified_fragment_ids(&operation).collect::<HashSet<_>>(),
        affected_rows: None,
        conflicting_frag_reuse_indices: Vec::new(),
        conflicting_mem_wal_compacted_sstables: Vec::new(),
    };
    let other = Transaction::new(
        0,
        Operation::Update {
            removed_fragment_ids: vec![],
            updated_fragments: vec![],
            new_fragments: vec![Fragment::new(1)],
            fields_modified: vec![],
            compacted_sstables: Vec::new(),
            fields_for_preserving_frag_bitmap: vec![],
            update_mode: Some(UpdateMode::RewriteRows),
            inserted_rows_filter: None,
            updated_fragment_offsets: None,
        },
        None,
    );

    let result = rebase.check_txn(&other, 1);
    assert!(matches!(result, Err(Error::RetryableCommitConflict { .. })));
}

cargo test -p lance test_rewrite_conflicts_with_append_like_update -- --nocapture

Expected a retryable conflict; observed Ok(()).

[lance-gatefixer]

Addressed in e47e8df. Rewrites now splice replacements at their logical source positions, so concurrent Append and row-adding Update fragments remain after the rewritten rows without forcing a retry; regressions cover both compatibility and resulting manifest order.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The new logical-order representation fixes bounded compaction on this head, but two independent paths still fail the manifest-order contract across version and transaction boundaries.

A viable revision would explicitly gate the new representation for readers and writers, with mixed-version evidence, and preserve existing logical positions for every transaction variant; alternatively, keep stable manifests ID-sorted and encode logical order separately.

[lance-gatekeeper]

This changes the stable manifest representation from fragment-ID order to logical row order without a reader/writer feature boundary. The v9.0.1 reader maps bitmap rank directly to manifest position, so a manifest written here as [5, 2, 3, 4] resolves ID 5 to fragment 4; released validation also rejects descending IDs, and an older writer can re-sort the manifest and change row order. Gate this representation with explicit reader/writer feature or format versions and mixed-version tests, or keep stable manifests ID-sorted and store logical order separately.

Reproducer

I added this after the first compaction in test_bounded_compaction_preserves_order_across_candidate_gap:

let release_index = dataset.fragment_bitmap.rank(5) as usize - 1;
assert_eq!(dataset.manifest.fragments[release_index].id, 5);

Then ran:

cargo test -p lance test_bounded_compaction_preserves_order_across_candidate_gap -- --nocapture

Expected fragment 5; observed fragment 4 for both Legacy and Stable cases.

[lance-gatefixer]

Addressed in 3e8e101. Non-ID-sorted manifests now set the logical-order reader and writer feature bit, so pre-feature releases reject them; commit and validation boundaries enforce the flag, with mixed-version and Legacy/Stable regressions.

[lance-gatekeeper]

This logical-order repair is limited to DataReplacement. With the final global sort removed, Operation::Merge still uses the caller-provided vector as logical row order, while merge_fragments_valid checks membership and counts rather than order. A valid reversed vector therefore reverses the dataset. Apply the same current-manifest-order reassembly to existing merge fragments before finalizing the manifest.

Reproducer

I built a manifest with IDs [0, 1, 2], supplied a Merge with the same fragments reversed, verified validate_operation accepted it, and asserted the built manifest remained [0, 1, 2].

cargo test -p lance merge_build_manifest_preserves_logical_fragment_order -- --nocapture

Expected [0, 1, 2]; observed [2, 1, 0].

[lance-gatefixer]

Addressed in 3e8e101. Merge now reassembles every existing fragment in current manifest order and appends only genuinely new fragments in supplied order; the accepted reversed-vector reproducer now remains [0, 1, 2].

[github-actions]

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.

[lance-gatekeeper]

🟡 Gate recommendation: maintainer decision required.

The implementation now resolves the verified ordering and mixed-version failures. The remaining choice is whether to adopt logical fragment order as a stable table-format feature: it preserves row order for bounded compaction, but datasets using it require readers and writers that recognize the new feature bit.

If that contract is intended, please link the required completed PMC format-spec vote. Otherwise, retain the ID-sorted manifest contract and choose a compaction design that does not introduce this format feature.

[lance-gatefixer]

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]

❌ Gate recommendation: request changes.

The stable ID-sorted representation is restored, but the suffix-relabel mechanism makes bounded compaction table-wide and also rejects released manifests whose untouched fragments lack reliable row counts.

A viable revision must keep compaction work proportional to its configured bound and preserve released-manifest inputs. Under the current format, that can mean limiting partial compaction to ranges whose required relabel/remap scope is itself bounded; a different logical-order mechanism needs the separate format decision.

[lance-gatekeeper]

Creating a rewrite group for every untouched trailing fragment makes max_source_fragments cease to bound commit work. With any remappable index, these groups enter the default IndexRemapMode::Direct path, where transpose_row_addrs materializes one hash-map entry per trailing physical row; the changed index tests also confirm physical-address coverage is emptied across the relabeled suffix. A small early compaction on a billion-row suffix can therefore attempt roughly a billion mappings and table-wide index work or fallback scans.

Keep the required relabel/remap scope bounded—for example, only admit partial compaction when the affected suffix is within the configured resource bound—or use an order representation that does not change every trailing physical address. Switching only to compact remap does not bound index, deletion-file, conflict, or ID churn.

Reproducer

I added a focused test on this head with one selected row followed by an untouched 100,000-row fragment. It called complete_rewrite_suffix(..., capture_row_addrs = true), assigned the fresh output IDs, and ran the same transpose_row_addrs/direct_map.extend loop as commit_compaction:

assert_eq!(tasks.len(), 2);
let mut direct_map = HashMap::new();
for task in tasks {
    let row_addrs = RoaringTreemap::deserialize_from(
        &mut Cursor::new(task.row_addrs.as_ref().unwrap()),
    )
    .unwrap();
    direct_map.extend(remapping::transpose_row_addrs(
        row_addrs,
        &task.original_fragments,
        &task.new_fragments,
    ));
}
assert_eq!(direct_map.len(), 100_001);

cargo test -p lance test_disposable_direct_remap_expands_bounded_suffix -- --nocapture passed with direct_map.len() == 100_001, demonstrating that the map grows with the untouched suffix, not the one-row selected source.

[lance-gatefixer]

Addressed in 56ee824. max_source_fragments now bounds the entire manifest suffix whose identities may change; bounded planning selects candidates only from that suffix, and manual or stale commits reject or retry when the relabel scope exceeds the configured limit. This prevents table-wide suffix remapping under a small bound.

[lance-gatekeeper]

This reads physical_rows directly from an untouched suffix fragment, but released legacy manifests may omit it or contain the inaccurate values that rewrite_files already migrates for selected source fragments. The untouched suffix is not migrated here: a missing count aborts a valid partial compaction, while an inaccurate count builds an incomplete or overlong row-address remap.

Migrate/recompute every relabeled suffix fragment before serializing its addresses and cover this with a released historical fixture, or avoid relabeling untouched legacy fragments.

Reproducer

On this head I added a focused test with a selected source having physical_rows = Some(1) and an untouched Fragment::new(1), whose count is None:

let error = complete_rewrite_suffix(
    &[source.clone(), trailing],
    vec![RewriteResult {
        metrics: CompactionMetrics::default(),
        new_fragments: vec![output],
        read_version: 1,
        original_fragments: vec![source],
        row_addrs: Some(Vec::new()),
    }],
    true,
    1,
    1,
)
.unwrap_err();
assert!(error.to_string().contains("missing physical_rows"));

cargo test -p lance test_disposable_partial_rewrite_rejects_unmigrated_suffix -- --nocapture passed by observing the unexpected rejection.

[lance-gatefixer]

Addressed in 56ee824. Metadata-only relabel fragments are now migrated before physical addresses are serialized; manifests without writer-version metadata recompute their fragment statistics. A released v0.7.5 fixture verifies that a missing physical_rows count is recovered as the complete 100-row address set.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The suffix cap bounds relabel work, but it replaces the established oldest-first incremental behavior with tail-only planning that can permanently leave eligible fragments untouched.

A viable revision must preserve a progress contract as well as row order and bounded work. Decouple logical order from fresh fragment IDs, or make tail-only/no-progress semantics an explicit maintainer decision instead of describing this option as bounded incremental compaction.

[lance-gatekeeper]

This hard suffix cut can permanently strand eligible compaction work. With fragment row counts [100, 100, 1000, 1000], target_rows_per_fragment=250, and max_source_fragments=2, fragments 0 and 1 are an eligible pair, but the healthy final pair makes suffix_start=2, so this branch removes the only candidate bin. Every repeated bounded run therefore returns an empty plan rather than advancing the oldest eligible work.

The planner needs to keep earlier eligible work reachable within the accepted contract. If that requires logical order independent of fresh IDs, the format decision must be explicit rather than silently turning this incremental option into a permanent no-op.

Reproducer

I added this focused test on the current head:

#[tokio::test]
async fn test_max_source_fragments_progresses_before_healthy_suffix() {
    let test_dir = TempStrDir::default();
    let data = sample_data();
    let schema = data.schema();
    let fragment_rows = [100, 100, 1_000, 1_000];
    let write_params = WriteParams {
        max_rows_per_file: 1_000,
        ..Default::default()
    };

    Dataset::write(
        RecordBatchIterator::new(vec![Ok(data.slice(0, fragment_rows[0]))], schema.clone()),
        &test_dir,
        Some(write_params.clone()),
    )
    .await
    .unwrap();
    let mut offset = fragment_rows[0];
    for row_count in fragment_rows.iter().copied().skip(1) {
        let mut append_params = write_params.clone();
        append_params.mode = WriteMode::Append;
        Dataset::write(
            RecordBatchIterator::new(vec![Ok(data.slice(offset, row_count))], schema.clone()),
            &test_dir,
            Some(append_params),
        )
        .await
        .unwrap();
        offset += row_count;
    }

    let dataset = Dataset::open(&test_dir).await.unwrap();
    let options = CompactionOptions {
        target_rows_per_fragment: 250,
        max_source_fragments: Some(2),
        ..Default::default()
    };
    let plan = plan_compaction(&dataset, &options).await.unwrap();
    let planned_fragment_ids = plan
        .tasks()
        .iter()
        .flat_map(|task| task.fragments.iter().map(|fragment| fragment.id))
        .collect::<Vec<_>>();
    assert_eq!(planned_fragment_ids, vec![0, 1]);
}

cargo test -p lance test_max_source_fragments_progresses_before_healthy_suffix -- --nocapture

Expected [0, 1]; observed [].