search

dragonflydb / dragonfly

A modern replacement for Redis and Memcached
https://www.dragonflydb.io/
Other
25.47k stars 926 forks source link

Add GEOPOS support #1546

Closed romange closed 1 year ago

romange commented 1 year ago

See here: https://redis.io/commands/geopos/

yoelsherwin commented 1 year ago

Hi, I would like to give it a shot :)

romange commented 1 year ago

feel free to do so after #1543 is submitted :)

romange commented 1 year ago

hi @yoelsherwin , are you going to work on this? :)

yoelsherwin commented 1 year ago

Hi @romange , yes I would like to give it a shot. As this is my first contribution to dragonfly, could you please provide some guidance on where I should begin?

romange commented 1 year ago

First step is to read https://github.com/dragonflydb/dragonfly/blob/main/CONTRIBUTING.md and setup your dev environment. we give guidance for ubuntu linux.

Then I would build zset_family_test and run it. Learn the command behavior, see how other commands are implemented inside zset_family.cc, how they are registered there, add tests for geopos and start hacking.

The task assumes knowledge of C++. if you are stumbled upon just write here and mention @kostasrim - he will help you :)

yoelsherwin commented 1 year ago

Hi @romange @kostasrim I created a test based on the example provided here: https://redis.io/commands/geopos/ I'm expecting this result:

1) 1) "13.36138933897018433"
   2) "38.11555639549629859"
2) nil

But when running it using my implementation I got a different result:

1) 1) "13.361389338970184"
   2) "38.1155563954963"
2) (nil)

It seems like it is an issue with the precision. Do you think this makes sense? I tried working with the infrastructure implemented in #1543 but for some reason, I'm getting different results compared to Redis

chakaz commented 1 year ago

Looking at the geopos example, I see that the coordinates are specified using 6 decimal digits, and the divergence in your above print happens many (6 more) digits after, so I personally don't think this is worrisome. @romange do you know if common usage of these methods compare positions with infinite precision?

romange commented 1 year ago

@yoelsherwin your results are good. We are using a differrent algorithm printing doubles but essentially these are the same doubles. Proof (python):

>>> 13.36138933897018433 == 13.361389338970184
True
>>> 38.11555639549629859 == 38.1155563954963
True
yoelsherwin commented 1 year ago

Ok, thanks @romange ! I will go over @chakaz comments today and will submit another review soon