Support sorting iterators

[LilithHafner]

Closes #38328

[gbaraldi]

Is the specific AbstractVector method necessary since it dispatches to the same code? Or would removing that method be breaking?

[LilithHafner]

Without it, sort([1,2,3]) would dispatch to sort(A::AbstractArray; dims::Integer, ...), a more specific method than sort(v; kw...), and throw an UndefKeywordError.

[gbaraldi]

Outside of the .1 vs 0.1 LGTM

[ararslan]

See also #16853 for some historical context

[LilithHafner]

Thanks for the link @ararslan!

I hadn't noticed the problematic error messages for sort(4) and sort('c') before.

My thoughts on the past conversation:

But would it not be strange to get an array from sort when passed a general iterable?

What else could you get?

There is a precedent of returning the most specific type that can be reasonably sorted, which may not be the input type. For example, ranges sorted in forward and reverse orders produce ranges; StaticVectors produce MutableVectors.

I would favor this possibility to sort general iterables, but without special cases: this would return an array even for tuples and strings as input.

I agree. I feel called to implement a different sorting algorithm for every tuple length that returns a tuple, but for general tuples sorting would not be type stable, and sorting NTuples seems to be the domain of StaticArrays, not base.

Most people want Strings not Vector{Char}s, but most people don't sort strings. For those that do sort strings, I think it's fine to give them a Vector{Char}. They just stored it, so they probably think of characters as re-arrangeable.

Just sorting the Unicode characters also doesn't make too much sense, you'd really want to find all of the graphemes and sort those, I'd imagine.

This seems pretty sensible to me. The umlaut stays with the a, for example. @ScottPJones or others, do you have an example of a grapheme that is not represented as a single Char?

julia> sort("亀 mäke ☃🙂 bc. she 💞🐢s")
21-element Vector{Char}:
 ' ': ASCII/Unicode U+0020 (category Zs: Separator, space)
 ' ': ASCII/Unicode U+0020 (category Zs: Separator, space)
 ' ': ASCII/Unicode U+0020 (category Zs: Separator, space)
 ' ': ASCII/Unicode U+0020 (category Zs: Separator, space)
 ' ': ASCII/Unicode U+0020 (category Zs: Separator, space)
 '.': ASCII/Unicode U+002E (category Po: Punctuation, other)
 'b': ASCII/Unicode U+0062 (category Ll: Letter, lowercase)
 'c': ASCII/Unicode U+0063 (category Ll: Letter, lowercase)
 'e': ASCII/Unicode U+0065 (category Ll: Letter, lowercase)
 'e': ASCII/Unicode U+0065 (category Ll: Letter, lowercase)
 'h': ASCII/Unicode U+0068 (category Ll: Letter, lowercase)
 'k': ASCII/Unicode U+006B (category Ll: Letter, lowercase)
 'm': ASCII/Unicode U+006D (category Ll: Letter, lowercase)
 's': ASCII/Unicode U+0073 (category Ll: Letter, lowercase)
 's': ASCII/Unicode U+0073 (category Ll: Letter, lowercase)
 'ä': Unicode U+00E4 (category Ll: Letter, lowercase)
 '☃': Unicode U+2603 (category So: Symbol, other)
 '亀': Unicode U+4E80 (category Lo: Letter, other)
 '🐢': Unicode U+1F422 (category So: Symbol, other)
 '💞': Unicode U+1F49E (category So: Symbol, other)
 '🙂': Unicode U+1F642 (category So: Symbol, other)

In summary, I stand by this PR as is (with better error messages) but can see an argument for making strings throw a MethodError instead.

[LilithHafner]

@gbaraldi, in light of the historical context from @ararslan's PR, do you still think this PR looks good as is?

[gbaraldi]

That's a tough one, I can see someone calling sort on a string as a way to make it alphabetical order so getting a string back makes sense. The first tutorial for sorting a string (https://www.geeksforgeeks.org/sorting-of-strings-in-julia/) collects it, sorts and then joins, which sounds a bit roundabout.
In the other hand almost all things will return arrays as a result of sort.

[Moelf]

Can we just keep this PR but add an error on

sort(::AbstractString)

with a message like ambiguity or something?

[LilithHafner]

Seems reasonable. For almost all iterables this PR is clean; no reason to sweat on the String case unless someone really wants to sort strings:

julia> sort("hello world")
ERROR: ArgumentError: sort(x::AbstractString) is ambiguous. Use sort!(collect(x)) or String(sort!(collect(x))) instead.

[LilithHafner]

How do folks feel about this PR now that we throw an error on AbstractStrings and a better error on Numbers and other zero-dimensional iterators?

[JeffBezanson]

Shouldn't infinite (and maybe unknown) also be errors? I see that people probably want an error for sort(1), so ok, but kind of strange that that of all things would be disallowed.

[LilithHafner]

Unknown sizes should be supported

sort(w for w in ["karma", "infracostalis", "postencephalon", "Elia"] if all(islowercase, w))

For almost all infinite iterators, we already throw inside copymutable, but perhaps someone could define an infinite iterator that can be copymutableed, returning a sparse vector of infinite length. If they also define a sorting method for that sparse representation, then it would be a mistake to throw on infinite iterators.

[Seelengrab]

But sort is an eager operation - surely if someone wants to sort their infinite iterator lazily, they have to implement it either way and not just rely on the generic fallback that's supposed to collect all elements of the iterator? Wouldn't it be better UX to throw early and make them aware where the actual problem lies, instead of having them chase down an (incidental) error from copymutable?

[LilithHafner]

For generic iterables, copymutable collects, but it only needs to make a mutable copy, not necessarily instantiate every element. For example, I believe this is allowed:

struct WithTrailingZerosIterable
    head::Int
    tail::Union{Nothing, WithTrailingZerosIterable}
end

Base.iterate(x::WithTrailingZerosIterable) = iterate(x, x)
Base.iterate(::WithTrailingZerosIterable, x::WithTrailingZerosIterable) = x.head, x.tail
Base.iterate(::WithTrailingZerosIterable, ::Nothing) = 0, Nothing

Base.IteratorSize(::Type{<:WithTrailingZerosIterable}) = Base.SizeInfinite()


struct WithTrailingZerosVector <: AbstractVector{Int}
    data::Vector{Int}
end
Base.size(x::WithTrailingZerosVector) = (typemax(Int),)
Base.getindex(x::WithTrailingZerosVector, i::Int) = i <= length(x.data) ? x.data[i] : 0
function Base.show(io::IO, ::MIME"text/plain", x::WithTrailingZerosVector)
    println(io, "infinite-element WithTrailingZerosVector:")
    for i in 1:5
        x[i] >= 0 && print(io, ' ')
        println(io, x[i])
    end
    println(" ⋮")
end


function Base.collect(x::WithTrailingZerosIterable)
    data = Int[]
    while x !== nothing
        push!(data, x.head)
        x = x.tail
    end
    WithTrailingZerosVector(data)
end

function Base.sort!(x::WithTrailingZerosVector)
    filter!(x -> x < 0, x.data)
    sort!(x.data)
    return x
end


const X = WithTrailingZerosIterable(1, WithTrailingZerosIterable(-2, WithTrailingZerosIterable(3, nothing)))
display(collect(X))
#=
infinite-element WithTrailingZerosVector:
 1
-2
 3
 0
 0
 ⋮
=#

display(sort!(Base.copymutable(X)))
#=
infinite-element WithTrailingZerosVector:
-2
 0
 0
 0
 0
 ⋮
=#

Nevertheless, we could put this in the same camp as sort(::AbstractString): perhaps it might make sense in some way but for now just throw because it is rarely a good idea to sort an infinite iterable.

[LilithHafner]

Throwing on an infinite iterator seems like the sensible choice. I've made the change.

[JeffBezanson]

Triage approves.

[PallHaraldsson]

Sorry, I believe this doesn't work, because:

or others, do you have an example of a grapheme that is not represented as a single Char?

yes, flag emojis can be long, at least 5 Chars (codeunits, here 11 bytes):

julia> collect("🏳️‍⚧️")
5-element Vector{Char}:
 '🏳': Unicode U+1F3F3 (category So: Symbol, other)
 '️': Unicode U+FE0F (category Mn: Mark, nonspacing)
 '\u200d': Unicode U+200D (category Cf: Other, format)
 '⚧': Unicode U+26A7 (category So: Symbol, other)
 '️': Unicode U+FE0F (category Mn: Mark, nonspacing)

I chose a flag at random, from here:
https://emojipedia.org/flags/

i.e. the transgender flag, and we do not want to piss off people, screwing it up.

julia> sort2("🏳️‍⚧️")
5-element Vector{Char}:
 '\u200d': Unicode U+200D (category Cf: Other, format)
 '⚧': Unicode U+26A7 (category So: Symbol, other)
 '️': Unicode U+FE0F (category Mn: Mark, nonspacing)
 '️': Unicode U+FE0F (category Mn: Mark, nonspacing)
 '🏳': Unicode U+1F3F3 (category So: Symbol, other)

I believe you want to get the same order back. I'm not completely sure it's sensitive to the order, though pretty confident. This doesn't look right when pasting into the REPL (a minor unrelated issue), so I'm not sure how to easily check after sorting.

I didn't see this sooner, hope I was of help, and this can be fixed before the feature freeze. I knew knowing obscure Uncode (flag) emoji trivia would some day come in handy. Ok, not really.

Such stuff was in the lead of the Wikipeda Unicode article, where I put it, at some point, before someone shortened it to a short summary:
https://en.wikipedia.org/w/index.php?title=Unicode&oldid=1086712843

Examples include: the Devanagari kshi, which is encoded by 4 code points

I put such stuff in the lead, not (just) because it's trivia, but also to show people Unicode is in practice variable-length, even UTF-16 (and to be avoided, some think it's better since fixed-length, but that hasn't been true since UCS-2).

[LilithHafner]

I've implemented triage's requested changes.

[LilithHafner]

Hmmm, I'm looking back at @PallHaraldsson's objections, and I think implementing reverse(::AbstractString) might have been a mistake. And even though we have reverse(::AbstractString), I'm not convinced that sort(::AbstractString) is a good idea to provide. It seems like too much of a foot gun. I'm picturing folks submitting issues like the one @PallHaraldsson brought up here, and the position "yes, those mangled results are expected behavior" seems pretty weak. I'd rather say "that's a strange thing to want to do to a string, but if you really want sort the characters in a string, you can do String(sort(collect(s))) or join(sort(SomePackage.collect_graphemes(s)))".

Is anyone opposed to reverting d42c234?

[LilithHafner]

Everything but whether to support sorting strings is settled and ready to go. Unless folks object, I'm going to merge this in a couple days without support for sorting strings (listening to the first triage, not the second triage on that point only). If we decide that it is best to support sorting strings later, that will be non-breaking.

[vtjnash]

Could I ask why there is a discrepancy here, so that I still can't get a sorted list of keys, but can take the keys from a sorted dictionary:

julia> sort(keys(Dict(1=>2)))
ERROR: MethodError: no method matching sort!(::Set{Int64})

Closest candidates are:
  sort!(::AbstractUnitRange)
   @ Base range.jl:1392
  sort!(::AbstractVector, ::Base.Sort.Algorithm, ::Base.Order.Ordering)
   @ Base sort.jl:2245
  sort!(::AbstractVector{T}, ::Integer, ::Integer, ::Base.Sort.MergeSortAlg, ::Base.Order.Ordering, ::Vector{T}) where T
   @ Base sort.jl:2172
  ...

Stacktrace:
 [1] sort(v::Base.KeySet{Int64, Dict{Int64, Int64}}; kws::@Kwargs{})
   @ Base.Sort ./sort.jl:1503
 [2] sort(v::Base.KeySet{Int64, Dict{Int64, Int64}})
   @ Base.Sort ./sort.jl:1499
 [3] top-level scope
   @ REPL[7]:1

julia> first.(sort(Dict(1=>2)))
1-element Vector{Int64}:
 1

[LilithHafner]

For folks following along, the answer is because of #52086 (comment)