[GitHub] [arrow-rs] alamb commented on a diff in pull request #2991: Fix ignored limit on `lexsort_to_indices`

[GitBox <gi...@apache.org>]

alamb commented on code in PR #2991:
URL: https://github.com/apache/arrow-rs/pull/2991#discussion_r1009306085


##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -3519,7 +3532,8 @@ mod tests {
                 Some(-2),
             ])) as ArrayRef,
         ];
-        test_lex_sort_arrays(input, expected, None);
+        test_lex_sort_arrays(input.clone(), expected.clone(), None);
+        test_lex_sort_arrays(input, slice_arrays(expected, 0, 2), Some(2));

Review Comment:
   This test fails immediately without the fix -- the output is too big!)



##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -3439,7 +3451,8 @@ mod tests {
             Some(2),
             Some(17),
         ])) as ArrayRef];
-        test_lex_sort_arrays(input.clone(), expected, None);
+        test_lex_sort_arrays(input.clone(), expected.clone(), None);
+        test_lex_sort_arrays(input.clone(), slice_arrays(expected, 0, 2), Some(2));

Review Comment:
   This addition is strictly unnecessary from a coverage perspective (it was already covered), but I wanted to make the `test_lex_sort_arrays` based tests all consistently patterned so it was easier to reason about coverage



##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -3439,7 +3451,8 @@ mod tests {
             Some(2),
             Some(17),
         ])) as ArrayRef];
-        test_lex_sort_arrays(input.clone(), expected, None);
+        test_lex_sort_arrays(input.clone(), expected.clone(), None);
+        test_lex_sort_arrays(input.clone(), slice_arrays(expected, 0, 2), Some(2));

Review Comment:
   The only place on master that a limit is passed to `lexsort_to_indices` in the tests is immediately below here. However, very sadly, there is a special case code path for single arrays that doesn't hit the bug path
   
   ```
           let expected = vec![Arc::new(PrimitiveArray::<Int64Type>::from(vec![
               Some(-1),
               Some(0),
               Some(2),
           ])) as ArrayRef];
           test_lex_sort_arrays(input, expected, Some(3));
   ```



##########
arrow/src/compute/kernels/sort.rs:
##########
@@ -950,7 +950,7 @@ pub fn lexsort_to_indices(
     });
 
     Ok(UInt32Array::from_iter_values(
-        value_indices.iter().map(|i| *i as u32),
+        value_indices.iter().take(len).map(|i| *i as u32),

Review Comment:
   this is the bugfix



-- 
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