feat(orderBy): Stable sort the input array #12408
Conversation
|
This indirectly fixes #12405 |
lgalfaso
changed the title
|
Does this have a performance implications? |
|
| @@ -183,6 +183,7 @@ function orderByFilter($parse) { | |||
| if (sortPredicate.length === 0) { sortPredicate = ['+']; } | |||
|
|
|||
| var predicates = processPredicates(sortPredicate, reverseOrder); | |||
| predicates.push({ get: function() { return {}; }, descending: reverseOrder ? -1 : 1}); | |||
There was a problem hiding this comment.
I think we should leave a comment explaining this, it won't be very nice to have to look up this issue in the blame for this function to figure out what this means once it stops being clear, otherwise this looks maybe a bit brittle
Just a quick header comment with an overview explaining how the predicate works, imho
It might still be a good idea to look at the impact this has on the bigtable benchmarks, and put some numbers in the commit message and issue --- so that there's a bit more confidence there. Can we do that? |
lgalfaso
force-pushed
the
stable-orderBy
branch
from
286c0b3 to
b3ecfc1
Compare
|
Run order_by benchpress several times with and without the patch With the patch Without the patch This PR does not make the code any faster (as the numbers look like they claim), but do show that the variations between runs is much bigger than whatever performance penalty this patch adds |
|
Can you add to the commit message that it fixes #12405? Otherwise LGTM |
Stable sort the input array Fixes angular#12405
lgalfaso
force-pushed
the
stable-orderBy
branch
from
b3ecfc1 to
37278ae
Compare
Stable sort the input array Closes angular#12408 Fixes angular#12405
Stable sort the input array