fix(compaction): skip collect rowids for unindexed fragment or stable row ids

[zhangyue19921010]

In compaction, collecting and transferring fragment rowids / row_addrs is expensive.

If the fragments in a compaction task do not belong to any index, then this data does not require immediate remapping

therefore does not require collecting the old rowids

[codecov]

Codecov Report

❌ Patch coverage is 96.24060% with 5 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
rust/lance/src/dataset/optimize.rs	96.24%	3 Missing and 2 partials ⚠️

📢 Thoughts on this report? Let us know!

[pratik0316]

maybe I am missing some context, just curious why do we always need to capture the row_addr column in case the fragments have an index?

[zhangyue19921010]

Indexed fragments need row addr capture because compaction changes the physical addresses that existing indexes point to(unstable row IDs).To keep the index valid, compaction needs a mapping() from old addresses to new addresses. The captured _rowaddr column gives the old addresses of the rows that survived the rewrite.

[pratik0316]

clear! thanks for the explanation @zhangyue19921010

[zhangyue19921010]

Hi @Xuanwo, would you mind taking a look? I’d really appreciate your help. Thanks in advance!

[zhangyue19921010]

Hi @wjones127 , would you mind taking a look? Thanks ：）

[yanghua]

@claude review

[claude]

🟡 The planner sets has_indexed_fragments: Some(!bin.indices.is_empty()) at line 708, but bin.indices comes from load_index_fragmaps (line 1044), which iterates all indices from dataset.load_indices() without filtering system indices (FRAG_REUSE_INDEX, MEM_WAL_INDEX). Meanwhile needs_row_addr_capture at line 1148 explicitly filters via !is_system_index(index). When a bin's fragments are covered only by FRAG_REUSE_INDEX (and no user index covers them anywhere in the dataset), the planner records has_indexed_fragments=Some(true) and needs_row_addr_capture short-circuits at line 1136-1138, defeating the optimization. Fix: filter bin.indices by !is_system_index in both line 708 and the bin-merging check at line 670, mirroring the user-index filter in needs_row_addr_capture.

Extended reasoning...

What the bug is

The planner and the runtime check disagree on what counts as an indexed fragment.

load_index_fragmaps (rust/lance/src/dataset/optimize.rs:1044-1057) walks every index returned by dataset.load_indices(), including system indices (FRAG_REUSE_INDEX_NAME and MEM_WAL_INDEX_NAME, see lance-index/src/lib.rs:347-349). FRAG_REUSE_INDEX is built with a populated fragment_bitmap whose contents are the new fragment IDs from prior compactions, so it does cause indices_containing_frag (line 624-631) to return a non-empty position list for those fragments.

The planner then sets has_indexed_fragments: Some(!bin.indices.is_empty()) at line 708, regardless of whether those indices are user or system.

In needs_row_addr_capture (line 1126-1150) the short-circuit at line 1135-1138 trusts that flag:

let has_indexed_fragments = task.has_indexed_fragments.unwrap_or(true);
if has_indexed_fragments {
    return Ok(true);
}

Only the fallback path (when has_indexed_fragments == Some(false)) filters system indices, at line 1148: .any(|index| !is_system_index(index)). The two paths are therefore inconsistent.

Addressing the refutation

The refutation correctly points out that in the common FRI lifecycle (a user index U still exists alongside FRI), the fallback would also return true via has_user_indices, so the visible behavior is identical. That argument is sound for that scenario.

The bug still manifests in narrower-but-realistic cases:

1. User dropped their index, FRI lingers. drop_index (rust/lance/src/index.rs) only removes the named user index; FRAG_REUSE_INDEX is not touched and persists with its old fragment_bitmap. On the next compaction, bins whose fragments fall inside FRI's bitmap get bin.indices = [pos_of_FRI], has_indexed_fragments=Some(true), and needs_row_addr_capture short-circuits to true before reaching the has_user_indices fallback. The optimization the PR is delivering is silently bypassed.

2. defer_index_remap=false with FRI-tracked fragments. The fallback at line 1140-1142 returns Ok(false) only when has_indexed_fragments was Some(false). With the bug, system-index coverage sets it to Some(true) and we short-circuit to true before that branch.

In both cases the user-visible effect is a performance regression to the pre-PR baseline (capture row_addr even though no user index needs remapping). It is not a correctness bug, which is why I am filing this as a nit rather than normal.

Step-by-step proof (case 1)

1. Create a dataset with 6 fragments (1k rows each), unstable row IDs.
2. Create a scalar user index U on column i.
3. Run compact_files with defer_index_remap=true. This commits a FRAG_REUSE_INDEX whose fragment_bitmap covers the newly written compacted fragment IDs (see build_frag_reuse_index_metadata in rust/lance/src/index/frag_reuse.rs).
4. Drop the user index U (the test would use Dataset::drop_index). FRAG_REUSE_INDEX remains.
5. Append more rows so there are new small fragments alongside the compacted ones from step 3.
6. Call plan_compaction and inspect a bin whose fragments are in FRI's bitmap. bin.indices will contain the FRI position, so the planned TaskData.has_indexed_fragments == Some(true).
7. Call rewrite_files on that task. needs_row_addr_capture short-circuits at line 1136-1138 and returns true without consulting has_user_indices. The expensive _rowaddr capture runs even though there is no user index in the dataset to remap.

Compare with the fix: filtering bin.indices by !is_system_index would make bin.indices empty, has_indexed_fragments=Some(false), and then the fallback at line 1144-1148 correctly returns Ok(false) (no user indices) — no capture.

How to fix

When computing index_fragmaps (or indices_containing_frag), filter out system indices, e.g.:

async fn load_index_fragmaps(dataset: &Dataset) -> Result<Vec<RoaringBitmap>> {
    let indices = dataset.load_indices().await?;
    let mut index_fragmaps = Vec::with_capacity(indices.len());
    for index in indices.iter() {
        if is_system_index(index) {
            index_fragmaps.push(RoaringBitmap::new());
            continue;
        }
        // ... existing logic
    }
    Ok(index_fragmaps)
}

(Or filter inside indices_containing_frag / when building bin.indices.) That way the line-708 check and the line-670 bin.indices == indices bin-splitting check both ignore system-index coverage, matching the user-index filter at line 1148. The line-670 fix is a small secondary win: bins that differ only in FRI coverage are no longer needlessly split.

Severity

Filed as nit. The bug is real and the inconsistency is worth fixing, but the common code path covered by the PR (user index alongside FRI) is unaffected because the has_user_indices fallback already returns true. Impact is bounded to the lingering-FRI-without-user-index scenario and the defer_index_remap=false edge case, and even then behavior degrades to the pre-PR baseline rather than producing incorrect results.

[yanghua]

Left some comments.

[yanghua]

If the case is defer_index_remap == false and task.has_indexed_fragments == fase, here we actively return false, but the task comes from plan phase. The task.has_indexed_fragments == fase may be old or not freshness right? For example, someone creates a index before current mement but after planning. Shall we re-compute the has_indexed_fragments?

[yanghua]

Here, we may also meet the consistent issue like before. We could add an assertion in the end, like this:

else if options.defer_index_remap {
    debug_assert!(
        task.has_indexed_fragments != Some(true),
        "defer_index_remap task with indexed fragments must carry row_addrs"
    );
}

[zhangyue19921010]

replaced by #7773