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

feat(server): Support for LIMIT and REV in ZRANGE #422 #456

Merged
merged 5 commits into from
Nov 3, 2022

Conversation

RedhaL
Copy link
Contributor

@RedhaL RedhaL commented Nov 1, 2022

Closes #422

Since the new ZRange regroups all the other (now deprecated) ZRange functions, I did some refactoring to avoid duplicating code from these functions. This made it possible to have support for ZRevRangeByLex for free, since the visitor function already supported it.
Also spotted and corrected an error in ZRangeByLex when parsing the offset parameter.

There are some unrelated formatting changes that were added by clang-format.

@romange romange requested a review from dranikpg November 2, 2022 05:03
@@ -1251,42 +1297,18 @@ void ZSetFamily::ZRangeByLex(CmdArgList args, ConnectionContext* cntx) {
return (*cntx)->SendError(kSyntaxErr);
string_view os = ArgS(args, 5);
string_view cs = ArgS(args, 6);
if (!SimpleAtoi(os, &count) || !SimpleAtoi(cs, &count)) {
if (!SimpleAtoi(os, &offset) || !SimpleAtoi(cs, &count)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks!

@RedhaL
Copy link
Contributor Author

RedhaL commented Nov 2, 2022

I made a mistake when handling parameters following the LIMIT group, the new commits should fix that

Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

Looks really solid! 🎩

It looks like ParseRangeByScoreParams has a similar issue. It doesn't allow inverse order: limit a b withscores. If possible, could you fix it as well? And add a test, that makes sure it works for both your command and deprecated ZRangeByScore

src/server/zset_family.h Outdated Show resolved Hide resolved
src/server/zset_family.cc Outdated Show resolved Hide resolved
@romange
Copy link
Collaborator

romange commented Nov 2, 2022

@RedhaL i pinged you at discord :)

dranikpg
dranikpg previously approved these changes Nov 2, 2022
Copy link
Contributor

@dranikpg dranikpg left a comment

Choose a reason for hiding this comment

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

Thanks!

@RedhaL
Copy link
Contributor Author

RedhaL commented Nov 3, 2022

@dranikpg I just added the fix for zrangebyscore as well

@dranikpg dranikpg merged commit d95de33 into dragonflydb:main Nov 3, 2022
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.

Support for LIMIT and REV in ZRANGE
3 participants