feat(rowids): validate stable row ids in Dataset::validate

[wjones127]

Dataset::validate() checked fragment ids, fragment ordering, per-fragment file lengths, and index metadata, but nothing about stable row ids. Historically stable row id bugs have escaped at the interaction of features — deletions crossed with updates, compaction, and indices — and none of those invariants had a cheap assertion to hang a test off.

On a dataset that uses stable row ids, validate() now also checks that:

• every fragment has row id metadata, and its sequence holds exactly physical_rows ids
• every live row id is unique across the whole dataset
• manifest.next_row_id is past every id already in use, tombstoned slots included, so no id is handed out twice
• row version metadata (created_at / last_updated_at), when present, covers the same number of rows as the fragment

Only live ids have to be unique. An update rewrites a row into a new fragment under the same id and tombstones the original slot, so the same id legitimately appears in two fragments' sequences — one live, one deleted. The check skips deleted slots for that reason, and the new test asserts that situation really does arise after an update rather than just documenting it.

The payoff is reusable invariant checking for write-path tests: sequencing appends, deletes, updates, merge inserts, and compactions and calling validate() after each now catches alignment regressions without hand-written assertions. One such test is included.

Fixes #8247

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The validation belongs at the right dataset-wide boundary, but the cardinality check must not trust malformed compressed metadata. Please enforce RangeWithHoles invariants during decoding and cover that case so validation cannot accept an ID for a nonexistent physical slot.

[lance-gatekeeper]

✅ Gate recommendation: approve.

The iterator-count check and end-to-end regression close the prior cardinality false negative while preserving the dataset-wide validation design. The stated stable-row-ID invariants are now supported across the covered write operations.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

Stable-row-ID validation must be total for every sequence accepted by read_row_ids. The new checks expose two independent panic paths—unchecked aggregate length and bounds lookup on empty decoded segments—so malformed metadata can unwind instead of producing a corruption result.

Please make aggregate cardinality checked and empty-segment bounds safe (or reject those encodings during decoding), with regressions for both cases.

[lance-gatekeeper]

❌ Gate recommendation: request changes.

The checked aggregate length and empty-array changes resolve those two prior reproductions. The bounds fix is incomplete, though: two other decoded empty segment variants still hit the same end - 1 path and panic.

Apply the empty-range contract across every range-backed representation and cover both accepted encodings.

[lance-gatekeeper]

range() is still not total for every decoded empty segment. TryFrom accepts zero-length RangeWithHoles with no holes and zero-length RangeWithBitmap with an empty bitmap, but both remain in the arm that computes range.end - 1. Appending either to Range(10..20) preserves a ten-ID sequence, yet Dataset::validate() panics here in debug builds; release builds wrap the bound to u64::MAX and falsely reject the valid allocator state. Extend the empty-range guard to both sparse range variants (or reject those zero-length encodings) and cover each case.

Reproducer

I temporarily added end-to-end tests equivalent to:

let empty_holes = pb::u64_segment::Segment::RangeWithHoles(
    pb::u64_segment::RangeWithHoles {
        start: 0,
        end: 0,
        holes: Some(pb::EncodedU64Array {
            array: Some(pb::encoded_u64_array::Array::U64Array(
                pb::encoded_u64_array::U64Array { values: Vec::new() },
            )),
        }),
    },
);
let empty_bitmap = pb::u64_segment::Segment::RangeWithBitmap(
    pb::u64_segment::RangeWithBitmap {
        start: 0,
        end: 0,
        bitmap: Vec::new(),
    },
);
for empty in [empty_holes, empty_bitmap] {
    let temp_dir = lance_core::utils::tempfile::TempStrDir::default();
    let mut dataset = validation_fixture(&temp_dir).await;
    let encoded = encode_segments(vec![range_segment(10, 20), empty]);
    edit_fragments(&mut dataset, |fragments| {
        fragments[1].row_id_meta = Some(RowIdMeta::Inline(encoded));
    });
    dataset.validate().await.unwrap();
}

I ran each case with its exact test filter. Expected: successful validation. Both observed: panic attempt to subtract with overflow at segment.rs:290.

[wjones127]

Correct, my fix was incomplete — the if range.is_empty() guard only bound the Self::Range pattern, so the two sparse variants still fell through to end - 1. Fixed properly in ffdbb87: the guard now applies to all three range-backed variants via (!range.is_empty()).then(|| range.start..=(range.end - 1)).

I verified both directions rather than assuming. Against the incomplete guard, the range_with_holes and range_with_bitmap cases panic at attempt to subtract with overflow in segment.rs, and the array cases pass; with the guard applied to all three, all four pass.

Coverage is now parameterized over every empty encoding rather than the single array case I had: test_validate_tolerates_empty_segment is an rstest with array, sorted_array, range_with_holes, and range_with_bitmap cases, and test_empty_segments_have_no_range in segment.rs asserts range() == None for all five constructors directly.

I kept zero-length encodings accepted rather than rejecting them at decode, since they are legal and round-trip: from_stats_and_sequence emits Range(0..0) for an empty input, so refusing them would reject metadata the writer itself can produce.

[lance-gatekeeper]

✅ Gate recommendation: approve.

The shared empty-range guard now covers every range-backed row-ID representation, and the parameterized validator regressions exercise both previously failing encodings. Together with the checked aggregate length and existing invariant tests, this resolves the remaining panic paths without changing the dataset-wide validation design.

[hamersaw]

Looks great! Just one clarification on validating the aggregate segment lengths.

[hamersaw]

"but the total need not fit a u64". I'm having some difficulty understanding this, because this is checking that the total fits in a u64.

[hamersaw]

Also, I think that this check only occurs on read. So technically we can write a pb.RowIdSequence that then fails to read. I did a small check that write_row_ids succeeds, but then read_row_ids fails and seq.len() panics. Should we have this check on the write-side so that we don't write a non-readable segment?

[wjones127]

It's a good point. We should check on the write side as well. But I think a read guard is still important.