[LuQQiu]

Stacked on #10409 (the branch contains its commits); the incremental delta is one commit 4b89c9f8a in rust/lance/src/dataset/optimize.rs.

What

DefaultCompactionPlanner::plan_fragments(&self, dataset, fragments: Vec<FileFragment>): plan over the given fragments, in the order given (ascending ids), as if they were adjacent. A fragment of the table that is not in the list is not a boundary between its neighbours in the list; an excluded fragment (excluded_fragment_ids) that is in the list still ends a bin, as it does today. CompactionPlanner::plan for the default planner is now plan_fragments(dataset, dataset.get_fragments()) and is unchanged in behavior.

Why

A planner that compacts groups of fragments sharing a property (for example the outputs of one clustering bucket, written by several passes and therefore interleaved in the manifest with other buckets' outputs) has today only excluded_fragment_ids to confine the default planner to a group. Exclusions are boundaries, so the group's fragments are never binned together across the fragments between them, and small per-pass tails never merge. With plan_fragments the caller passes the group's members and keeps its own exclusions as boundaries; both kinds of exclusion keep their meaning.

No new option, no manifest or file format change, no policy about which fragments form a group.

Test

plan_fragments_treats_the_listed_fragments_as_adjacent: six 100-row fragments. Listing [0, 2, 4] plans one task over exactly those three; the same list with excluded_fragment_ids: [2] plans nothing (0 and 4 are separated by the boundary); plan over the whole table still plans one task over all six.

🤖 Generated with Claude Code

[LuQQiu]

@claude review

[claude]

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🔴 rust/lance/src/dataset/optimize.rs — With plan_fragments over a subset, the max_source_fragments limit and the excluded-fragment suffix are worked out wrongly, so plans come back empty or relabel excluded fragments. i/pos_range count positions in the caller's list. budget_suffix_start uses dataset.manifest.fragments.len(), and exclusion_suffix_start looks only at excluded fragments that are in the list. Fix: compute both limits from each listed fragment's position in the manifest. Consider every excluded fragment in the manifest, not only the listed ones. plan must keep its current behaviour.

  Why this was flagged

  Case 1: a table has 1000 fragments, max_source_fragments: Some(100), and a caller passes a 50-fragment group to plan_fragments. budget_suffix_start is 1000-100=900 (optimize.rs:1100-1102). Every bin's pos_range.end is at most 50, so the filter at optimize.rs:1108 drops all of them and the plan is empty, even when the group sits at the end of the manifest. Case 2: excluded_fragment_ids holds fragment 3 and the list is [0,2,4,6]. The rposition at optimize.rs:987-993 never sees fragment 3, so the planner does not limit itself to the suffix after it. Once the planned rewrite commits, complete_rewrite_suffix relabels every fragment after the first rewritten position, including fragment 3. That breaks the stated rule that "excluded identities stay unchanged". Through plan, list positions equal manifest positions, so the base branch is correct.

  Verification: It triggers when a caller of the new public DefaultCompactionPlanner::plan_fragments passes a strict subset of the table's fragments and sets max_source_fragments. The budget limit is taken from the whole manifest, not the list (optimize.rs:1100-1102). 1000 fragments, a limit of 100 and a 50-fragment list give suffix_start = 900, so every bin is dropped and the plan is empty.

[claude]

🔴 Callers of the new plan_fragments get plans that can never commit when the listed fragments are not adjacent in the manifest, which is the case it exists for. commit_compaction always runs complete_rewrite_suffix (optimize.rs:4198), which needs each task's sources to be a contiguous manifest range. A task over [0, 2, 4] fails with "compaction source fragments must be contiguous and in manifest order" at optimize.rs:2963. Fix: either teach commit to accept gapped source groups (with ordering and relabelling of the fragments in between), or have plan_fragments split bins at manifest gaps. Add a test that runs and commits such a plan.

Why this was flagged

The trigger is a caller planning a clustering bucket whose fragments are interleaved with other buckets' fragments, e.g. plan_fragments(&ds, [0,2,4]), then running commit_compaction or compact_files_with_planner on the plan. The planner (optimize.rs:1026-1093) bins 0, 2 and 4 into one task because a fragment missing from the list no longer ends a bin. At commit, complete_rewrite_suffix (optimize.rs:2941-2966) finds the first source's manifest position and compares original_fragments with manifest_fragments[start..end]. It hits fragment 1 where it expects 2 and returns Error::invalid_input. The rewrite work is done and then thrown away, and the dataset never compacts. On the base branch this cannot happen because plan only produced contiguous bins. The new test plan_fragments_treats_the_listed_fragments_as_adjacent only checks the plan and never executes or commits it, so it misses this.

Verification: The new test (lines 4933-4938) asserts a single task over [0, 2, 4]. commit_compaction always calls complete_rewrite_suffix(&dataset.manifest.fragments, completed_tasks, ...) (lines 4198-4204). For [0, 2, 4] the slice is [0, 1, 2], so commit fails with "expected fragment 2 but found 1". The new test only plans and never commits, so it does not catch this.

[LuQQiu]

Acknowledged, both findings are right. commit_compaction goes through complete_rewrite_suffix, which requires each task's sources to be a contiguous manifest range, so a plan over a gapped list cannot commit through compaction; and the suffix/budget limits are computed in list positions, which is wrong for a subset. Before changing the hook I am checking whether the use case needs it at all: the manifest builder already appends the outputs of a non-contiguous Rewrite group, and the clustering rewrite commits that way with a stable-partition reuse transition, so bucket-local merges may not need the compaction planner. If that holds, this Draft will be withdrawn rather than patched; otherwise the fix will compute both limits from manifest positions, validate the list (strictly increasing ids, otherwise InvalidInput) and split bins at manifest gaps with a committing test. No change pushed yet.

[LuQQiu]

@claude review

[claude]

Findings marked 🟡 are optional suggestions and need no follow-up push.

Still open from earlier reviews (1):

• 🔴 rust/lance/src/dataset/optimize.rs:921 — Callers of the new plan_fragments get plans that can never commit when the listed fragments are not adjacent in the man…

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

[claude]

🟡 (optional) Callers of the new public plan_fragments get no error in release builds when their list is unsorted, has duplicates, or holds another dataset's fragments. The only check is the debug_assert! at optimize.rs:948. Its message, "fragments in manifest are not sorted", blames the manifest, not the caller. Fix: check at entry that ids strictly ascend (which also rules out duplicates) and that every id is in dataset.manifest.fragments. Return Error::invalid_input naming the caller's list if not. plan passes dataset.get_fragments(), which already meets this, so it keeps working.

Why this was flagged

This only matters for external callers of the new public plan_fragments(dataset, fragments) (optimize.rs:917), because plan always passes the manifest's own sorted list. The trigger is a caller passing ids out of order or duplicated (e.g. [4, 2] or [2, 2]), or FileFragments taken from another version or dataset. In release builds debug_assert! (optimize.rs:948-951) compiles away. The planner then bins those fragments, computes pos_range and the exclusion suffix from list positions (optimize.rs:987-993, 1074), and returns a plan. That plan fails only later, after the rewrite I/O, or it reorders or duplicates rows if commit-time checks change. In debug builds the caller gets a panic saying "fragments in manifest are not sorted", which points at the manifest instead of their argument. On the base branch none of this can happen, because the list always came from dataset.get_fragments(). This is a separate fix from the prior commit-adjacency finding: it is input validation at the API entry, not a change to how commit checks contiguity.

Verification: The only check on the caller's list is the debug_assert! at optimize.rs:948-951, which compiles away in release. complete_rewrite_suffix (2941-2966) rejects most bad lists only at commit, after the rewrite I/O is done. With [5, 1] and excluded_fragment_ids=[5], the excluded fragment 5 is carried into the completed suffix (3004-3027).