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.

Sign up for GitHub

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

Jump to bottom

Add the support of the ZMPOP command #1468

Merged
merged 7 commits into from
May 26, 2023
Merged

Add the support of the ZMPOP command #1468

merged 7 commits into from
May 26, 2023

Conversation

MizukiCry
Copy link
Contributor

@MizukiCry MizukiCry commented May 23, 2023 •
edited
Loading

This PR supports the ZMPOP command like Redis.

  • Syntax
    ZMPOP numkeys key [key ...] <MIN | MAX> [COUNT count]
  • Complexity
    Worstly O(K) + O(M*log(N))
    K: numkeys
    N: the number of elements in the zset
    M: the number of elements popped.

It returns nil if no elements popped.

Close #1458

By the way, do I need to add test cases for it? Similar commands like ZPOPMIN / MAX also don't have corresponding test cases.

@MizukiCry
Add the support of the ZMPOP command
974d99a 
Signed-off-by: MizukiCry <YuukaC@outlook.com>
@torwig
Copy link
Contributor

torwig commented May 23, 2023

@MizukiCry Thank you for your contribution. Unfortunately, there are some functions that don't have test coverage. However, we are thriving to test all the new features and changes so it would be great if you provide test cases.

@xiaobiaozhao
Copy link
Contributor

great job!

  1. Please add some test case
  2. Please explain the difference with the redis official ZMPOP command
    1. syntax
    2. Complexity

@PragmaTwice
Copy link
Member

do I need to add test cases for it?

Yeah, it is definitely an efficient way to prove to us that your code can work well.

ZPOPMIN / MAX also don't have corresponding test cases.

You are welcome to add them if you want.

@MizukiCry
Add test cases for ZPOPMIN, ZPOPMAX and ZMPOP
2d0312e 
Signed-off-by: MizukiCry <YuukaC@outlook.com>
@MizukiCry
Improve ZMPOP key range
5548b07 
Signed-off-by: MizukiCry <YuukaC@outlook.com>
@infdahai
Copy link
Contributor

Issue number: #1458

Please use close or fixes. The related site is https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

torwig
torwig previously approved these changes May 24, 2023
View reviewed changes
Copy link
Contributor

@torwig torwig left a comment

Choose a reason for hiding this comment

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

Generally, LGTM.
@MizukiCry Good job!
Many thanks to @PragmaTwice for reviewing and helping with this PR.

@MizukiCry
Copy link
Contributor Author

Issue number: #1458

Please use close or fixes. The related site is https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

Fixed.

git-hulk
git-hulk reviewed May 26, 2023
View reviewed changes
@git-hulk
Copy link
Member

Others are good to me

@MizukiCry
Use enum to represent min / max state
10b7074 
Signed-off-by: MizukiCry <YuukaC@outlook.com>
@git-hulk git-hulk requested a review from torwig May 26, 2023 07:06
git-hulk
git-hulk approved these changes May 26, 2023
View reviewed changes
Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution.

@git-hulk
Copy link
Member

Thanks all, merging...

@git-hulk git-hulk merged commit b719f03 into apache:unstable May 26, 2023
@git-hulk
Copy link
Member

Thanks @MizukiCry again.

@infdahai infdahai mentioned this pull request May 26, 2023
Closed
9 tasks
enjoy-binbin added a commit to enjoy-binbin/kvrocks that referenced this pull request Jul 5, 2023
@enjoy-binbin
Fix ZMPOP/BZMPOP duplicate parameters
db436db 
We did not check for repeated parameters before, so the
following code is executable:
```
127.0.0.1:6666> zmpop 1 zset min max
(nil)
127.0.0.1:6666> zmpop 1 zset min count 10 count 100
(nil)
127.0.0.1:6666> zmpop 1 zset min min max max min
(nil)
127.0.0.1:6666> zmpop 1 zset min count 10 count 100 max
(nil)
127.0.0.1:6666> bzmpop 0.1 1 zset min max
(nil)
127.0.0.1:6666> bzmpop 0.1 1 zset min count 1 count 10
(nil)
```

Now we don't allow duplicate parameters (throw a syntax error):
```
127.0.0.1:6666> zmpop 1 zset min max
(error) ERR syntax error
127.0.0.1:6666> zmpop 1 zset min count 10 count 100
(error) ERR syntax error
127.0.0.1:6666> zmpop 1 zset min min max max min
(error) ERR syntax error
127.0.0.1:6666> zmpop 1 zset min count 10 count 100 max
(error) ERR syntax error
127.0.0.1:6666> bzmpop 0.1 1 zset min max
(error) ERR syntax error
127.0.0.1:6666> bzmpop 0.1 1 zset min count 1 count 10
(error) ERR syntax error
```

Also added some tests to cover these wrong error paths.
Refs: ZMPOP was added in apache#1468 and BZMPOP was added in apache#1490.
@enjoy-binbin enjoy-binbin mentioned this pull request Jul 5, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@git-hulk git-hulk git-hulk approved these changes

@torwig torwig torwig approved these changes

@PragmaTwice PragmaTwice Awaiting requested review from PragmaTwice

@infdahai infdahai Awaiting requested review from infdahai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add the support of the ZMPOP command
6 participants
@MizukiCry @torwig @xiaobiaozhao @PragmaTwice @infdahai @git-hulk