Fix ignored limit on lexsort_to_indices#2991
Conversation
|
|
||
| Ok(UInt32Array::from_iter_values( | ||
| value_indices.iter().map(|i| *i as u32), | ||
| value_indices.iter().take(len).map(|i| *i as u32), |
| ])) 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
| ]; | ||
| 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!)
isidentical
left a comment
isidentical
left a comment
There was a problem hiding this comment.
What a fast fix ❤️ Looks great to me!
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. |
5 participants
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