zhuqi-lucas opened a new issue, #25097:
URL: https://github.com/apache/datafusion/issues/25097
### 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<T>` (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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
