Skip to content

Perf: Optimize CursorValues compare performance for StringViewArray (1.4X faster for sort-tpch Q11) #16509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 24, 2025
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 25 additions & 3 deletions datafusion/physical-plan/src/sorts/cursor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -293,14 +293,19 @@ impl CursorValues for StringViewArray {
self.views().len()
}

#[inline(always)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For anyone else following along, I believe this is the same optimization as applied in

fn eq(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> bool {
// SAFETY: Both l_idx and r_idx are guaranteed to be within bounds,
// and any null-checks are handled in the outer layers.
// Fast path: Compare the lengths before full byte comparison.

let l_view = unsafe { l.views().get_unchecked(l_idx) };
let l_len = *l_view as u32;
let r_view = unsafe { r.views().get_unchecked(r_idx) };

if l.data_buffers().is_empty() && r.data_buffers().is_empty() {
return l_view.eq(r_view);
}

let l_len = *l_view as u32;
let r_len = *r_view as u32;
if l_len != r_len {
return false;
Expand All @@ -309,13 +314,19 @@ impl CursorValues for StringViewArray {
unsafe { GenericByteViewArray::compare_unchecked(l, l_idx, r, r_idx).is_eq() }
}

#[inline(always)]
fn eq_to_previous(cursor: &Self, idx: usize) -> bool {
// SAFETY: The caller guarantees that idx > 0 and the indices are valid.
// Already checked it in is_eq_to_prev_one function
// Fast path: Compare the lengths of the current and previous views.
let l_view = unsafe { cursor.views().get_unchecked(idx) };
let l_len = *l_view as u32;
let r_view = unsafe { cursor.views().get_unchecked(idx - 1) };
if cursor.data_buffers().is_empty() {
return l_view.eq(r_view);
}

let l_len = *l_view as u32;

let r_len = *r_view as u32;
if l_len != r_len {
return false;
Expand All @@ -326,10 +337,21 @@ impl CursorValues for StringViewArray {
}
}

#[inline(always)]
fn compare(l: &Self, l_idx: usize, r: &Self, r_idx: usize) -> Ordering {
// SAFETY: Prior assertions guarantee that l_idx and r_idx are valid indices.
// Null-checks are assumed to have been handled in the wrapper (e.g., ArrayValues).
// And the bound is checked in is_finished, it is safe to call get_unchecked
if l.data_buffers().is_empty() && r.data_buffers().is_empty() {
let l_view = unsafe { l.views().get_unchecked(l_idx) };
let r_view = unsafe { r.views().get_unchecked(r_idx) };
let l_len = *l_view as u32;
let r_len = *r_view as u32;
let l_data = unsafe { StringViewArray::inline_value(l_view, l_len as usize) };
let r_data = unsafe { StringViewArray::inline_value(r_view, r_len as usize) };
return l_data.cmp(r_data);
}

unsafe { GenericByteViewArray::compare_unchecked(l, l_idx, r, r_idx) }
}
}
Expand Down