Update(orderBy): Guaranteed stable sort #15914

BobChao87 · 2017-04-13T07:16:17Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature

What is the current behavior? (You can also link to an open issue here)

What is the new behavior (if this is a feature change)?

Falls back on built-in comparison function if user-provided comparison function fails to break a tie between elements being ordered.

Does this PR introduce a breaking change?

Yes

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

BREAKING CHANGE: Using orderBy with a custom comparator function while
simultaneously setting reverse to true results in a potentially different
list order than previously.

Specifically, when the custom comparator fails to sort two objects (compares
them as being "equal"), the objects would previously have remained in their
original order in the array relative to each other. They will now switch places
as the reverse will be more accurately respected.

To migrate, make sure that your custom comparator properly differentiates
between all cases that your code base cares about and that "equal" cases are
not order dependent. We expect that due to the nature of this change, since the
entries are considered "equal", that very few projects will be affected by this
change in all but a minor visual manor that still respects the expected
operation being performed.

gkalpak

A couple of minor comments only.
I am not sure about the BC tbh. I don't really consider it to be one. It would only break usecases where all of the following conditions are true:

The app is using a custom comparator.
The custom comparator fails to differentiate between two values.
The app expects both ascending and descending sorting to yield the same results (which depend on the browser).

gkalpak · 2017-04-14T09:34:23Z

test/ng/filter/orderBySpec.js

+        var reversed = ['B', 'a', 'c'];
+        var sorted = ['a', 'B', 'c'];
+
+        expect(orderBy(items)).toEqual(sorted);


This doesn't seem related.

gkalpak · 2017-04-14T09:35:37Z

src/ng/filter/orderBy.js

@@ -115,7 +115,7 @@
 *
 * @param {boolean=} reverse - If `true`, reverse the sorting order.
 * @param {(Function)=} comparator - The comparator function used to determine the relative order of
- *    value pairs. If omitted, the built-in comparator will be used.
+ *    value pairs. If omitted or finds two entries to be equal, the built-in comparator will be used.


I think this is more confusing than helpful. If they are interested they will read the details below.

BobChao87 · 2017-04-14T09:48:36Z

Fair enough about the breaking change. I wasn't sure either, but figured it would be better to write it up than gloss over it.

I'll get right to work on those changes.

lgalfaso · 2017-04-14T10:37:47Z

src/ng/filter/orderBy.js

@@ -599,7 +602,7 @@ function orderByFilter($parse) {
        }
      }

-      return compare(v1.tieBreaker, v2.tieBreaker) * descending;
+      return (compare(v1.tieBreaker, v2.tieBreaker) || defaultCompare(v1.tieBreaker, v2.tieBreaker)) * descending;


I do not fully understand this change, this should be just
return defaultCompare(v1.tieBreaker, v2.tieBreaker) * descending;

There is no reason for an external function to understand the internal tieBreaker structure.

This is not intended to defer to the defaultCompare function over the user-provided function, but rather to provide a reasonable fallback should a user-provided function fail to the break the tie. See #14481 (comment)

@lgalfaso, for better or worse, this tiebreaker structure is documented and has been used with custom comparator 😕 Do you think we should change that?
I thought it is useful to give the custom comparator a chance to break the tie in its own way (in the rare case where this will be needed).

lgalfaso · 2017-04-14T11:06:14Z

Was not aware that this was added as part of the public API. Once this was added, then I think it is best to keep it.

…

On Fri, Apr 14, 2017 at 1:00 PM George Kalpakas ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/ng/filter/orderBy.js <#15914 (comment)>: > @@ -599,7 +602,7 @@ function orderByFilter($parse) { } } - return compare(v1.tieBreaker, v2.tieBreaker) * descending; + return (compare(v1.tieBreaker, v2.tieBreaker) || defaultCompare(v1.tieBreaker, v2.tieBreaker)) * descending; @lgalfaso <https://github.com/lgalfaso>, for better or worse, this tiebreaker structure is documented and has been used with custom comparator 😕 Do you think we should change that? I thought it is useful to give the custom comparator a chance to break the tie in its own way (in the rare case where this will be needed). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15914 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAX44gelh5eDy1EUF7MJaAOqjkTOLxESks5rv1G-gaJpZM4M8VuB> .

BobChao87 · 2017-05-01T05:24:43Z

Was there anything else pending or can we go ahead and mark this as ready to be merged? I think we've covered all the concerns that have been raised.

gkalpak

Can you also change the commit message to adhere to our guidelines. E.g.:

fix(orderByFilter): guarantee stable sort

If the user-provided comparator fails to break a tie, fall back to the built-in comparator.

Fixes #14881

gkalpak · 2017-05-02T13:27:11Z

src/ng/filter/orderBy.js

@@ -460,6 +460,9 @@
 * way. (When specifying a custom comparator, you also need to pass a value for the `reverse`
 * argument - passing `false` retains the default sorting order, i.e. ascending.)
 *
+ * If your comparator fails to differentiate between two items, they will fall back to the built-in
+ * comparator function, this produces a guarantee of stable sorting.
+ *


Actually, this is the wrong place for this comment. It should be moved to L55 and be changed to something like:

If a custom comparator still can't distinguish between two items, then they will be sorted based on their index using the built-in comparator.

If user-provided comparison function fails to differentiate between two elements, fall back to built-in comparator. Fixes angular#14881

BobChao87 · 2017-05-15T07:30:10Z

Apologies about the commit message, apparently it's been a while since I read the contributing guidelines and I use a similar scheme on another project.

gkalpak

A couple of nit. I will fix up while merging.

gkalpak · 2017-05-19T11:21:48Z

src/ng/filter/orderBy.js

@@ -49,9 +49,8 @@
 * second, or `1` otherwise.
 *
 * In order to ensure that the sorting will be deterministic across platforms, if none of the
- * specified predicates can distinguish between two items, `orderBy` will automatically introduce a
- * dummy predicate that returns the item's index as `value`.
- * (If you are using a custom comparator, make sure it can handle this predicate as well.)


This line should not be removed. It is still necessary for the comparator to handle the tie-break value. E.g. if it has an array of strings, it should not expect that all values will be strings, because there is the possibility of getting passed a number (the index).

gkalpak · 2017-05-19T11:23:56Z

src/ng/filter/orderBy.js

- * specified predicates can distinguish between two items, `orderBy` will automatically introduce a
- * dummy predicate that returns the item's index as `value`.
- * (If you are using a custom comparator, make sure it can handle this predicate as well.)
+ * specified predicates nor the custom comparator can distinguish between two items, `orderBy` will


This change is confusing imo. The custom comparator is not something additional to the predicates. They are used together (the predicates provide the values and the comparator compares them).

If a user-provided comparator fails to differentiate between two items, fall back to the built-in comparator (using the tie-breaker predicate). Fixes #14881 Closes #15914

googlebot added the cla: yes label Apr 13, 2017

gkalpak suggested changes Apr 14, 2017

View reviewed changes

gkalpak added component: filters frequency: low needs: work severity: inconvenient labels Apr 14, 2017

gkalpak added this to the Backlog milestone Apr 14, 2017

BobChao87 force-pushed the patch-14881-orderBy-default-fallback branch from 854c03f to 8f236af Compare April 14, 2017 10:05

lgalfaso reviewed Apr 14, 2017

View reviewed changes

gkalpak approved these changes May 2, 2017

View reviewed changes

gkalpak suggested changes May 2, 2017

View reviewed changes

fix(orderBy): gaurantee stable sort

fe5f585

If user-provided comparison function fails to differentiate between two elements, fall back to built-in comparator. Fixes angular#14881

BobChao87 force-pushed the patch-14881-orderBy-default-fallback branch from 8f236af to fe5f585 Compare May 15, 2017 06:10

gkalpak approved these changes May 19, 2017

View reviewed changes

gkalpak pushed a commit that referenced this pull request May 19, 2017

fix(orderBy): guarantee stable sort

e50ed4d

If a user-provided comparator fails to differentiate between two items, fall back to the built-in comparator (using the tie-breaker predicate). Fixes #14881 Closes #15914

gkalpak closed this in 762580f May 19, 2017

GulajavaMinistudio mentioned this pull request May 19, 2017

fix(orderBy): guarantee stable sort angular-indonesia/angular.js#30

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update(orderBy): Guaranteed stable sort #15914

Update(orderBy): Guaranteed stable sort #15914

BobChao87 commented Apr 13, 2017

gkalpak left a comment

gkalpak Apr 14, 2017

gkalpak Apr 14, 2017 •

edited

Loading

BobChao87 commented Apr 14, 2017

lgalfaso Apr 14, 2017

BobChao87 Apr 14, 2017

gkalpak Apr 14, 2017

lgalfaso commented Apr 14, 2017 via email

BobChao87 commented May 1, 2017

gkalpak left a comment •

edited

Loading

gkalpak May 2, 2017 •

edited

Loading

BobChao87 commented May 15, 2017

gkalpak left a comment

gkalpak May 19, 2017

gkalpak May 19, 2017

Update(orderBy): Guaranteed stable sort #15914

Update(orderBy): Guaranteed stable sort #15914

Conversation

BobChao87 commented Apr 13, 2017

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak Apr 14, 2017

Choose a reason for hiding this comment

gkalpak Apr 14, 2017 • edited Loading

Choose a reason for hiding this comment

BobChao87 commented Apr 14, 2017

lgalfaso Apr 14, 2017

Choose a reason for hiding this comment

BobChao87 Apr 14, 2017

Choose a reason for hiding this comment

gkalpak Apr 14, 2017

Choose a reason for hiding this comment

lgalfaso commented Apr 14, 2017 via email

BobChao87 commented May 1, 2017

gkalpak left a comment • edited Loading

Choose a reason for hiding this comment

gkalpak May 2, 2017 • edited Loading

Choose a reason for hiding this comment

BobChao87 commented May 15, 2017

gkalpak left a comment

Choose a reason for hiding this comment

gkalpak May 19, 2017

Choose a reason for hiding this comment

gkalpak May 19, 2017

Choose a reason for hiding this comment

gkalpak Apr 14, 2017 •

edited

Loading

gkalpak left a comment •

edited

Loading

gkalpak May 2, 2017 •

edited

Loading