-
Notifications
You must be signed in to change notification settings - Fork 172
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
Fix incorrect results for sort/distinct over a link after a sort #3102
Conversation
When a sort or distinct over links was done on an already-sorted TableView, the link translation map was done using the unsorted rows, resulting in the second sort/distinct being done with the incorrect values.
cc09edd
to
3bb6a5c
Compare
Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-3102/2/Performance_Report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok nice catch of this case!
Looks like we can't get rid of passing around the IntegerColumn so easily though because the unit tests reveal instability of stale rows across sync_if_needed
.
I don't see any test failures related to stale rows over sync_if_needed(). |
Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-3102/3/Performance_Report |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I was mistaken.
Nice fix 👍
Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-3102/4/Performance_Report |
Check the performance result here: https://ci.realm.io/job/realm/job/realm-core/job/PR-3102/5/Performance_Report |
When a sort or distinct over links was done on an already-sorted TableView, the link translation map was done using the unsorted rows, resulting in the second sort/distinct being done with the incorrect values.
This appears to have been broken since the functionality was first introduced and is not a regression.
Reported at realm/realm-swift#5936.