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

#761: Added ZADD options implementation - XX|NX|CH|INCR| LT|GT according to Redis source code #1262

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

rushabhk04
Copy link
Contributor

@rushabhk04 rushabhk04 commented Nov 9, 2024

I have implemented Redis's option parsing for the ZADD command. Additionally, I wrote unit and integration test cases to cover the functionality.

@apoorvyadav1111 Please review this and let me know if any modifications are needed. Thank you!

I added the evaluated command logic, which handles ZADD based on option flags and operates according to the Redis source code. I also created the zset_test file for HTTP, web socket, and resp, which contains test cases for the command with various combinations of options.

Fixes #761
Closed PR related to the above #1138
The existing open PR for this issue is #1205 (the reason I have created a new PR is because of a messed up history in PR 1138).

Will put the documents in the other thread to net mess things up!

Thanks for understanding and assistance!

@rushabhk04
Copy link
Contributor Author

@JyotinderSingh Can you please review the PR, thanks

@JyotinderSingh JyotinderSingh self-requested a review November 11, 2024 06:24
Copy link
Collaborator

@JyotinderSingh JyotinderSingh left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution @rushabhk04! This is a complicated feature with a lot of different permutations. Thanks for the exhaustive tests.

I have left a few comments.

@rushabhk04
Copy link
Contributor Author

@JyotinderSingh , I have updated the changes. Can you please re-review it and merge it?

Also, please let me know if any questions or suggestions. Thanks

Copy link
Collaborator

@JyotinderSingh JyotinderSingh left a comment

Choose a reason for hiding this comment

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

Thanks for working on this @rushabhk04! The changes look good.

@JyotinderSingh JyotinderSingh changed the title Added ZADD options implementation - XX|NX|CH|INCR| LT|GT according to Redis source code #761: Added ZADD options implementation - XX|NX|CH|INCR| LT|GT according to Redis source code Nov 12, 2024
@JyotinderSingh JyotinderSingh merged commit 9f9b441 into DiceDB:master Nov 12, 2024
1 check passed
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.

Add support for different options of ZADD command
2 participants