alamb commented on a change in pull request #236:
URL: https://github.com/apache/arrow-rs/pull/236#discussion_r623248741
##########
File path: arrow/src/compute/kernels/sort.rs
##########
@@ -487,24 +487,27 @@ where
len = limit.min(len);
}
if !descending {
- sort_by(&mut valids, len - nulls_len, |a, b| cmp(a.1, b.1));
+ sort_by(&mut valids, len.saturating_sub(nulls_len), |a, b| {
Review comment:
Silently ignoring the case where ` nulls_len`is greater than the total
length being sorted seems wrong to me.
It seems like the issue may be that `nulls_len` is not being updated
correctly to take the limit into account. In other words if there is a limit
the code should only look at the first `limit` rows to see if there are any
nulls present.
I figured it might be easier to try this out myself than to try and explain
it; Here is my proposed fix (to your fix) for your consideration:
https://github.com/medwards/arrow-rs/pull/1
--
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]
