feat: Add GEOSEARCH support #2070

azuredream · 2023-10-25T04:27:03Z

Signed-off-by: azuredream <[email protected]>

chakaz

Thank you so much for your contribution!
Please see some comments below, also feel free to point out anything I might have missed

src/redis/geo.c

chakaz · 2023-10-25T06:11:14Z

src/server/zset_family.cc

@@ -48,6 +54,20 @@ using MScoreResponse = std::vector<std::optional<double>>;
 using ScoredMember = std::pair<std::string, double>;
 using ScoredArray = std::vector<ScoredMember>;

+typedef struct GeoPoint {


Suggested change

typedef struct GeoPoint {

struct GeoPoint {

This is c-syntax, we're in a C++ file :)

chakaz · 2023-10-25T06:11:25Z

src/server/zset_family.cc

+           const std::string& _member)
+      : longitude(_longitude), latitude(_latitude), dist(_dist), score(_score), member(_member){};
+
+} GeoPoint;


Suggested change

} GeoPoint;

};

chakaz · 2023-10-25T06:16:34Z

src/server/zset_family.cc

+  double dist;
+  double score;
+  std::string member;
+  GeoPoint() = default;


This will not zero the double members of the struct.
I suggest removing both constructors, and add default initializers (e.g. double logitude = 0.0;).
As for the c'tor accepting arguments, one could initialize the struct with GeoPoint{a, b, c, ...} or even better GeoPoint{.logitude=a, .latitude=b, ...}

chakaz · 2023-10-25T06:39:33Z

src/server/zset_family.cc

@@ -2599,6 +2636,296 @@ void ZSetFamily::GeoDist(CmdArgList args, ConnectionContext* cntx) {
                             distance_multiplier);
 }

+// Search all eight neighbors + self geohash box
+int membersOfAllNeighbors(ConnectionContext* cntx, string_view key, const GeoHashRadius* n,


Please:

Capitalize the first character (MembersOfAllNeighbors)

Add anonymous namespace wrapper around this function for internal linkage (i.e. add namespace { before and } // namespace after

chakaz · 2023-10-25T08:10:18Z

src/server/zset_family.cc

+  bool withhash = false;
+
+  // parse arguments
+  size_t i = 1;


Move i to be defined as part of the loop?

src/server/zset_family.cc

chakaz · 2023-10-25T08:13:18Z

src/server/zset_family.cc

+      }
+    } else if (cur_arg == "COUNT") {
+      if (i + 1 < args.size()) {
+        // TODO: need a string_view impl of stoull


absl::SimpleAtoi()

chakaz · 2023-10-25T08:13:57Z

src/server/zset_family.cc

+      withhash = true;
+    else {
+      return (*cntx)->SendError(kSyntaxErr);
+      break;


No need for break after return :)

chakaz · 2023-10-25T08:14:58Z

src/server/zset_family.cc

+  auto asc_comparator = [](const GeoPoint& a, const GeoPoint& b) { return a.dist < b.dist; };
+  auto desc_comparator = [](const GeoPoint& a, const GeoPoint& b) { return a.dist > b.dist; };


Can you reduce the scope of asc_comparator and desc_comparator to the smallest possible?

azuredream · 2023-10-26T14:19:07Z

Hi @chakaz. Thanks a lot for your review. I haven't clean it up when I submitted it as draft. Your insights saved me a lot of time.

I'll accept most of your comments but have question about two: using neighbor vector, and const ref shape.
I'll have to rewrite(copy from geo.c and change the input type) almost all the GEOSEARCH related functions. I want to confirm with you before I proceed.

Signed-off-by: azuredream <[email protected]>

chakaz · 2023-10-26T16:42:34Z

Hi @chakaz. Thanks a lot for your review. I haven't clean it up when I submitted it as draft. Your insights saved me a lot of time.

I'll accept most of your comments but have question about two: using neighbor vector, and const ref shape. I'll have to rewrite(copy from geo.c and change the input type) almost all the GEOSEARCH related functions. I want to confirm with you before I proceed.

Gladly, thank you for your contribution!
But I'm not sure what your questions are?
If you accept a T& t and need to pass it to a function accepting a pointer, you could call if via foo(&t), but I'm not sure if this is what you're asking?

romange · 2023-10-26T16:53:20Z

If accepting constref shape requires copying functions from geo.c that otherwise we could use from there then you should use mutable pointer - I prefer avoid copying. Another option is cheating and use const_cast<...> to remove constness inside your function before calling redis functions. it's also fine.

Signed-off-by: azuredream <[email protected]>

src/server/zset_family.cc

azuredream · 2023-10-26T20:57:40Z

src/server/zset_family.h

@@ -48,6 +48,8 @@ class ZSetFamily {
  struct ZRangeSpec {
    std::variant<IndexInterval, ScoreInterval, LexInterval, TopNScored> interval;
    RangeParams params;
+    ZRangeSpec() = default;
+    ZRangeSpec(const ScoreInterval& si, const RangeParams& rp) : interval(si), params(rp){};


Required by emplace_back().

azuredream · 2023-10-26T21:18:57Z

Thanks for your advice. const_cast sounds good.
I've updated my PR. I'd appreciate it if you could review the updated PR when you have some time.

To-do list for future developments:

Remove the intermediate result result_arrays.
Refine the distance precision to 4 decimal places
Improve the precision for longitude and latitude values.
Redis:

1. "Berlin"
2. "0.0002"
3. 1. "13.40500205755233765"
  2. "52.51999907056681138"
    DFDB:
1. "Berlin"
2. "0.00017343178521311378"
3. 1. "13.405002057552338"
  2. "52.51999907056681"

Once this PR is merged, implementing #1548 and #1878 should be straightforward. Please feel free to assign them to me if you need.

Signed-off-by: azuredream <[email protected]>

chakaz · 2023-10-26T16:39:41Z

src/server/zset_family.cc

  double longitude;
  double latitude;
  double dist;
  double score;
  std::string member;
-  GeoPoint() = default;
+  GeoPoint() : longitude(0.0), latitude(0.0), dist(0.0), score(0.0), member(""){};


Nits:

It's slightly better to use initializers when defining the members (like double logitude = 0.0;) instead of here. The reason is that future c'tors will work with 0s by default, unless modified

No need to initialize member as it's a class, its c'tor will be called

I removed member(""). Set it just for consistency.
A default constructor is required by ga.resize(), I can't remove GeoPoint():
../src/server/zset_family.cc:2739:21: required from here
/usr/include/c++/9/bits/stl_construct.h:75:7: error: no matching function for call to ‘dfly::{anonymous}::GeoPoint::GeoPoint()’

src/redis/geo.c

chakaz · 2023-10-29T03:01:00Z

src/server/zset_family.cc

+// Search all eight neighbors + self geohash box
+int MembersOfAllNeighbors(ConnectionContext* cntx, string_view key, const GeoHashRadius& n,
+                          const GeoShape& shape_ref, GeoArray* ga, unsigned long limit) {
+  vector<GeoHashBits> neighbors(9);


Because you know the size in advance, you could use std::array<GeoHashBits, 9> instead of a vector. It would save a dynamic memory allocation

chakaz · 2023-10-29T03:12:21Z

src/server/zset_family.cc

+  GeoShape* shape = &(const_cast<GeoShape&>(shape_ref));
+  GeoHashRadius georadius = geohashCalculateAreasByShapeWGS84(shape);
+  GeoArray ga;
+  MembersOfAllNeighbors(cntx, key, georadius, shape_ref, &ga, any ? count : 0);


This function will reply to the client in case of error, so you have to check its return value here. We can't reply twice.
Further, it seems like you only case about the error case of the function, so no need to calculate and return count there

I modified MembersOfAllNeighbors to return a bool value. It would return false if already replied inside.

chakaz · 2023-10-29T03:13:28Z

src/server/zset_family.cc

+
+  // if no matching results, the user gets an empty reply.
+  if (ga.empty()) {
+    (*cntx)->SendNull();


return after replying

chakaz · 2023-10-29T03:14:54Z

src/server/zset_family.cc

+  double conversion = geo_ops.conversion;
+  uint64_t count = geo_ops.count;
+  bool asc = geo_ops.asc;
+  bool desc = geo_ops.desc;
+  bool any = geo_ops.any;
+  bool withdist = geo_ops.withdist;
+  bool withcoord = geo_ops.withcoord;
+  bool withhash = geo_ops.withhash;
+


I'd remove all of these variables, they don't really help but make this function more bloat. You could just use geo_ops.* instead.
For example, conversion is only used once (with a fairly short line)

chakaz · 2023-10-29T03:17:32Z

src/server/zset_family.cc

+  bool asc = 0;
+  bool desc = 0;


Maybe add an enum like:

enum class Sorting { kUnsorted, kAsc, kDesc };

And replace these members with Sorting sorting;?

This would be easier to read than a bunch of booleans, and also will only represent valid cases (with this code - both could theoretically be true, which doesn't make sense)

chakaz · 2023-10-29T03:26:42Z

src/server/zset_family.cc

+    if (count > 0) {
+      std::partial_sort(ga.begin(), ga.begin() + count, ga.end(), asc_comparator);
+      ga.resize(count);
+    } else {
+      std::sort(ga.begin(), ga.end(), asc_comparator);
+    }


Can you create a function (in the anonymous namespace) with this, which accepts a comparator (probably of type absl::FunctionRef<bool(const GeoPoint&, const GeoPoint&)>)?
This way you both shorten this function, and remove code copy-paste

An alternative would be to create a function which will take the above-mentioned enum and sort internally. Something like:

void SortIfNeeded(GeoArray* ga, Sorting sorting) { if (sorting == kUnsorted) return; auto comparator = [&](const GeoPoint& a, const GeoPoint& b) { if (sorting == Sorting::kAsc) { return a.dist < b.dist; } else { DCHECK(sorting == Sorting::kDesc); return a.dist > b.dist; } }; if (count > 0) { std::partial_sort(ga.begin(), ga.begin() + count, ga.end(), comparator); ga.resize(count); } else { std::sort(ga.begin(), ga.end(), comparator); } }

I created SortIfNeeded as you suggested:)

src/server/zset_family.cc

Signed-off-by: azuredream <[email protected]>

azuredream · 2023-10-30T17:48:12Z

PR updated. Thanks again for your review and suggestions.

chakaz

Woohoo, thank you again for your contribution!
(and for withstanding my comments :)

romange · 2023-10-31T12:09:53Z

thank you @azuredream !

piyongcai-liucai · 2024-05-20T01:49:31Z

FULL SUPPORT?

feat: Add GEOSEARCH support

6076051

Signed-off-by: azuredream <[email protected]>

romange requested a review from chakaz October 25, 2023 06:02

chakaz reviewed Oct 25, 2023

View reviewed changes

feat: Add GEOSEARCH support

b622f3e

Signed-off-by: azuredream <[email protected]>

feat: Add GEOSEARCH support

d2b44d8

Signed-off-by: azuredream <[email protected]>

azuredream commented Oct 26, 2023

View reviewed changes

src/server/zset_family.cc Show resolved Hide resolved

azuredream commented Oct 26, 2023

View reviewed changes

azuredream added 2 commits October 26, 2023 17:27

feat: Add GEOSEARCH support

8154ecb

Signed-off-by: azuredream <[email protected]>

feat: Add GEOSEARCH support

c3cd3d7

Signed-off-by: azuredream <[email protected]>

azuredream marked this pull request as ready for review October 27, 2023 13:50

chakaz reviewed Oct 29, 2023

View reviewed changes

feat: Add GEOSEARCH support

e472a0c

Signed-off-by: azuredream <[email protected]>

romange requested a review from chakaz October 31, 2023 07:22

chakaz approved these changes Oct 31, 2023

View reviewed changes

chakaz merged commit 05919ef into dragonflydb:main Oct 31, 2023
7 checks passed

chakaz mentioned this pull request Oct 31, 2023

implement GEOSEARCH #1879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add GEOSEARCH support #2070

feat: Add GEOSEARCH support #2070

azuredream commented Oct 25, 2023

chakaz left a comment

chakaz Oct 25, 2023

chakaz Oct 25, 2023

chakaz Oct 25, 2023

chakaz Oct 25, 2023

chakaz Oct 25, 2023

chakaz Oct 25, 2023

chakaz Oct 25, 2023

chakaz Oct 25, 2023

azuredream commented Oct 26, 2023

chakaz commented Oct 26, 2023

romange commented Oct 26, 2023 •

edited

Loading

azuredream Oct 26, 2023 •

edited

Loading

azuredream commented Oct 26, 2023 •

edited

Loading

chakaz Oct 26, 2023

azuredream Oct 30, 2023

chakaz Oct 29, 2023

chakaz Oct 29, 2023

azuredream Oct 30, 2023

chakaz Oct 29, 2023

chakaz Oct 29, 2023

chakaz Oct 29, 2023

chakaz Oct 29, 2023

azuredream Oct 30, 2023

azuredream commented Oct 30, 2023

chakaz left a comment

romange commented Oct 31, 2023

piyongcai-liucai commented May 20, 2024

		auto asc_comparator = [](const GeoPoint& a, const GeoPoint& b) { return a.dist < b.dist; };
		auto desc_comparator = [](const GeoPoint& a, const GeoPoint& b) { return a.dist > b.dist; };

feat: Add GEOSEARCH support #2070

feat: Add GEOSEARCH support #2070

Conversation

azuredream commented Oct 25, 2023

chakaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azuredream commented Oct 26, 2023

chakaz commented Oct 26, 2023

romange commented Oct 26, 2023 • edited Loading

azuredream Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

azuredream commented Oct 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azuredream commented Oct 30, 2023

chakaz left a comment

Choose a reason for hiding this comment

romange commented Oct 31, 2023

piyongcai-liucai commented May 20, 2024

romange commented Oct 26, 2023 •

edited

Loading

azuredream Oct 26, 2023 •

edited

Loading

azuredream commented Oct 26, 2023 •

edited

Loading