Skip to content

fix(lance-table): treat trailing empty segments as no excess in rechunk_sequences - #9302

Open
LuciferYang wants to merge 2 commits into
lance-format:mainfrom
LuciferYang:fix/srid-rechunk-trailing-empty
Open

LuciferYang wants to merge 2 commits into
lance-format:mainfrom
LuciferYang:fix/srid-rechunk-trailing-empty

Conversation

@LuciferYang

Copy link
Copy Markdown
Contributor

Closes #9301

rechunk_sequences errors when segments are left over after the last chunk is filled. Its fill loop drains empty segments only while a chunk still needs rows, so a trailing empty segment survives to that check and a call whose chunk sizes sum to exactly the number of row ids fails with InvalidInput. The check now asks whether a leftover segment still carries ids.

Empty segments are already tolerated everywhere else in the function, with a test case for each position: inside a chunk, at a chunk boundary, and several in a row. Trailing was the one gap. The outcome changes only for inputs where every leftover segment is empty, including the degenerate case of an all-empty input against empty chunk_sizes; the check is not gated on allow_incomplete, so this applies in both modes. A segment the last chunk only partially consumed is not empty, so leftover ids are still reported.

No caller reaches this today. Production row removal goes through RowIdSequence::mask, which drops empty segments, and the sequences the four callers pass in come either from rechunk_sequences itself or from a capture stream whose scan nodes drop zero-row batches. The capture path has no zero-row guard of its own: extract_row_ids captures every batch it polls, and RowIdSequence::from(&[]) yields one empty segment, so the invariant lives in the upstream nodes rather than at the capture site.

How was this patch tested?

test_row_id_sequence_rechunk_with_empty_segments gains three cases: two trailing empty segments with the row counts matching exactly must produce one chunk of [0, 1]; a segment with ids behind a trailing empty must still be reported as excess; and so must a segment the last chunk only partially consumed. All were mutation-checked. With the old check the first case fails with "Got too many segments for the provided chunk lengths. Processed 1 chunks out of 1 expected"; with a check that never reports excess, the other two fail along with the pre-existing excess case in test_row_id_sequence_rechunk.

yangjie01 and others added 2 commits September 16, 2026 22:50
The fill loop drains empty segments only while a chunk still needs rows,
so once the last chunk is satisfied a trailing empty segment stays in the
iterator and the final too-many-segments check rejected the call even
though the total ids matched the chunk sizes exactly. A trailing empty
segment carries no ids — skip them before the excess check, matching how
the fill loop already treats interior and boundary empties.
@github-actions github-actions Bot added the bug Something isn't working label Sep 16, 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.

The terminal check now matches rechunk_sequences' row-count contract: leftover segments are rejected only when they still contain row IDs. The added regression cases cover consecutive trailing empties while confirming non-empty and partially consumed excess remains invalid.

@lance-gatekeeper lance-gatekeeper Bot added the K-approved Latest Gatekeeper recommendation permits acceptance. label Sep 16, 2026
@LuciferYang

Copy link
Copy Markdown
Contributor Author

The windows-build failure is unrelated to this change: it is the recall assertion in index::vector::ivf::v2::tests::test_legacy_ivf_pq_cosine_multivec_smoke (recall 0.48 against a 0.5 threshold on randomized multivec data). That test builds a legacy IVF-PQ index and never reaches rechunk_sequences, and windows-build has been green on recent main commits. Everything else on this PR is green.

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

Labels

bug Something isn't working K-approved Latest Gatekeeper recommendation permits acceptance.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: rechunk_sequences rejects a valid input when a sequence ends with an empty segment

2 participants