nevi-me commented on a change in pull request #7193:
URL: https://github.com/apache/arrow/pull/7193#discussion_r426001210
##########
File path: rust/arrow/src/compute/kernels/sort.rs
##########
@@ -283,6 +285,30 @@ mod tests {
None,
vec![3, 1, 4, 2, 0, 5],
);
+ test_sort_to_indices_primitive_arrays::<Float32Type>(
+ vec![
+ None,
+ Some(-0.05),
+ Some(2.225),
+ Some(-1.01),
+ Some(-0.05),
+ None,
+ ],
+ None,
+ vec![3, 1, 4, 2, 0, 5],
+ );
+ test_sort_to_indices_primitive_arrays::<Float64Type>(
+ vec![
+ None,
+ Some(-0.05),
+ Some(2.225),
+ Some(-1.01),
+ Some(-0.05),
+ None,
Review comment:
PTAL at the solution that I've just pushed. Given that the `unwrap()`
will only fail if there's a `NAN`, I've replaced it by a default
`std::cmp::Ordering::Greater` to treat `NAN` as the highest value. In the
descending sort path, it ends up being inverted to the lowest value.
I suppose a better approach might be to let the `nulls_first` sort option
drive the behaviour, with the below sort options:
- ascending, nulls last: `Ordering::Greater` placing NaNs before the first
null
- ascending, nulls first: `Ordering::Less` placing NaNs after the last null
- descending, nulls last: `Ordering::Greater`
- descending, nulls first: `Ordering::Less`
... so the ordering being determined by the null behaviour (it helped to
write it out :smile:)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
