Re: [PR] Create version of LexicographicalComparator that compares fixed number of columns (~ -15%) [arrow-rs]

["Copilot (via GitHub)" <gi...@apache.org>]

Copilot commented on code in PR #7530:
URL: https://github.com/apache/arrow-rs/pull/7530#discussion_r2097291094


##########
arrow-ord/src/sort.rs:
##########
@@ -724,14 +724,44 @@ pub fn lexsort_to_indices(
         len = limit.min(len);
     }
 
-    let lexicographical_comparator = LexicographicalComparator::try_new(columns)?;
-    // uint32 can be sorted unstably
-    sort_unstable_by(&mut value_indices, len, |a, b| {
-        lexicographical_comparator.compare(*a, *b)
-    });
-
-    Ok(UInt32Array::from_iter_values(
-        value_indices.iter().take(len).map(|i| *i as u32),
+    match columns.len() {

Review Comment:
   The match arms for fixed column counts (2, 3, 4, 5) contain nearly identical logic. Consider extracting the common comparator creation and sort_unstable_by call into a helper function or macro to reduce code duplication.



##########
arrow-ord/src/sort.rs:
##########
@@ -781,6 +811,49 @@ impl LexicographicalComparator {
     }
 }
 
+/// A lexicographical comparator that wraps given array data (columns) and can lexicographically compare data
+/// at given two indices. This version of the comparator is for compile-time constant number of columns.
+/// The lifetime is the same at the data wrapped.
+pub struct FixedLexicographicalComparator<const N: usize> {
+    compare_items: [DynComparator; N],
+}
+
+impl<const N: usize> FixedLexicographicalComparator<N> {
+    /// lexicographically compare values at the wrapped columns with given indices.
+    pub fn compare(&self, a_idx: usize, b_idx: usize) -> Ordering {
+        for comparator in &self.compare_items {
+            match comparator(a_idx, b_idx) {
+                Ordering::Equal => continue,
+                r => return r,
+            }
+        }
+        Ordering::Equal
+    }
+
+    /// Create a new lex comparator that will wrap the given sort columns and give comparison
+    /// results with two indices.
+    pub fn try_new(

Review Comment:
   Document that the generic parameter N must match the number of columns provided, and consider enhancing the error message in map_err with details about the expected count to aid in debugging.



-- 
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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org