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 Order.rev() for reverse sorting #55891

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aplavin
Copy link
Contributor

@aplavin aplavin commented Sep 26, 2024

Sorting with by=x -> (x[1], x[2]) sorts by x[1] and then (for x[1] duplicates) by x[2].

Then, sort(X; by) sorts in the ascending order of x[1] and x[2], sort(X; by, rev=true) sorts in the descending order of both.

But there's no way to sort in ascending order of x[1] and then descending order of x[2]. This PR makes it possible, with by=x -> (x[1], Order.rev(x[2]).

What do you think of this interface? I imagine this Order.rev function being public, but not exported.

@nsajko nsajko added sorting Put things in order feature Indicates new feature / enhancement requests needs tests Unit tests are required for this change needs docs Documentation for this change is required needs news A NEWS entry is required for this change labels Sep 26, 2024
@nsajko
Copy link
Contributor

nsajko commented Sep 26, 2024

Couldn't you base this on Ordering somehow?

@aplavin
Copy link
Contributor Author

aplavin commented Sep 26, 2024

Idk, what do you have in mind? ReverseOrdering exists but it basically serves rev=true.

@LilithHafner
Copy link
Member

I think this case is handled adequately by the lt ordering. lt = (a,b) -> isless(a[1], b[1]) || isequal(a[1], b[1]) && isless(b[2], a[2]). Yes, Order.rev would be concise, but I think that it is better to use functions to express complex comparisons rather than introduce additional API.

Having optimizations that dispatch on by ordering that return objects with Order.rev components could shift that balance a bit, but I don't see those optimizations being worth the complexity they would require.

@aplavin
Copy link
Contributor Author

aplavin commented Sep 28, 2024

Sure, it's possible to express any kind of sorting is lt already, and by argument is not necessary in general. It's just not too readable and more error-prone: eg, compare lt = (a,b) -> isless(a[1], b[1]) || isequal(a[1], b[1]) && isless(b[2], a[2]) and lt = (a,b) -> isless(a[1], b[1]) || isequal(a[1], b[1]) && isless(a[2], b[2]). Is it easy to spot the difference? To make a typo? Much cleaner to have by=x -> (x[1], Order.rev(x[2])) and by=x -> (x[1], x[2])!
And this is just two-part key, think about three or more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests needs docs Documentation for this change is required needs news A NEWS entry is required for this change needs tests Unit tests are required for this change sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants