Implement partial_sort_unstable for slice

[tisonkun]

This refers to #149046.

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[tisonkun]

cc @orlp

[orlp]

Left some remarks, some on style, but also some with substance.

Besides comments on the code that's written, I do note a lack of tests?

View changes since this review

[tisonkun]

I do note a lack of tests?

Doc tests cover most branches. I don't find a dedicated file to cover its cousin sort_unstable. If you can point me to one, I'm glad to add cases there.

[orlp]

The examples can change at any time. And you didn't test, for example, the post-condition that all elements ..start are less than or equal to the elements start..end and that those are less than or equal to the elements end.., including for the zero-length case.

[tisonkun]

The examples can change at any time. And you didn't test, for example, the post-condition that all elements ..start are less than or equal to the elements start..end and that those are less than or equal to the elements end.., including for the zero-length case.

Thanks and yes. Do you know where the unit tests of sort/sort_unstable locate?

[orlp]

I believe the bulk is found in https://github.com/rust-lang/rust/blob/main/library/alloctests/tests/sort/tests.rs.

[orlp]

What I suggested in the ACP was a sketch implementation, I did some more thinking and I think the following handles all corner cases nicely:

pub fn partial_sort<T, F, R>(mut v: &mut [T], range: R, is_less: &mut F)
where
    F: FnMut(&T, &T) -> bool,
    R: RangeBounds<usize>,
{
    let len = v.len();
    let Range { start, end } = slice::range(range, ..len);
    
    if end - start <= 1 {
        // Can be resolved in at most a single partition_at_index call, without
        // further sorting. Do nothing if it is an empty range at start or end.
        if start != len && end != 0 {
            sort::select::partition_at_index(v, start, is_less);
        }
        return;
    }
    
    // Don't bother reducing the slice to sort if it eliminates fewer than 8 elements.
    if end + 8 <= len {
        v = sort::select::partition_at_index(v, end - 1, is_less).0;
    }
    if start >= 8 {
        v = sort::select::partition_at_index(v, start, is_less).2;
    }
    sort::unstable::sort(v, is_less);
}

And to formalize the post-conditions, I think the following should hold after a call to v.partial_sort_unstable(b..e):

for i in 0..b {
    for j in b..n {
        assert!(v[i] <= v[j]);
    }
}
for i in 0..e {
    for j in e..n {
        assert!(v[i] <= v[j]);
    }
}
for i in b..e {
    for j in i..e {
        assert!(v[i] <= v[j]);
    }
}

[quaternic]

And to formalize the post-conditions, I think the following should hold after a call to v.partial_sort_unstable(b..e):

A lot of those individual comparisons are implied by transitivity of the ordering, so it can be reduced to choosing the maximum of the prefix (if any), the minimum of the suffix (if any), and then asserting that the concatenation is sorted.

Informally, max(v[..b]) <= v[b] <= v[b + 1] <= ... <= v[e-1] <= min(v[e..]), or in code:

let max_before = v[..b].iter().max().into_iter();
let sorted_range = v[b..e].iter();
let min_after = v[e..].iter().min().into_iter();
let seq = max_before.chain(sorted_range).chain(min_after);
assert!(seq.is_sorted());

That's pretty much what you said in rust-lang/libs-team#685 (comment) , just using transitivity of the comparison. Without assuming that, the implementation couldn't guarantee the universally quantified property anyway.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[tisonkun]

Pushed a new implementation.

I'm writing tests but perhaps we'd have a new mod under library/alloctests/tests/partial_sort rather than patch the existing library/alloctests/tests/sort/tests.rs. Since the sort tests are heavily depending on macros while partial sorts assertions are quite different.

[tisonkun]

Pushed a new implementation.

I'm writing tests but perhaps we'd have a new mod under library/alloctests/tests/partial_sort rather than patch the existing library/alloctests/tests/sort/tests.rs. Since the sort tests are heavily depending on macros while partial sorts assertions are quite different.

cc @Amanieu for early review for the direction and advice on where to organize the tests.

[Amanieu]

Regarding the tests I'm happy with either a separate file or as part of the existing tests as you see fit.

[tisonkun]

Pushed some basic test cases at 01bfcc0.

The existing sort tests have many assumptions, like checked_sort always sorts the whole range (for sure), and this helper is tightly coupled in cases. I can hardly insert the range concept in the current sort tests set up.

I added the basic test cases and will try to leverage the pattern functions in the sort module to generate more cases. But this patch itself should be mergeable as long as the implementation looks good, since now we have the test structure, which can be improved continuously.

[scottmcm]

r? libs