#1130: Adds support for command ZCOUNT

DiceDB / dice

DiceDB is a redis-compliant, reactive, scalable, highly-available, unified cache optimized for modern hardware.

https://dicedb.io/

Other

6.83k stars 1.08k forks source link

#1130: Adds support for command ZCOUNT #1171

Closed tarun-29 closed 4 weeks ago

tarun-29 commented 1 month ago

This PR closes (https://github.com/DiceDB/dice/issues/1130) issue.

Changes made:

[x] Added support for ZCOUNT command.
[x] Added Unit tests, Integration tests for RESP/HTTP
[x] Added Benchmark function for this command.
[x] Added Documentation for this command.

This is my first PR for dicDB. I've done my best to understand the existing code and compared everything with Redis commands. I've also included benchmark functions, but I'm unsure about the implementation. I would appreciate any feedback or corrections on what might be wrong. Please review this PR!

Attaching benchmark screenshots

tarun-29 commented 1 month ago

@JyotinderSingh @lucifercr07 please review this PR

apoorvyadav1111 commented 1 month ago

Hi, Thanks for contributing and raising this PR to close an important issue. Meanwhile, if you are unsure about the PR being ready, you can check the list below:

For Reviewer's Checklist for Migration label PR/Issues

[x] moved code from eval.go to store_eval.go
[x] updated cmd meta in eval, worker & server
[x] moved tests from async to resp
[x] added/updated tests in http and websocket
[x] added/updated unit tests in eval folder
[x] audited documentation and updated as per new standards
[x] no changes in legacy async code / eval.go file
[x] rebased with master
[x] verified the code in local

apoorvyadav1111 commented 1 month ago

On the quick look, I think we need to add tests in websocket folder as well.

tarun-29 commented 1 month ago

On the quick look, I think we need to add tests in websocket folder as well.

This is resolved for zcount command @JyotinderSingh @apoorvyadav1111 @lucifercr07

apoorvyadav1111 commented 1 month ago

Hi @tarun-29 , please check the linter run failed logs. Additionally, you can try running lint checks in local for any modifications.

tarun-29 commented 1 month ago

Hi @tarun-29 , please check the linter run failed logs. Additionally, you can try running lint checks in local for any modifications.

I have resolved the lint issues @apoorvyadav1111

tarun-29 commented 4 weeks ago

@apoorvyadav1111 please check i have resolved some of the comments and posted doubts as well

apoorvyadav1111 commented 4 weeks ago

Please check for lint run in you local, once it passes the checks we are good to merge this PR, @tarun-29 .

tarun-29 commented 4 weeks ago

Please check for lint run in you local, once it passes the checks we are good to merge this PR, @tarun-29 .

This is done @apoorvyadav1111