Fix reversed sorting by multiple index + limit

zopefoundation / Products.ZCatalog

Zope's indexing and search solution.

Other

5 stars 22 forks source link

Fix reversed sorting by multiple index + limit #120

Closed ale-rt closed 3 years ago

ale-rt commented 3 years ago

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 commented 3 years ago

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

ale-rt commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Thanks for the reviews!

dataflake commented 3 years ago

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

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

agitator commented 3 years ago

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

ale-rt commented 3 years ago

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