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.

Sign up for GitHub

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

Jump to bottom

Support GEOSEARCH and GEOSEARCHSTORE commands #2677

Closed
wants to merge 26 commits into from
Closed

Support GEOSEARCH and GEOSEARCHSTORE commands #2677

wants to merge 26 commits into from

Conversation

AvitalFineRedis
Copy link
Contributor

@AvitalFineRedis AvitalFineRedis commented Oct 25, 2021 •
edited

Support GEOSEARCH and GEOSEARCHSTORE commands.
Also created GeoSearchParam class to support all the params for both commands.

@AvitalFineRedis AvitalFineRedis linked an issue Oct 25, 2021 that may be closed by this pull request
GEOSEARCH/GEOSEARCHSTORE commands for bounding box spatial queries (#8094) #2390
Closed
@AvitalFineRedis AvitalFineRedis changed the title Geosearch geosearchstore Support GEOSEARCH and GEOSEARCHSTORE commands Oct 25, 2021
sazzad16
sazzad16 requested changes Oct 25, 2021
View reviewed changes
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

Please keep the old methods.

@AvitalFineRedis AvitalFineRedis marked this pull request as draft October 25, 2021 13:15
@AvitalFineRedis AvitalFineRedis marked this pull request as ready for review October 26, 2021 14:25
@codecov-commenter
Copy link

codecov-commenter commented Oct 26, 2021 •
edited

Codecov Report

Merging #2677 (56613d9) into master (253664f) will decrease coverage by 0.38%.
The diff coverage is 44.92%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2677      +/-   ##
============================================
- Coverage     59.39%   59.01%   -0.39%     
- Complexity     2586     2646      +60     
============================================
  Files           133      134       +1     
  Lines         11995    12318     +323     
  Branches        564      580      +16     
============================================
+ Hits           7125     7270     +145     
- Misses         4640     4807     +167     
- Partials        230      241      +11
Impacted Files Coverage Δ
src/main/java/redis/clients/jedis/BinaryJedis.java 79.14% <0.00%> (-2.01%) ⬇️
...n/java/redis/clients/jedis/BinaryJedisCluster.java 9.65% <0.00%> (-0.37%) ⬇️
...n/java/redis/clients/jedis/BinaryShardedJedis.java 4.94% <0.00%> (-0.12%) ⬇️
...rc/main/java/redis/clients/jedis/JedisCluster.java 5.04% <0.00%> (-0.19%) ⬇️
...java/redis/clients/jedis/MultiKeyPipelineBase.java 9.42% <0.00%> (-0.70%) ⬇️
...rc/main/java/redis/clients/jedis/PipelineBase.java 10.46% <0.00%> (-0.27%) ⬇️
...rc/main/java/redis/clients/jedis/ShardedJedis.java 7.53% <0.00%> (-0.18%) ⬇️
...is/clients/jedis/commands/BinaryJedisCommands.java 50.00% <ø> (ø)
...is/clients/jedis/commands/BinaryRedisPipeline.java 0.00% <ø> (ø)
...va/redis/clients/jedis/commands/JedisCommands.java 100.00% <ø> (ø)
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 253664f...56613d9. Read the comment docs.

@sazzad16 sazzad16 added this to the 3.8.0 milestone Oct 28, 2021
@sazzad16
Copy link
Collaborator

sazzad16 commented Nov 1, 2021

@yangbodong22011 @dengliming @gkorland @mina-asham

Should we name geosearch or geoSearch?

@yangbodong22011
Copy link
Collaborator

@yangbodong22011 @dengliming @gkorland @mina-asham

Should we name geosearch or geoSearch?

If you follow the old style (georadius, georadiusStore), it should be:
geosearch
geosearchStore

@sazzad16
Copy link
Collaborator

sazzad16 commented Nov 1, 2021

Also, what about the name of method for geosearchstore with storedist?

  • geosearchStoreStoreDist - according to Redis doc (and existing pattern)
  • geosearchStoreDist - alters existing pattern
  • geosearchStoreWithDist - meaningful but doesn't match Redis doc

@yangbodong22011
Copy link
Collaborator

Also, what about the name of method for geosearchstore with storedist?

  • geosearchStoreStoreDist - according to Redis doc (and existing pattern)
  • geosearchStoreDist - alters existing pattern
  • geosearchStoreWithDist - meaningful but doesn't match Redis doc

🤣 i vote for geosearchStoreStoreDist

yangbodong22011
yangbodong22011 reviewed Nov 4, 2021
View reviewed changes
Copy link
Collaborator

@yangbodong22011 yangbodong22011 left a comment

Choose a reason for hiding this comment

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

It seems that there is a lack of adding corresponding commands in MultiKeyPipelineBase.java and PipelineBase.java

sazzad16
sazzad16 requested changes Nov 4, 2021
View reviewed changes
Copy link
Collaborator

@sazzad16 sazzad16 left a comment

Choose a reason for hiding this comment

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

For MultiKey commands (geosearchStore...), .run(src); -> .run(2, dest, src);.
Similar for binary commands.

@sazzad16 sazzad16 modified the milestones: 3.8.0, 3.9.0 Dec 4, 2021
@AvitalFineRedis AvitalFineRedis mentioned this pull request Dec 27, 2021
Merged
@sazzad16 sazzad16 deleted the geosearch_geosearchstore branch April 12, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sazzad16 sazzad16 sazzad16 approved these changes

@dengliming dengliming dengliming approved these changes

@yangbodong22011 yangbodong22011 yangbodong22011 approved these changes

Assignees
No one assigned
Labels
ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

GEOSEARCH/GEOSEARCHSTORE commands for bounding box spatial queries (#8094)
5 participants
@AvitalFineRedis @codecov-commenter @sazzad16 @yangbodong22011 @dengliming