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

Add support for command GEORADIUSBYMEMBER_RO #1253

Open
arpitbbhayani opened this issue Nov 7, 2024 · 13 comments
Open

Add support for command GEORADIUSBYMEMBER_RO #1253

arpitbbhayani opened this issue Nov 7, 2024 · 13 comments
Assignees

Comments

@arpitbbhayani
Copy link
Contributor

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.

@iamskp11
Copy link
Contributor

iamskp11 commented Nov 7, 2024

Hey, would like to work on this. Please assign!

@tarun-29
Copy link
Contributor

tarun-29 commented Nov 8, 2024

Hi @iamskp11 i believe you already have issue in your hand #1133 can you please let me take up this issue

@apoorvyadav1111
Copy link
Contributor

Hi @iamskp11 , Please let me know regarding your interest in taking up this issue and address @tarun-29 's comment.
Thanks

@iamskp11
Copy link
Contributor

iamskp11 commented Nov 9, 2024

Hi @iamskp11 i believe you already have issue in your hand #1133 can you please let me take up this issue

@tarun-29 That's in review stage, almost done. Will close soon.

@iamskp11
Copy link
Contributor

iamskp11 commented Nov 9, 2024

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.

@tarun-29
Copy link
Contributor

tarun-29 commented Nov 9, 2024

Okay @apoorvyadav1111 can you please assign me any new issue if created next time

Thanks

@apoorvyadav1111
Copy link
Contributor

Hi @iamskp11, assigned.

@iamskp11
Copy link
Contributor

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.

@shah-nirmit
Copy link

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.

@iamskp11
Copy link
Contributor

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.

@benbarten
Copy link

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.

@shah-nirmit
Copy link

shah-nirmit commented Nov 13, 2024

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.

@benbarten
Copy link

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.

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

No branches or pull requests

6 participants