recognize Tuple and AbstractVector as Ordered in Base.OrderStyle - #62485
Merged
Conversation
adienes
reviewed
Jul 23, 2026
| OrderStyle(::Type{<:Any}) = Unordered() | ||
| OrderStyle(::Type{Union{}}, slurp...) = Ordered() | ||
|
|
||
| OrderStyle(T::Type{<:Tuple}) = _tuple_ordering(T) |
Member
There was a problem hiding this comment.
Suggested change
| OrderStyle(T::Type{<:Tuple}) = _tuple_ordering(T) | |
| OrderStyle(T::Type{<:Tuple}) = all(==(Ordered()) ∘ OrderStyle, fieldtypes(T)) ? Ordered() : Unordered() |
Contributor
Author
There was a problem hiding this comment.
Not sure why but all() reports consistently higher runtimes than recursion.
julia> b = @be sort!([(rand(1:20), rand(1:20)) for _ in 1:100]) unique!
Benchmark: 2811 samples with 185 evaluations
min 150.270 ns
median 163.243 ns
mean 164.669 ns
max 362.703 ns
julia> b = @be sort!([(rand(1:20), rand(1:20)) for _ in 1:1000]) unique!
Benchmark: 1453 samples with 68 evaluations
min 436.765 ns
median 461.765 ns
mean 464.643 ns
max 1.015 μs
Benchmark: 3049 samples with 316 evaluations
min 71.835 ns
median 87.658 ns
mean 88.143 ns
max 305.696 ns
julia> b = @be sort!([(rand(1:20), rand(1:20)) for _ in 1:1000]) unique!
Benchmark: 1470 samples with 86 evaluations
min 365.116 ns
median 391.860 ns
mean 393.518 ns
max 852.326 ns
I'd guess that all() isn't able to resolve at compile time and is adding runtime overhead
Member
There was a problem hiding this comment.
maybe needs a check for isconcretetype
julia> function OrderStyle(::Type{T}) where {T<:Tuple}
isconcretetype(T) || return Unordered()
all(S -> OrderStyle(S) === Ordered(), fieldtypes(T)) ? Ordered() : Unordered()
end
OrderStyle
julia> b = @be sort!([(rand(1:20), rand(1:20)) for _ in 1:100]) unique!
Benchmark: 3540 samples with 148 evaluations
min 148.649 ns
median 163.851 ns
mean 167.909 nsthis is also needed for correctness. the current implementation fails on something like unique!([(1,), (1,2)]) since fieldtypes doesn't work
Contributor
Author
There was a problem hiding this comment.
@adienes Pushed the isconcretetype() check, also found out adding map() skips iterating over at runtime so speeds are the same as recursion without the O(N^2) complexity
julia> @be sort!([(rand(1:20), rand(1:20)) for _ in 1:100]) unique!
Benchmark: 2510 samples with 323 evaluations
min 78.328 ns
median 88.545 ns
mean 89.448 ns
max 583.591 ns
adienes
reviewed
Jul 26, 2026
Co-authored-by: Andy Dienes <51664769+adienes@users.noreply.github.com>
adienes
approved these changes
Jul 26, 2026
Member
|
Thanks, @AMVS24! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As mentioned in issue #61959
unique!andalluniquehave a sorted fast path method gated onBase.OrderStyle, but the trait only applies to a short, obvious list(Real, AbstractString, Symbol and Union{}), leaving the fast path unreachable for any other valid case. Since Tuples and Vectors both famously have a total ordering which satisfies the contiguous requirement ofunique!they should be includedAdded:
OrderStyle(::Type{<:AbstractVector{T}}): vector is Ordered iff T isThe original issue also mentioned CartesianIndex and other user defined types that simply wrap tuples/vectors but I'd like maintainer input before tackling that