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

Fix reversed sorting by multiple index + limit #120

Merged
merged 1 commit into from
Mar 29, 2021
Merged

Conversation

ale-rt
Copy link
Member

@ale-rt ale-rt commented Mar 26, 2021

Fix reversed sorting by multiple index by forcing the _sort_iterate_resultset sorting method when we have more than one sorting index

Fixes #108

@ale-rt
Copy link
Member Author

ale-rt commented Mar 26, 2021

This is probably a fix for #81 as well, even if I see that there is more advanced work happening in PR #82

Fix reversed sorting by multiple index by forcing the ``_sort_iterate_resultset`` sorting method when we have more than one sorting index

Fixes #108
@ale-rt
Copy link
Member Author

ale-rt commented Mar 26, 2021

Without this patch the added test fails like:

AssertionError: Lists differ: [97, 76, 52, 39, 29, 26, 25, 24, 17, 0] != [99, 98, 97, 96, 95, 94, 93, 92, 91, 90]

First differing element 0:
97
99

- [97, 76, 52, 39, 29, 26, 25, 24, 17, 0]
+ [99, 98, 97, 96, 95, 94, 93, 92, 91, 90]

@ale-rt ale-rt requested review from d-maurer and dataflake March 29, 2021 06:16
Copy link
Member

@dataflake dataflake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have any knowledge about the inner workings of ZCatalog, so I hope Dieter has better input. The unit test appears to show that your changes work, that's all I can go by.

Copy link
Contributor

@d-maurer d-maurer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change is obviously safe - but it may disregard optimization potential: reverse sorting can be optimized in the same way as forward sorting and multi-index sorting can be optimized recursively.

@ale-rt
Copy link
Member Author

ale-rt commented Mar 29, 2021

I would merge this because it fixes an issue observed by many people, even if it might cost some CPU cycles.
I believe it is better to have something that works slow rather than something that (in some case) doesn't work but it is faster.

I see #82 looks promising performance wise, but it seems to me it requires still some work before it gets merged.

Should I backport this patch to the 5.x branch?

@ale-rt ale-rt merged commit 5a3272f into master Mar 29, 2021
@ale-rt ale-rt deleted the fix-sorting branch March 29, 2021 09:32
@ale-rt
Copy link
Member Author

ale-rt commented Mar 29, 2021

Thanks for the reviews!

@dataflake
Copy link
Member

Should I backport this patch to the 5.x branch?

Unless there's someone is specifically asking for it I wouldn't bother.

@agitator
Copy link
Contributor

@ale-rt backport for 5.x would be great, since we're using that 5.x for Plone 5.2.x

@ale-rt
Copy link
Member Author

ale-rt commented Mar 29, 2021

@agitator yeah I know, anyway I think we can even switch to latest master, I do not see it so hard

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong sorting when using sort_limit
4 participants