Preserve the input element type in unique

[ararslan]

Previously the element type of the output was the smallest type that would fit the union of the input's individual element types. Now the output has an identical element type to the input.

Fixes #22696.

@nanosoldier runbenchmarks(ALL, vs=":master")

[ararslan]

Uh, what happened to AppVeyor?

[Sacha0]

:) !

[nanosoldier]

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

[StefanKarpinski]

Looks good to me – @JeffBezanson do you remember why it worked like this originally? Do we still want that logic for iterators or something, i.e. the case where we don't know the element type?

[ararslan]

Bump @JeffBezanson

[fredrikekre]

@test unique(Integer[1,1,2])::Vector{Integer} == [1,2] to test type and result? :)

[Sacha0]

lgtm for the touch ups! :)

[JeffBezanson]

This should call Base.iteratoreltype(itr) and use the old code if it returns EltypeUnknown().

[ararslan]

Will do, thanks Jeff. Should that check be in place of the !isleaftype(T) or should I work it into dispatch?

[JeffBezanson]

The check can be !isleaftype(T) && iteratoreltype....

[ararslan]

Timeouts and ProcessExitedExceptions across the board? That's more than a little concerning, but I don't see how it could be related, unless I'm missing something.

[ararslan]

Anyone have advice on the CI failures?

[ararslan]

Hmm, I can reproduce the stalling locally.

[ararslan]

When running in LLDB, I'm getting a SIGINT in the core tests built from this branch. LLDB session here: https://gist.github.com/ararslan/da7e7e0cfc2adb9ca40777d17cf75b9e.

[ararslan]

Okay, I've figured out what's happening now.

julia> U = Union{Int8, UInt8, Int16, UInt16, Int32, UInt32, Int64, UInt64, Int128, UInt128}
Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8}

julia> F = Base.uniontypes(U)[2]
Int16

julia> A = U[rand(F(1):F(10)) for i = 1:10]
10-element Array{Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8},1}:
 6
 5
 7
 5
 1
 1
 9
 1
 2
 1

julia> unique(A) # freezes

It freezes because now it hits a different code path. In this PR, !isleaftype(U) && iteratoreltype(A) == EltypeUnknown() is false, but the check prior to this PR, !isleaftype(U), is true. In this PR, it now tries to push! to a Set{U}() where U is that nasty Union type above. That kind of pushing explodes on 0.6, master, and this PR, but it isn't a problem on 0.6 and master since that code path doesn't get hit.

[quinnj]

Luckily, we seem to already have a fix for this over on #23367

julia> U = Union{Int8, UInt8, Int16, UInt16, Int32, UInt32, Int64, UInt64, Int128, UInt128}
Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8}

julia> F = Base.uniontypes(U)[2]
Int16

julia> A = U[rand(F(1):F(10)) for i = 1:10]
10-element Array{Union{Int128, Int16, Int32, Int64, Int8, UInt128, UInt16, UInt32, UInt64, UInt8},1}:
  3
 10
  2
  7
 10
  5
  9
  9
  2
  1

julia> unique(A)
7-element Array{Int16,1}:
  3
 10
  2
  7
  5
  9
  1

[ararslan]

Thanks, Jacob! In your example, is unique defined as it is here? Otherwise it won't be hitting the code that makes it freeze: pushing to a Set{Union{Nasty}}.

[ararslan]

I checked out your jq/bitsunions branch, merged in aa/unique-eltype, built, and tried the above example again. It appears unique still chokes on the crazy Union.

[ararslan]

Jacob has noted that the issue is likely #22688

[ararslan]

Okay, I've worked around the issue causing the core test to choke. Is this okay now?