apache / apache/datafusion

Make RowValues::compare's current-offset assumption self-checking

Open
#25,097 1 comment 0 reactions 1 assignee Claimed by @nathanb9 View on GitHub
enhancement
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.