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

Add in-place sorting methods to OrderedDictionary #103579

Open
eiriktsarpalis opened this issue Jun 17, 2024 · 11 comments
Open

Add in-place sorting methods to OrderedDictionary #103579

eiriktsarpalis opened this issue Jun 17, 2024 · 11 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections
Milestone

Comments

@eiriktsarpalis
Copy link
Member

Maybe it's worth adding more methods

public void Move(int oldIndex, int newIndex);
public void Sort(IComparer<TValue> comparer);

Originally posted by @kronic in #103309 (comment)

An in-place, stable sorting method in particular would be very useful in the context of using OrderedDictionary in System.Text.Json

cc @stephentoub

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 17, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jun 17, 2024
@eiriktsarpalis eiriktsarpalis added area-System.Collections api-ready-for-review API is ready for review, it is NOT ready for implementation and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 17, 2024
@eiriktsarpalis eiriktsarpalis added this to the 9.0.0 milestone Jun 17, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-collections
See info in area-owners.md if you want to be subscribed.

@stephentoub
Copy link
Member

stephentoub commented Jun 17, 2024

would be very useful in the context of using OrderedDictionary in System.Text.Json

Can you point to where?

And would it actually need Move? It's quite possible the fastest implementation would just sort the Entry list and then throw away and recreate the buckets.

@eiriktsarpalis
Copy link
Member Author

eiriktsarpalis commented Jun 17, 2024

Can you point to where?

Sure:

propertyCache.List.StableSortByKey(static propInfo => propInfo.Value.Order);

The List property of the current structure exposes the mutable List component for faster access.

@stephentoub
Copy link
Member

And it'd be ok for it to not be a stable sort?

@eiriktsarpalis
Copy link
Member Author

In this case stability is a requirement (it would change the semantics of the JsonPropertyOrderAttribute)

@eiriktsarpalis
Copy link
Member Author

We could perhaps make stability opt-in by feeding the index into the comparer?

@stephentoub
Copy link
Member

stephentoub commented Jun 17, 2024

We could perhaps make stability opt-in by feeding the index into the comparer?

Then you couldn't use IComparer<T> or Comparison<T>, which would make this a very unusual API and it would force the dictionary to have its own complete sorting implementation top to bottom, rather than being able to use Array.Sort and friends (unless it does what LINQ does, which is allocate a copy of the original indices, allocate a new comparer that uses it, etc.)

@eiriktsarpalis
Copy link
Member Author

An in-place stable sorting implementation is something we're sorely missing at the moment. We could start by adding it to array and span, then work our way up to List-like types.

@stephentoub
Copy link
Member

An in-place stable sorting implementation is something we're sorely missing at the moment. We could start by adding it to array and span, then work our way up to List-like types.

I'm fine with that, but I wouldn't want to add this API until that existed, we figured out naming conventions, there was something for this to depend on and use, etc. You marked this as 9.0; will you be pushing for a stable sort up and down the stack for 9 as well?

@eiriktsarpalis eiriktsarpalis added api-suggestion Early API idea and discussion, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 17, 2024
@eiriktsarpalis
Copy link
Member Author

Let's keep it as a suggestion for now.

@stephentoub
Copy link
Member

We could still expose Move (and/or Swap) if that would be the deal breaker with STJ using this type in 9.

@stephentoub stephentoub modified the milestones: 9.0.0, Future Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Collections
Projects
None yet
Development

No branches or pull requests

2 participants