Skip to content

Commit

Permalink
REVERT ME: revert the re-introduction of PartialQuickSort
Browse files Browse the repository at this point in the history
There are some pretty compelling reasons to use PartialQuickSort instead of ScratchQuickSort as a base case for BracketedSort.
However, PartialQuickSort can segfault for invalid but seemingly innocuous lt functions.
I don't want to talk about introducing segfaults to the default sorting algorithms or introducing a regression to PartialQuickSort in this PR
Revert this commit it you want `partialsort!(::Vector{Int}, ::Int)` not to allocate.
  • Loading branch information
Lilith Hafner authored and Lilith Hafner committed Nov 4, 2023
1 parent 650c6a2 commit 5c18e25
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 13 deletions.
9 changes: 3 additions & 6 deletions base/sort.jl
Original file line number Diff line number Diff line change
Expand Up @@ -1151,10 +1151,8 @@ be much less than `next`'s. If the maximum additional space usage of `next` scal
then for small k the average* maximum additional space usage of BracketedSort will be
`O(n^(2.3/3))`.
By default, BracketedSort uses the in place `PartialQuickSort` algorithm recursively for
integer `target`s and the faster but not in place `ScratchQuickSort` for unit range
`target`s. This is because the runtime of recursive calls is negligible for large inputs
unless `k` is similar in size to `n`.
By default, BracketedSort uses the `O(n)` space and `O(n + k log k)` runtime
`ScratchQuickSort` algorithm recursively.
*Sorting is unable to depend on Random.jl because Random.jl depends on sorting.
Consequently, we use `hash` as a source of randomness. The average runtime guarantees
Expand All @@ -1178,8 +1176,7 @@ struct BracketedSort{T, F} <: Algorithm
end

# TODO: this composition between BracketedSort and ScratchQuickSort does not bring me joy
BracketedSort(k::Integer) = BracketedSort(k, k -> InitialOptimizations(PartialQuickSort(k)))
BracketedSort(k::OrdinalRange) = BracketedSort(k, k -> InitialOptimizations(ScratchQuickSort(k)))
BracketedSort(k) = BracketedSort(k, k -> InitialOptimizations(ScratchQuickSort(k)))

function bracket_kernel!(v::AbstractVector, lo, hi, lo_signpost, hi_signpost, o)
i = 0
Expand Down
16 changes: 9 additions & 7 deletions test/sorting.jl
Original file line number Diff line number Diff line change
Expand Up @@ -742,8 +742,7 @@ end
for alg in safe_algs
@test sort(v, alg=alg, lt = <=) == s
end
# Broken by the introduction of BracketedSort in #52006 which...
@test_broken partialsort(v, 172, lt = <=) == s[172] # ...uses PartialQuickSort which can segfault on invalid lt
@test partialsort(v, 172, lt = <=) == s[172]
@test partialsort(v, 315:415, lt = <=) == s[315:415]

# ...and it is consistently reverse stable. All these algorithms swap v[i] and v[j]
Expand All @@ -753,16 +752,15 @@ end
for alg in safe_algs
@test sort(1:n, alg=alg, lt = (i,j) -> v[i]<=v[j]) == perm
end
# Broken by the introduction of BracketedSort in #52006 which...
@test_broken partialsort(1:n, 172, lt = (i,j) -> v[i]<=v[j]) == perm[172] # ...uses PartialQuickSort which can segfault on invalid lt
@test_broken partialsort(1:n, 315:415, lt = (i,j) -> v[i]<=v[j]) == perm[315:415] # ...and is unstable
# Broken by the introduction of BracketedSort in #52006 which is unstable
@test_broken partialsort(1:n, 172, lt = (i,j) -> v[i]<=v[j]) == perm[172]
@test_broken partialsort(1:n, 315:415, lt = (i,j) -> v[i]<=v[j]) == perm[315:415]

# lt can be very poorly behaved and sort will still permute its input in some way.
for alg in safe_algs
@test sort!(sort(v, alg=alg, lt = (x,y) -> rand([false, true]))) == s
end
# Sometimes throws a bounds error, sometimes segfaults, works. Broken by #52006 & PartialQuickSort.
# @test partialsort(v, 172, lt = (x,y) -> rand([false, true])) ∈ 1:5
@test partialsort(v, 172, lt = (x,y) -> rand([false, true])) 1:5
@test all(partialsort(v, 315:415, lt = (x,y) -> rand([false, true])) .∈ (1:5,))

# issue #32675
Expand Down Expand Up @@ -1097,6 +1095,10 @@ end
x = rand(1:5, 1000)
@test partialsort(x, i, lt=(<=)) == sort(x)[i]
end
for i in [1, 7, 8, 490, 495, 852, 993, 996, 1000]
x = rand(1:5, 1000)
@test partialsort(x, i, lt=(<=)) == sort(x)[i]
end
end

# This testset is at the end of the file because it is slow.
Expand Down

0 comments on commit 5c18e25

Please sign in to comment.