fix(rowids): reject duplicate values in a SortedArray segment - #9245
Merged
wjones127 merged 1 commit intoSep 17, 2026
Merged
Conversation
`U64Segment` is documented as "a sequence of distinct u64s", and the write path rejects repeats in `RowIdSequence::try_from_iter`, but the read path did not. Deserializing a `RangeWithHoles` segment used `first_non_increasing_pair`, which rejects equal neighbours, while `SortedArray` used a separate `first_descending_pair` that only rejected strictly descending ones. lance-format#8259 added the stricter predicate to close exactly this class of gap -- its root cause note reads "Corrupt metadata could therefore panic or silently report an incorrect length" -- and wired it into one arm of the match only. A repeated id is representable on disk: the encoder picks `SortedArray` whenever it is smaller than a bitmap, so a sparse sequence encodes that way and survives the protobuf round trip. `SegmentStats::n_holes` then derives the hole count by subtracting the value count from the slot span, which underflows once a value repeats -- a panic where overflow checks are on, and a wildly wrong hole count where they are not, which drops live ids from the rebuilt segment instead of reporting a corrupt file. Use the strict predicate for both arms and drop the weaker duplicate, so the two cannot diverge again. The message follows the `RangeWithHoles` wording; it now reports decoded values rather than raw offsets, so the assertion matches on the prefix. ## Testing `cargo test --release -p lance-table` (399 passed). Extended `test_rejects_unsorted_sorted_array` with a duplicate case per encoding width; restoring the strict-descending predicate fails those three plus the existing `RangeWithHoles` duplicate case. Also `cargo fmt --all -- --check` and `cargo clippy --release -p lance-table --all-features --all-targets -- -D warnings`.
![lance-gatekeeper[bot]](https://avatars.githubusercontent.com/in/4433141?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Gate recommendation: approve.
This closes the gap in SortedArray metadata validation: equal neighbours are rejected as corrupt for all three encoded widths, matching the existing strict-order contract used for RangeWithHoles. The focused regression coverage exercises both descending and duplicate inputs.
lance-gatekeeper
Bot
added
the
K-approved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
U64Segmentis documented as "a sequence of distinct u64s", andRowIdSequence::try_from_iterrejects repeats on the write path, but the read path did not.RangeWithHolesdeserialization usedfirst_non_increasing_pair, which rejects equal neighbours;SortedArrayused a separatefirst_descending_pairthat only rejected descending ones. #8259 added the stricter predicate and wired it into one arm of the match only.A repeated id is representable on disk, since the encoder picks
SortedArraywhenever it beats a bitmap.SegmentStats::n_holesthen subtracts the value count from the slot span, which underflows once a value repeats: a panic where overflow checks are on, and otherwise a hole count large enough to drop live ids instead of reporting a corrupt file.Use the strict predicate in both arms and drop the weaker duplicate.
Testing
cargo test --release -p lance-table(399 passed), pluscargo fmtand clippy with-D warnings.test_rejects_unsorted_sorted_arraygains a duplicate case per width; restoring the old predicate fails those three and the existingRangeWithHolesduplicate case.