DiceDB
Add support for command `GEORADIUSBYMEMBER_RO`
Add support for the GEORADIUSBYMEMBER_RO command in DiceDB similar to the GEORADIUSBYMEMBER_RO command in Redis. Please refer to the following commit in Redis to understand the implementation specifics - source.
Write unit and integration tests for the command referring to the tests written in the Redis codebase 7.2.5. For integration tests, you can refer to the tests folder. Note: they have used TCL for the test suite, and we need to port that to our way of writing integration tests using the relevant helper methods. Please refer to our tests directory.
For the command, benchmark the code and measure the time taken and memory allocs using benchmem and try to keep them to the bare minimum.
Hi @iamskp11 i believe you already have issue in your hand https://github.com/DiceDB/dice/issues/1133 can you please let me take up this issue
Hi @iamskp11 , Please let me know regarding your interest in taking up this issue and address @tarun-29 's comment. Thanks
Hi @iamskp11 i believe you already have issue in your hand https://github.com/DiceDB/dice/issues/1133 can you please let me take up this issue
@tarun-29 That's in review stage, almost done. Will close soon.
Hi @iamskp11 , Please let me know regarding your interest in taking up this issue and address @tarun-29 's comment. Thanks Hey @apoorvyadav1111 , replied. Will get this done. Please assign.
Okay @apoorvyadav1111 can you please assign me any new issue if created next time
Thanks
Hey @apoorvyadav1111 , can we make this as dependent on #1252 ? I can implement it independently, but with some modifications in the original GEORADIUSBYMEMBER, we can implement read_only variant. Similar to what we did with BITFIELD and BITFIELD_RO. Or, you may assign both commands to me, will get it done.
If you see the Redis Src Code all the 6 commands from #1250 - #1255 have common generic function ref that is being used ideally one command should implement the generic API and should be checked in first . So others can augument it ig.
If you see the Redis Src Code all the 6 commands from #1250 - #1255 have common generic function ref that is being used ideally one command should implement the generic API and should be checked in first . So others can augument it ig.
True, all georadius commands into one, similar to redis, will make code cleaner.
Hey folks,
I followed your discussion since I'm currently working on #1252 and was wondering the same thing. In my opinion, the generic function in Redis is pretty hard to read. It's more than 300 loc and has a lot of nested conditional clauses, especially when handling the different flags in combination with the individual commands. See i.e. these bad boys here.
It might just be my personal preference, but since we have to port it to Go anyway, we could break this up. Certain parts can be extracted and re-used by each command implementation. As @iamskp11 suggested, we could make those PRs dependent on each other to avoid duplicate work.
In my opinion, the generic function in Redis is pretty hard to read. It's more than 300 loc and has a lot of nested conditional clauses, especially when handling the different flags in combination with the individual commands.
Agree could be broken into like different parts like option parsing , finding neighbours , etc but then we would need to pass around additional context between mutliple functions , which seems unnecessary to me , but readability wise makes sense. or we can have each command do its own option parsing and pass the context to generic.
Since I'm halfway through implementing #1252, how about I add you as a reviewer? Then we can discuss whether we want to extract a generic function or structure it differently. It should be easier to consult with a first draft.