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

implement GEORADIUS #1878

Open
romange opened this issue Sep 18, 2023 · 11 comments
Open

implement GEORADIUS #1878

romange opened this issue Sep 18, 2023 · 11 comments
Assignees
Labels
enhancement New feature or request hacktoberfest Participates in Hacktoberfest help wanted Extra attention is needed

Comments

@romange
Copy link
Collaborator

romange commented Sep 18, 2023

see https://redis.io/commands/georadius/ for details

@romange romange added enhancement New feature or request help wanted Extra attention is needed hacktoberfest Participates in Hacktoberfest labels Sep 18, 2023
@yoelsherwin
Copy link
Contributor

Hi @romange , I would like to give this a go. Were you thinking of implementing the georadius in its generic way like in redis? Or starting by something simple and making it generic in the future?

@romange
Copy link
Collaborator Author

romange commented Sep 21, 2023

Hey @yoelsherwin ! What do you mean by a generic way like in Redis? if you ask whether we must support all the options from the start then no, we do not need to. as long as your code brings us closer, you can choose the scope of your PR.

@yoelsherwin
Copy link
Contributor

I mean that in Redis, geoRadius (along with other commands) is basically just calling the georadiusGeneric with different parameters. I was not sure if that the right way to implement it (generalizing where we don't need it yet), but since the end goal is known maybe this is not such a bad idea. Would love to hear your thoughts :)

@romange
Copy link
Collaborator Author

romange commented Sep 26, 2023

I have not looked into the code. 1. we can not use georadiusGeneric unfortunately, because it uses client* which we do not have. Therefore it needs to be reimplemented. I would not be bothered at this point with "generic" implementation fitting all the options/usecases, and I would choose a specific option set at API level and implement only it at first. Without STORE, for example. Only for M, only WITHCOORD etc.

@romange
Copy link
Collaborator Author

romange commented Sep 26, 2023

basically an MVP which can gradually grow

@yoelsherwin
Copy link
Contributor

Yeah, I was aware that we need to reimplement it, just wasn't sure at what scope. Anyway, got the answer I was looking for :) Thanks!
Will probably get to it around next week

@yoelsherwin
Copy link
Contributor

hi @romange , I started working on this but I won't be able to finish this soon due to personal reasons. Sorry. hope I could contribute again soon 🙏

@romange
Copy link
Collaborator Author

romange commented Feb 13, 2024

Hi @yoelsherwin hope everything is all right and thanks again for your contributions!

@azuredream
Copy link
Contributor

Hi @romange ,
According to https://redis.io/docs/latest/commands/georadius/, GEORADIUS can be replaced by GEOSEARCH with BYRADIUS option.
If you still want this, please assign it to me.
I think I just need to make some changes based on the impl of GEOSEARCH.

@romange
Copy link
Collaborator Author

romange commented Sep 20, 2024

Yes, we must have a single implementation and probably have GEORADIUS wrapping GEOSEARCH.

@azuredream
Copy link
Contributor

Hi @romange , PR created.
#3756

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest Participates in Hacktoberfest help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants