zhuqi-lucas commented on code in PR #25643:
URL: https://github.com/apache/datafusion/pull/25643#discussion_r4100414630
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -192,6 +192,12 @@ pub struct RowValues {
/// Cached byte length for the current row.
current_len: usize,
+ /// The offset the cache was last refreshed for. Debug builds only: used
+ /// by [`CursorValues::compare`] to assert callers only compare at the
+ /// current offset (the narrowed contract this impl relies on).
+ #[cfg(debug_assertions)]
+ current_offset: usize,
Review Comment:
Worth saying in the PR description that gating the field on
`debug_assertions` is deliberate rather than tidiness. `Cursor<RowValues>` is
the struct the loser tree shuffles on every comparison, so eight extra bytes is
the kind of thing that can land on the wrong side of a cache line. Keeping the
release layout byte-identical is the reason to cfg a field that a
`debug_assert_eq!` would already compile away on its own.
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -228,6 +234,8 @@ impl RowValues {
len,
current_ptr,
current_len,
+ #[cfg(debug_assertions)]
+ current_offset: 0,
Review Comment:
This initialiser now has to agree with something about forty lines away in a
different impl: `Cursor::new` builds `Self { offset: 0, values }` and never
calls `set_offset`, so it relies entirely on the constructor having seeded the
cache for row 0. That was already true of `current_ptr`/`current_len`; this
adds a third field to the same silent contract.
A line here saying the three fields must describe the same row, and that
`Cursor::new` depends on that row being 0, would help. If someone later
reorders the constructor, the assert starts firing from a call site that has
nothing to do with the mistake.
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -267,9 +280,21 @@ impl CursorValues for RowValues {
}
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering {
- // Merge callers always compare at current offsets; the cache is up
- // to date. (Debug-only: verify the invariant.)
- debug_assert!(l_idx < l.len && r_idx < r.len);
+ // Narrowed contract (see impl docs): merge callers always compare at
+ // the current offsets, so the cached slices are the requested rows.
+ // Debug builds verify the indices match the offsets the cache was
+ // refreshed for; zero cost in release.
+ #[cfg(debug_assertions)]
+ {
+ debug_assert_eq!(
+ l_idx, l.current_offset,
Review Comment:
One case worth confirming is intentional: `advance()` skips `set_offset`
once the cursor runs off the end, so a finished cursor sits at `offset == len`
with `current_offset == len - 1`, and this assert would fire on it.
That is not a regression — the previous `debug_assert!(l_idx < l.len)` would
have fired on the same state — and the comment in `advance()` already says a
finished cursor's stale cache is never read. Just worth a word in the
description so a reviewer meeting the assert does not have to rediscover that
the guard is upheld one layer up.
--
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]
