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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
