Skip to content

ZREVRANGEBYSCORE Optimization for out of range offset#5773

Merged
oranagra merged 5 commits intoredis:unstablefrom
omigafu:unstable
Nov 17, 2020
Merged

ZREVRANGEBYSCORE Optimization for out of range offset#5773
oranagra merged 5 commits intoredis:unstablefrom
omigafu:unstable

Conversation

@omigafu
Copy link
Contributor

@omigafu omigafu commented Jan 12, 2019 •
edited
Loading

Hi @antirez , there is a place optimized.
The edge case, where the offset exceeds the length of the zset. This is an invalid offset. It should be better to return directly. see issue #5767

@omigafu
Copy link
Contributor Author

omigafu commented Aug 23, 2019

this is a meaningful optimization, i think. @antirez

@antirez
Copy link
Contributor

antirez commented Sep 2, 2019

Hi @omigafu, this sounds good, I guess that every time there is a loop in which we scan with LIMIT for pagination or alike, at least the final query that would just skip all the elements will return ASAP. I wonder however if you had a real world experience about that. Btw note that the PR is wrong because it inherits a bug that was already here, so I'll merge it in a few days, after fixing the bug I introduced when migrating to RESP3. Basically it should not reply with a NULL reply when the index is out of range, but with an empty array.

@omigafu
Copy link
Contributor Author

omigafu commented Oct 1, 2019

Hi @antirez , sorry to see your reply so late.
I have experienced that when the parameters are not verified well, redis occasionally receives a large offset. At this point, it takes a long time, but I am not sure if it is caused by this problem. And I found that it can be improved.
BTW, I have fixed the bug you mentioned. Plz check.: )

@omigafu
Copy link
Contributor Author

omigafu commented Nov 2, 2020

@oranagra do you have any time to review it?

@oranagra oranagra linked an issue Nov 2, 2020 that may be closed by this pull request
[ZREVRANGEBYSCORE] When the offset is too large, the query is very slow. #5767
Closed
oranagra
oranagra previously approved these changes Nov 2, 2020
View reviewed changes
@oranagra
Copy link
Member

oranagra commented Nov 2, 2020

@omigafu thank you.
maybe you wanna improve the PR by adding a test that covers this case?
doesn't need to test efficiency, just trigger the edge case and verify the response, so that it's covered by the test suite.

@oranagra oranagra added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label Nov 2, 2020
@oranagra oranagra requested a review from itamarhaber November 17, 2020 11:03
@oranagra oranagra changed the title Optimization, for invalid offset, return directly ZREVRANGEBYSCORE Optimization for out of range offset Nov 17, 2020
@oranagra oranagra merged commit 39f716a into redis:unstable Nov 17, 2020
JackieXie168 pushed a commit to JackieXie168/redis that referenced this pull request Nov 19, 2020
@omigafu @oranagra
ZREVRANGEBYSCORE Optimization for out of range offset (redis#5773)
0d1e94f 
ZREVRANGEBYSCORE key max min [WITHSCORES] [LIMIT offset count]
When the offset is too large, the query is very slow. Especially when the offset is greater than the length of zset it is easy to determine whether the offset is greater than the length of zset at first, and If it exceed the length of zset, then return directly.

Co-authored-by: Oran Agra <[email protected]>
@enjoy-binbin enjoy-binbin mentioned this pull request Jun 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@itamarhaber itamarhaber itamarhaber approved these changes

@oranagra oranagra oranagra left review comments

Assignees

No one assigned

Labels

cpu efficiency state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ZREVRANGEBYSCORE] When the offset is too large, the query is very slow.

4 participants

@omigafu @antirez @oranagra @itamarhaber