nathanb9 commented on code in PR #25643:
URL: https://github.com/apache/datafusion/pull/25643#discussion_r4110133509
##########
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:
Added a comment
##########
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:
I've added this explanation to the comment block above the asserts
##########
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:
I've expanded the field's doc comment to spell it out: the
`debug_assert_eq!` would compile away on its own, but the field itself would
still widen `Cursor<RowValues>` by eight bytes in release, and that struct is
what the loser tree shuffles on every comparison. The cfg keeps the release
layout byte-identical.
I'll mention it in the description as well.
--
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]
