Copilot commented on code in PR #23840:
URL: https://github.com/apache/datafusion/pull/23840#discussion_r3645027036
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -164,16 +164,35 @@ impl<T: CursorValues> Ord for Cursor<T> {
/// Implements [`CursorValues`] for [`Rows`]
///
-/// Used for sorting when there are multiple columns in the sort key
+/// Used for sorting when there are multiple columns in the sort key.
+///
+/// Caches `(ptr, len)` for the current row's serialized bytes so the merge hot
+/// path compares two `&[u8]` slices directly rather than paying a
+/// `Rows::row(idx)` offset lookup for each side of each compare. The pointer
+/// is into `rows`'s Arc-owned buffer heap, so it stays valid even when this
+/// struct is moved (e.g. written into a `Vec<Option<Cursor<..>>>` slot).
#[derive(Debug)]
pub struct RowValues {
rows: Arc<Rows>,
+ /// Number of rows — snapshot of `rows.num_rows()`. Read on every
+ /// `Cursor::is_finished` / `advance` call.
+ len: usize,
+ /// Cached byte slice pointer for the current row.
+ current_ptr: *const u8,
+ /// Cached byte length for the current row.
+ current_len: usize,
+
/// Tracks for the memory used by in the `Rows` of this
/// cursor. Freed on drop
_reservation: MemoryReservation,
}
+// SAFETY: `current_ptr` points into `rows`'s Arc-owned buffer heap. `Rows`
+// is `Send + Sync`; the referenced bytes are read-only after construction.
+unsafe impl Send for RowValues {}
+unsafe impl Sync for RowValues {}
Review Comment:
The `unsafe impl Send/Sync` here is redundant: `RowValues` already satisfies
`CursorValues: Send + Sync` via its fields, and keeping a manual `unsafe impl`
makes future refactors riskier (a newly added `!Send/!Sync` field would
silently become unsound). Prefer relying on the auto-trait derivation and keep
only the safety rationale as a comment if desired.
This issue also appears on line 518 of the same file.
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -213,7 +253,26 @@ impl CursorValues for RowValues {
}
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering {
- l.rows.row(l_idx).cmp(&r.rows.row(r_idx))
+ // 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);
+ let _ = (l_idx, r_idx);
+ l.current_slice().cmp(r.current_slice())
+ }
Review Comment:
`RowValues::compare` ignores `l_idx`/`r_idx` and always compares the cached
current-row slices. The comment says debug builds verify the cache invariant,
but the current `debug_assert!` only checks bounds, so misuse (calling
`compare` with a non-current index) can silently produce wrong ordering in
debug builds too. Strengthen the debug-only check to validate the cache matches
`rows.row(l_idx)` / `rows.row(r_idx)`.
##########
datafusion/physical-plan/src/sorts/cursor.rs:
##########
@@ -688,4 +980,219 @@ mod tests {
b.advance();
assert_eq!(a.cmp(&b), Ordering::Less);
}
+
+ fn new_byte_array(
+ options: SortOptions,
+ strings: &[&str],
+ ) -> Cursor<ArrayValues<ByteArrayValues<i32>>> {
+ use arrow::array::StringArray;
+ let array = StringArray::from(strings.to_vec());
+ let byte_values = <StringArray as CursorArray>::values(&array);
+ let memory_pool: Arc<dyn MemoryPool> =
Arc::new(GreedyMemoryPool::new(10000));
+ let consumer = MemoryConsumer::new("test");
+ let reservation = consumer.register(&memory_pool);
+ let values = ArrayValues {
+ values: byte_values,
+ null_threshold: strings.len(),
+ options,
+ has_nulls: false,
+ _reservation: reservation,
Review Comment:
`new_byte_array` hard-codes `null_threshold: strings.len()`, which only
matches the `has_nulls: false` invariant when `options.nulls_first` is `false`.
If this helper is used with `nulls_first: true`, `ArrayValues::is_null` will
treat every row as NULL and tests will behave incorrectly. Set `null_threshold`
based on `options.nulls_first` when there are no nulls.
This issue also appears on line 1081 of the same file.
--
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]
