Skip to content
Merged
Changes from 2 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
110 changes: 105 additions & 5 deletions arrow-ord/src/sort.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use arrow_array::types::*;
use arrow_array::*;
use arrow_buffer::ArrowNativeType;
use arrow_buffer::BooleanBufferBuilder;
use arrow_data::ArrayDataBuilder;
use arrow_data::{ArrayDataBuilder, ByteView, MAX_INLINE_VIEW_LEN};
use arrow_schema::{ArrowError, DataType};
use arrow_select::take::take;
use std::cmp::Ordering;
Expand Down Expand Up @@ -303,18 +303,118 @@ fn sort_bytes<T: ByteArrayType>(
sort_impl(options, &mut valids, &nulls, limit, Ord::cmp).into()
}

/// Builds a 128-bit composite key for an inline value:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This comment does a good job explaining what this function does but I think it would be good if we could explain the why it is useful a bit more specifically than "fast comparisons". Spcifically what property(s) the returned u128 has

Something like this

Suggested change
/// Builds a 128-bit composite key for an inline value:
/// Builds a 128-bit composite key for an inline value for fast sorting
///
/// The `u128` returned by this function compares the same comparing
/// the `str` value from an inlined view.

I am not quite sure is that is correct because if it is, I don't understand why it is copying the length bytes as well 🤔

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @alamb for review, good suggestion!

And the idea is coming from the comments here:

#7748 (comment)

May be we can get the based PR ready first, thanks a lot!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, sorry -- I missed the other one -- let's work on #7748 first

/// - High 96 bits: the inline data in big-endian byte order (for correct lexicographical sorting)
/// - Low 32 bits: the length in big-endian byte order (so shorter strings are always numerically smaller)
///
/// This function extracts the length and the 12-byte inline string data from the
/// raw little-endian u128 representation, converts them to big-endian ordering,
/// and packs them into a single u128 value suitable for fast comparisons.
///
/// # Note
/// The input `raw` is assumed to be in little-endian format with the following layout:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

here is some potentially useful ascii art:

                                                                                        
      StringView Format                                                                 
                                                                                        
                                                                                        
                                                                                        
                     ┌───────────────────────────────────────────┬──────────────┐       
                     │                   data                    │    length    │       
 Strings, len <= 12  │             (padded with \0)              │    (u32)     │       
      (Inline)       │                                           │              │       
                     └───────────────────────────────────────────┴──────────────┘       
                     127                                        31             0  bit   
                                                                                  offset
                                                                                        
                                                                                        
                                                                                        
                                                                                        
                     ┌──────────────┬─────────────┬──────────────┬──────────────┐       
                     │buffer offset │ buffer index│ data prefix  │    length    │       
 Strings, len > 12   │    (u32)     │    (u32)    │  (4 bytes)   │    (u32)     │       
      (Offset)       │              │             │              │              │       
                     └──────────────┴─────────────┴──────────────┴──────────────┘       
                     127            95            63             31            0  bit   
                                                                                  offset
                                                                                        

And the raw monodraw file:
stringview.zip

/// - bytes 0..4: length (u32)
/// - bytes 4..16: inline string data (padded with zeros if less than 12 bytes)
///
/// The output u128 key places the inline string data in the upper 96 bits (big-endian)
/// and the length in the lower 32 bits (big-endian).
#[inline(always)]
pub fn inline_key_fast(raw: u128) -> u128 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

before merging this PR, I think we should write some unit tests showing that the output u128 of this function do indeed compare the same as the corresponding &str representations (assuming I am not missing something)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see now you did this in #7748 -- I will continue the conversation there

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @alamb , i added some testing for another PR which is this PR based:

#7748

May be we can get it ready first, thanks a lot!

// Convert the raw u128 (little-endian) into bytes for manipulation
let raw_bytes = raw.to_le_bytes();

// Extract the length (first 4 bytes), convert to big-endian u32, and promote to u128
let len_le = &raw_bytes[0..4];
let len_be = u32::from_le_bytes(len_le.try_into().unwrap()).to_be() as u128;

// Extract the inline string bytes (next 12 bytes), place them into the lower 12 bytes of a 16-byte array,
// padding the upper 4 bytes with zero to form a little-endian u128 value
let mut inline_bytes = [0u8; 16];
inline_bytes[4..16].copy_from_slice(&raw_bytes[4..16]);

// Convert to big-endian to ensure correct lexical ordering
let inline_u128 = u128::from_le_bytes(inline_bytes).to_be();

// Shift right by 32 bits to discard the zero padding (upper 4 bytes),
// so that the inline string occupies the high 96 bits

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe makes sense to use a mask here instead of right / left shifting?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @Dandandan for review, it try to use mask replacing the right / left shifting, but the performance no change, it believe the optimizer do some optimize already:

sort string_view[10] to indices 2^12
                        time:   [62.344 µs 62.458 µs 62.603 µs]
                        change: [0.7065% −0.3295% +0.0918%] (p = 0.11 > 0.05)
                        No change in performance detected.
Found 11 outliers among 100 measurements (11.00%)
  2 (2.00%) low mild
  5 (5.00%) high mild
  4 (4.00%) high severe

sort string_view[10] nulls to indices 2^12
                        time:   [36.626 µs 36.673 µs 36.719 µs]
                        change: [0.4095% −0.1715% +0.0813%] (p = 0.17 > 0.05)
                        No change in performance detected.
Found 6 outliers among 100 measurements (6.00%)
  3 (3.00%) low mild
  3 (3.00%) high mild

sort string_view[0-400] to indices 2^12
                        time:   [55.812 µs 55.875 µs 55.939 µs]
                        change: [0.1893% +0.0245% +0.2323%] (p = 0.82 > 0.05)
                        No change in performance detected.
Found 5 outliers among 100 measurements (5.00%)
  1 (1.00%) low severe
  1 (1.00%) low mild
  2 (2.00%) high mild
  1 (1.00%) high severe

sort string_view[0-400] nulls to indices 2^12
                        time:   [32.269 µs 32.310 µs 32.351 µs]
                        change: [0.1251% +0.1156% +0.3441%] (p = 0.34 > 0.05)
                        No change in performance detected.
Found 3 outliers among 100 measurements (3.00%)
  1 (1.00%) low mild
  1 (1.00%) high mild
  1 (1.00%) high severe

sort string_view_inlined[0-12] to indices 2^12
                        time:   [59.737 µs 59.853 µs 60.040 µs]
                        change: [+0.1045% +0.3772% +0.6674%] (p = 0.01 < 0.05)
                        Change within noise threshold.
Found 10 outliers among 100 measurements (10.00%)
  1 (1.00%) low mild
  3 (3.00%) high mild
  6 (6.00%) high severe

sort string_view_inlined[0-12] nulls to indices 2^12
                        time:   [35.176 µs 35.237 µs 35.299 µs]
                        change: [0.0949% +0.2861% +0.6023%] (p = 0.12 > 0.05)
                        No change in performance detected.
Found 1 outliers among 100 measurements (1.00%)
  1 (1.00%) high mild

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is the applied patch for testing:

diff --git a/arrow-ord/src/sort.rs b/arrow-ord/src/sort.rs
index 8796907669..2b0518c704 100644
--- a/arrow-ord/src/sort.rs
+++ b/arrow-ord/src/sort.rs
@@ -320,27 +320,37 @@ fn sort_bytes<T: ByteArrayType>(
 /// and the length in the lower 32 bits (big-endian).
 #[inline(always)]
 pub fn inline_key_fast(raw: u128) -> u128 {
-    // Convert the raw u128 (little-endian) into bytes for manipulation
+    // Interpret the 128-bit input as little-endian bytes
     let raw_bytes = raw.to_le_bytes();
 
-    // Extract the length (first 4 bytes), convert to big-endian u32, and promote to u128
+    // --- Step 1: Extract and convert the length field --- //
+    // The first 4 bytes (little-endian) encode the string length.
+    // Read them as u32 in little-endian order, then convert to big-endian
+    // and promote to u128 so it can occupy the low 32 bits of the key.
     let len_le = &raw_bytes[0..4];
-    let len_be = u32::from_le_bytes(len_le.try_into().unwrap()).to_be() as u128;
+    let len_be = u32::from_le_bytes(len_le.try_into().unwrap())
+        .to_be() as u128;
 
-    // Extract the inline string bytes (next 12 bytes), place them into the lower 12 bytes of a 16-byte array,
-    // padding the upper 4 bytes with zero to form a little-endian u128 value
+    // --- Step 2: Extract the inlined string bytes --- //
+    // Bytes 4..16 contain up to 12 bytes of the inline string.
+    // Copy them into bytes 4..16 of a 16-byte buffer, leaving top 4 bytes zero.
     let mut inline_bytes = [0u8; 16];
     inline_bytes[4..16].copy_from_slice(&raw_bytes[4..16]);
 
-    // Convert to big-endian to ensure correct lexical ordering
-    let inline_u128 = u128::from_le_bytes(inline_bytes).to_be();
+    // Convert that buffer to a big-endian u128 so that
+    // byte-wise lexical order corresponds to numeric order.
+    let inline_be = u128::from_le_bytes(inline_bytes).to_be();
 
-    // Shift right by 32 bits to discard the zero padding (upper 4 bytes),
-    // so that the inline string occupies the high 96 bits
-    let inline_part = inline_u128 >> 32;
+    // --- Step 3: Mask out the padding and isolate the inline portion --- //
+    // Define a mask that selects bits 32..128 (the 12 inlined bytes in BE)
+    // and zeroes out the high and low 32 bits.
+    const INLINE_MASK: u128 = 0x00FF_FFFF_FFFF_FFFF_FFFF_FFFF_0000_0000u128;
+    let inline_part = inline_be & INLINE_MASK;
 
-    // Combine the inline string part (high 96 bits) and length (low 32 bits) into the final key
-    (inline_part << 32) | len_be
+    // --- Step 4: Combine masked inline portion with length --- //
+    // The inline bytes already occupy bits 32..128 after masking.
+    // OR in the big-endian length (bits 0..32) to form the final key.
+    inline_part | len_be
 }
 
 fn sort_byte_view<T: ByteViewType>(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ah I missed that it was reused, let's merge that one first then and reuse it here!

let inline_part = inline_u128 >> 32;

// Combine the inline string part (high 96 bits) and length (low 32 bits) into the final key
(inline_part << 32) | len_be
}

fn sort_byte_view<T: ByteViewType>(
values: &GenericByteViewArray<T>,
value_indices: Vec<u32>,
nulls: Vec<u32>,
options: SortOptions,
limit: Option<usize>,
) -> UInt32Array {
let mut valids = value_indices
// 1. Build a list of (index, raw_view, length)
let mut valids: Vec<_> = value_indices
.into_iter()
.map(|index| (index, values.value(index as usize).as_ref()))
.collect::<Vec<(u32, &[u8])>>();
sort_impl(options, &mut valids, &nulls, limit, Ord::cmp).into()
.map(|idx| {
// SAFETY: we know idx < values.len()
let raw = unsafe { *values.views().get_unchecked(idx as usize) };
let len = raw as u32; // lower 32 bits encode length
(idx, raw, len)
})
.collect();

// 2. Compute the number of non-null entries to partially sort
let vlimit = match (limit, options.nulls_first) {
(Some(l), true) => l.saturating_sub(nulls.len()).min(valids.len()),
_ => valids.len(),
};

// 3. Mixed comparator: first prefix, then inline vs full comparison
let cmp_mixed = |a: &(u32, u128, u32), b: &(u32, u128, u32)| {
let (_, raw_a, len_a) = *a;
let (_, raw_b, len_b) = *b;

// 3.1 Both inline (≤12 bytes): compare full 128-bit key including length
if len_a <= MAX_INLINE_VIEW_LEN && len_b <= MAX_INLINE_VIEW_LEN {
return inline_key_fast(raw_a).cmp(&inline_key_fast(raw_b));
}

// 3.2 Compare 4-byte prefix in big-endian order
let pref_a = ByteView::from(raw_a).prefix.swap_bytes();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think ByteView can only be used if the view len is greater than 12:

/// Helper to access views of [`GenericByteViewArray`] (`StringViewArray` and
/// `BinaryViewArray`) where the length is greater than 12 bytes.

Isn't this potentially comparing a inline view and a non inline view?

@zhuqi-lucas zhuqi-lucas Jun 27, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you @alamb for review, this actually copy from another PR:
#7748

And i add the comments to it here, i also can change to the raw convert instead of using ByteView::from, but i think it's safe that we only using the prefix here, and inline/unlined all have the prefix.

https://github.com/apache/arrow-rs/pull/7748/files#diff-160ecd8082d5d28081f01cdb08a898cb8f49b17149c7118bf96746ddaae24b4fR560

May be we can make another PR ready to merge, then i can just copy the same code from there, thanks a lot!

let pref_b = ByteView::from(raw_b).prefix.swap_bytes();
if pref_a != pref_b {
return pref_a.cmp(&pref_b);
}

// 3.3 Fallback to full byte-slice comparison

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isn't this only valid if both views are not inlined (len > 12)? I am not sure it is ok to lexographically compare an inlined view and a non inline view 🤔 Won't that potentially compare the buffer offset to part of the strings?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Update: I double checked and value_unchecked correctly handles short/long views

Though it makes me wonder if we could squeeze more out of this function by handling the three cases explicitly (short, long), (long, short) and (long, long)

However, that might have a minimal payback

let full_a: &[u8] = unsafe { values.value_unchecked(a.0 as usize).as_ref() };
let full_b: &[u8] = unsafe { values.value_unchecked(b.0 as usize).as_ref() };
full_a.cmp(full_b)
};

// 4. Partially sort according to ascending/descending
if !options.descending {
sort_unstable_by(&mut valids, vlimit, cmp_mixed);
} else {
sort_unstable_by(&mut valids, vlimit, |x, y| cmp_mixed(x, y).reverse());
}

// 5. Assemble nulls and sorted indices into final output
let total = valids.len() + nulls.len();
let out_limit = limit.unwrap_or(total).min(total);
let mut out = Vec::with_capacity(total);

if options.nulls_first {
// Place null indices first
out.extend_from_slice(&nulls[..nulls.len().min(out_limit)]);
let rem = out_limit - out.len();
out.extend(valids.iter().map(|&(i, _, _)| i).take(rem));
} else {
// Place non-null indices first
out.extend(valids.iter().map(|&(i, _, _)| i).take(out_limit));
let rem = out_limit - out.len();
out.extend_from_slice(&nulls[..rem]);
}

out.into()
}

fn sort_fixed_size_binary(
Expand Down
Loading