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 GeoRadiusByMember support #2107

Merged
merged 3 commits into from
Nov 7, 2023
Merged

Conversation

azuredream
Copy link
Contributor

Signed-off-by: azuredream <zhaozixuan67@gmail.com>
return OpStatus::OK;
};
cntx->transaction->Schedule();
cntx->transaction->Execute(std::move(store_cb), true);
Copy link
Contributor Author

@azuredream azuredream Nov 2, 2023

Choose a reason for hiding this comment

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

I'm not sure which one should I use here. schedule-execute or ScheduleSingleHopT? It seems that I have to set "-4, 3, 3, 1" in Register to use schedule-execute. But got error message:
127.0.0.1:6379> GEORADIUSBYMEMBER Europe Madrid 700 KM WITHCOORD WITHDIST STORE test1
Error: Server closed the connection

The search part works as expected if I set "-4, 1, 1, 1" in Register and use ScheduleSingleHopT to get latitude and longitutde of a member.

I'd appreciate it if anyone could give me some advice.

Copy link
Collaborator

@romange romange Nov 2, 2023

Choose a reason for hiding this comment

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

Please do not add support for STORE and STOREDIST options. There are several reasons for this:

  1. it requires some adjustments to our transactional framework to support this non-standard command layout
  2. this also requires advanced knowledge of how to build such commands.

Having said that, implementing all the other options will provide substantiation contribution and will make it easier for us to close the gaps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your suggestion. I've updated my PR.

I'll be glad to make further changes to transactional framework if you could give me some suggestions.

Another thing on my to-do list is that GeoSearch fetches redundant data and subsequently filter them. To eliminate the intermediate result array, also requires some change to the standard search process which cannot take extra geo filter as input.

Signed-off-by: azuredream <zhaozixuan67@gmail.com>
Signed-off-by: azuredream <zhaozixuan67@gmail.com>
@azuredream azuredream marked this pull request as ready for review November 6, 2023 01:02
@romange
Copy link
Collaborator

romange commented Nov 7, 2023

@azuredream if you want, please add disabled unit tests for the missing features (see GTEST_SKIP()).
This way I will be able to see what needs to be done for the test to pass and I will be able to guide you.

@romange romange merged commit 6472bcf into dragonflydb:main Nov 7, 2023
7 checks 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.

None yet

2 participants