Fix ignored limit on lexsort_to_indices
#2991
Conversation
| @@ -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), | |||
| @@ -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)); | |||
There was a problem hiding this 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));
There was a problem hiding this 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
| @@ -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)); | |||
There was a problem hiding this comment.
This test fails immediately without the fix -- the output is too big!)
Co-authored-by: Batuhan Taskaya <[email protected]>
|
I plan to create 26.0.0 RC2 with this fix |
* Fix ignored limit on lexsort_to_indices * Update comments * Update arrow/src/compute/kernels/sort.rs Co-authored-by: Batuhan Taskaya <[email protected]> Co-authored-by: Batuhan Taskaya <[email protected]>
|
Benchmark runs are scheduled for baseline = 40d61ec and contender = 66c9636. 66c9636 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #2990
Rationale for this change
Regresssion was introduced in #2929 by https://github.com/apache/arrow-rs/pull/2929/files#r1005140128 and there was no test coverage 😭
What changes are included in this PR?
Are there any user-facing changes?
Not really as we haven't released this code yet
cc @isidentical