Make RowValues::compare's current-offset assumption self-checking
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
### Is your feature request related to a problem or challenge?
`RowValues::compare` (added in #23990) caches the current row's `(ptr, len)` and compares the cached slices, ignoring the `l_idx` / `r_idx` arguments:
```rust
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering {
debug_assert!(l_idx < l.len && r_idx < r.len);
let _ = (l_idx, r_idx);
l.current_slice().cmp(r.current_slice())
}
```
This is correct today: the only path that reaches it is `Cursor::cmp`, which always passes `self.offset` / `other.offset`, and `ArrayValues` (the wrapper that forwards arbitrary indices) is only ever built over `CursorArray::Values`, never over `RowValues`.
But the narrowing is unenforced. The trait documents `compare` as "Returns comparison of `l[l_idx]` and `r[r_idx]`", and the `debug_assert` above only checks the indices are in bounds — not that they equal the cached offset. A future caller passing arbitrary indices (a loser-tree change, a new wrapper around `RowValues`, …) would silently produce a wrong merge order with nothing to catch it, in debug or release.
### Describe the solution you'd like
Make the invariant self-checking, e.g. keep the offset in `RowValues` under `#[cfg(debug_assertions)]`, record it in `set_offset`, and assert `l_idx == l.current_offset` in `compare`. Zero cost in release. Also state at the impl that this implementation deliberately narrows the trait contract to current-offset comparisons.
### Describe alternatives you've considered
Falling back to `rows.row(l_idx)` when the indices don't match the cached offset would keep the trait contract intact, but it adds a branch to the merge hot path — the exact path #23990 set out to speed up.
### Additional context
Found while reviewing #23990 before merge; not a bug today, purely defence against future callers.
Contributor guide
Assessment
This issue has not been assessed yet.