From bb64be1817e196f3b97d52302e96ff75eb5e4c8a Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Fri, 11 Sep 2026 12:57:50 +0800 Subject: [PATCH 1/2] fix(lance-table): ignore trailing empty segments in rechunk_sequences MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- rust/lance-table/src/rowids.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/rust/lance-table/src/rowids.rs b/rust/lance-table/src/rowids.rs index 4cadaf67b76..75ee4aede70 100644 --- a/rust/lance-table/src/rowids.rs +++ b/rust/lance-table/src/rowids.rs @@ -897,6 +897,16 @@ pub fn rechunk_sequences( chunked_sequences.push(sequence); } + // The fill loop only drains empty segments while a chunk still needs + // rows, so a trailing empty segment (e.g. one a full-segment `delete` + // left behind) is still sitting in the iterator here. It carries no ids + // and must not be reported as excess. + while segment_iter + .peek() + .is_some_and(|segment| segment.is_empty()) + { + segment_iter.next(); + } if segment_iter.peek().is_some() { return Err(too_many_segments_error( chunked_sequences.len(), @@ -1797,6 +1807,21 @@ mod test { let elements: Vec = result[0].iter().collect(); assert_eq!(elements, vec![0, 1]); + + // trailing empty segment: the final chunk is satisfied, so the fill + // loop exits without draining the iterator. A trailing empty segment + // carries no ids and must not be reported as excess. + let input_sequences = vec![ + RowIdSequence::from(0..2), // [0, 1] - 2 elements + RowIdSequence::from(10..10), // [] - 0 elements (trailing empty) + ]; + let chunk_sizes = vec![2]; + let result = rechunk_sequences(input_sequences, chunk_sizes, false).unwrap(); + + assert_eq!(result.len(), 1); + assert_eq!(result[0].len(), 2); + let elements: Vec = result[0].iter().collect(); + assert_eq!(elements, vec![0, 1]); } #[test] From 49db12e4ee0c5b81ebeb445133af13290c60bb2b Mon Sep 17 00:00:00 2001 From: yangjie01 Date: Wed, 16 Sep 2026 23:33:40 +0800 Subject: [PATCH 2/2] fix(lance-table): report excess only for segments that still hold ids --- rust/lance-table/src/rowids.rs | 45 +++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/rust/lance-table/src/rowids.rs b/rust/lance-table/src/rowids.rs index 75ee4aede70..8527746a604 100644 --- a/rust/lance-table/src/rowids.rs +++ b/rust/lance-table/src/rowids.rs @@ -897,17 +897,11 @@ pub fn rechunk_sequences( chunked_sequences.push(sequence); } - // The fill loop only drains empty segments while a chunk still needs - // rows, so a trailing empty segment (e.g. one a full-segment `delete` - // left behind) is still sitting in the iterator here. It carries no ids - // and must not be reported as excess. - while segment_iter - .peek() - .is_some_and(|segment| segment.is_empty()) - { - segment_iter.next(); - } - if segment_iter.peek().is_some() { + // The fill loop drains empty segments only while a chunk still needs rows, + // so once the last chunk is satisfied any trailing empty segment is still + // sitting in the iterator. Those carry no ids; a segment the last chunk + // only partially consumed is not empty, so it is still excess. + if segment_iter.any(|segment| !segment.is_empty()) { return Err(too_many_segments_error( chunked_sequences.len(), total_chunks, @@ -1808,12 +1802,13 @@ mod test { let elements: Vec = result[0].iter().collect(); assert_eq!(elements, vec![0, 1]); - // trailing empty segment: the final chunk is satisfied, so the fill - // loop exits without draining the iterator. A trailing empty segment - // carries no ids and must not be reported as excess. + // trailing empty segments: the final chunk is satisfied, so the fill + // loop exits without draining the iterator. They carry no ids and must + // not be reported as excess. let input_sequences = vec![ RowIdSequence::from(0..2), // [0, 1] - 2 elements RowIdSequence::from(10..10), // [] - 0 elements (trailing empty) + RowIdSequence::from(20..20), // [] - 0 elements (trailing empty) ]; let chunk_sizes = vec![2]; let result = rechunk_sequences(input_sequences, chunk_sizes, false).unwrap(); @@ -1822,6 +1817,28 @@ mod test { assert_eq!(result[0].len(), 2); let elements: Vec = result[0].iter().collect(); assert_eq!(elements, vec![0, 1]); + + // a segment with ids behind the trailing empties is still excess, and + // so is a segment the last chunk only partially consumed. + let input_sequences = vec![ + RowIdSequence::from(0..2), + RowIdSequence::from(10..10), + RowIdSequence::from(20..21), + RowIdSequence::from(30..30), + ]; + let err = rechunk_sequences(input_sequences, vec![2], false).unwrap_err(); + assert!(matches!(err, Error::InvalidInput { .. })); + assert!( + err.to_string().contains("too many segments"), + "leftover ids should still be reported as excess, got: {err}" + ); + + let err = rechunk_sequences(vec![RowIdSequence::from(0..3)], vec![2], false).unwrap_err(); + assert!(matches!(err, Error::InvalidInput { .. })); + assert!( + err.to_string().contains("too many segments"), + "a partially consumed segment still holds ids, got: {err}" + ); } #[test]