Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sortable iterators makes it harder to make non-breaking feature additions #51977

Closed
LilithHafner opened this issue Nov 1, 2023 · 7 comments · Fixed by #52010
Closed

Sortable iterators makes it harder to make non-breaking feature additions #51977

LilithHafner opened this issue Nov 1, 2023 · 7 comments · Fixed by #52010
Labels
iteration Involves iteration or the iteration protocol sorting Put things in order triage This should be discussed on a triage call
Milestone

Comments

@LilithHafner
Copy link
Member

LilithHafner commented Nov 1, 2023

As @rfourquet pointed out here, sort(::Dict) produces a Vector of Pairs. This is because of #46104 which, incidentally, has gone through triage twice already. An OrderedDict could be a better result and changing that after 1.10 is release would be breaking. Specifically, if in 1.13 we decide to add OrderedDict to Base and switched sort(::Dict) to return an OrderedDict that would break push!(sort(d::Dict), p::Pair).

This can be fixed by making sort(::Dict) throw, but then we get to the question of libraries. Adding support for sorting an iterable into a custom type that is not the same as the type of collect(itr) will become a breaking change.

#46104 is in 1.10.

Questions for triage:

  • Should we add another exception to make sort(::Dict) throw?
  • In light of this API design issue which IIRC triage did not explicitly discuss previously, should we revert Support sorting iterators #46104, or let it permanently land?
@LilithHafner LilithHafner added the triage This should be discussed on a triage call label Nov 1, 2023
@LilithHafner LilithHafner added this to the 1.10 milestone Nov 1, 2023
@KristofferC
Copy link
Member

I'm in favor of reverting it and require an explicit collect + sort! call.

@adienes
Copy link
Contributor

adienes commented Nov 1, 2023

what about documenting that sort(::SomeType) must return something iterable with matching eltype, but the exact container is implementation dependent? then Vector{Pair} --> OrderedDict is not breaking semver

@PallHaraldsson
Copy link
Contributor

PallHaraldsson commented Nov 1, 2023

Specifically, if in 1.13 we decide to add OrderedDict to Base and switched sort(::Dict) to return an OrderedDict that would break push!(sort(d::Dict), p::Pair).

[It would be surprising to many to get a different type out?]

I tried to add OrderedDict to Base, instead of Dict. It wasn't successful, nor were people happy with having both.

Do we need AbstractSortedDict <: AbstractDict in Base, and change Dict to be the OrderedDefaultDict implementation(also for compatibility with Python)?

I would support that, for 1.11, even for 1.10... Why did you only mention 1.13 (not even 1.12)?

You could define only sort(::AbstractSortedDict).

What does Julia itself need for itself? None of these sorted iterator since done without so far? Nor sort defined for it, nor actually unsorted Dict, we have now, I'm not sure an ordered one would be slower, does it depend on size, and is the size ever expected to be very large? We could have UDict, even unexported, if it helps Julia, with the current implementation, but I'm not sure it helps much. It needs to be benchmarked, and could go into DataStructures.jl only under some name?

@Moelf
Copy link
Contributor

Moelf commented Nov 8, 2023

#46104 (comment)

we can just add an error message to disable Dict if you want? this feels like throwing the baby out

@JeffBezanson
Copy link
Member

Arguably with sort(::Any) we are repeating the mistake we made with map, which necessitated the ugly hack of defining map on a Dict to throw an error. A vector is not generally the right thing to return from sort or map of an arbitrary container, so arguably the generic method is just not correct.

@vtjnash
Copy link
Member

vtjnash commented Nov 9, 2023

Both sort and map share the property that the result must be something vector-like, as they must return ordered containers (and in the intermediate phase, the container must support fairly general mutation). Dict is a weird case for map because iterating 0 or 2+ items works fine, but is only an error for the exact case of 1 Dict. That seems someone separate from sort, which doesn't have the Vararg behaviors to worry about

@MasonProtter
Copy link
Contributor

MasonProtter commented Nov 16, 2023

I wasn't aware of this, until today, but I just want to chime in to say I think this method is a major potential footgun, and we should be really cautious releasing v1.10 with it in.

I'd say at the very least this feature should be deferred until v1.11 so we have more time to think over the API consequences. Ref: #52010

@nsajko nsajko added iteration Involves iteration or the iteration protocol sorting Put things in order labels Jun 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iteration Involves iteration or the iteration protocol sorting Put things in order triage This should be discussed on a triage call
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants