feat: Store and diststore for GeoRadiusByMember #2350

azuredream · 2023-12-28T19:08:21Z

Uses Schedule-Execute in GeoRadiusByMember workflow instead of ScheduleSingleHopT()
Registers command in WRITE mode instead of READ mode.
Write if needed
Implement georadiusbymember #1548

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

azuredream · 2023-12-28T19:20:19Z

src/server/zset_family.cc

    return cntx->SendError(kWrongTypeErr);
-  } else if (!result) {
+  } else if (!member_score) {
+    cntx->transaction->Conclude();


member_score is null and fail intermittently as mentioned here: #1548 (comment)

dataset:
geoadd Europe 13.4050 52.5200 Berlin 3.7038 40.4168 Madrid 9.1427 38.7369 Lisbon 2.3522 48.8566 Paris 16.3738 48.2082 Vienna 4.8952 52.3702 Amsterdam 10.7522 59.9139 Oslo 23.7275 37.9838 Athens 19.0402 47.4979 Budapest 6.2603 53.3498 Dublin

Not sure if I have registered this command correctly.

src/server/zset_family.cc

romange · 2023-12-30T11:45:47Z

src/server/zset_family.cc

+
+    rb->SendLong(smvec.size());
+  }
+  if (write_mode) {


I do not think you need this conclude step in the normal flow that does not expect errors. You always know what's your last hop is and that should have Execute(..., true)

The last hop could vary. When STORE is not used, the last hop would be hop 2.
If I can't use conclude in this way, I'll have to add a argument "do_store" for the whole workflow.

Also, after hop 2, every conclude() would have to check do_store. If already called conclude after hop 2, skip and return.

There are at most 3 hops
hop1. Getting latitude longitude from member name
hop2. Getting nearby nodes around <latitude ,longitude >
hop3. write to store key if needed

Conclude is also a hop. Seem my previous comment at line 2947

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

azuredream · 2023-12-31T17:16:37Z

Now the code works as expected.
Will refine error messages and add more test cases.

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

azuredream · 2024-01-01T08:43:15Z

According to Redis' behavior, refined the error messages. Added more test cases.
Ready for review.

romange

Thanks! I gave some styling/coding comments. Please take a look

romange · 2024-01-02T17:26:50Z

src/server/zset_family.cc

+}
+
+OpResult<double> OpScore(const OpArgs& op_args, string_view key, string_view member,
+                         bool use_member_not_found = false) {


Please remove the argument use_member_not_found, imho, this is not a good practice to dynamically change function behavior in such way. Instead adapt all the callers of the function to cope with MEMBER_NOTFOUND if needed.

romange · 2024-01-02T17:28:54Z

src/server/zset_family.cc

@@ -1609,7 +1612,14 @@ OpResult<unsigned> OpRem(const OpArgs& op_args, string_view key, ArgSlice member
  return deleted;
 }

-OpResult<double> OpScore(const OpArgs& op_args, string_view key, string_view member) {
+OpResult<double> OpKeyExisted(const OpArgs& op_args, string_view key) {


seems you never return double value from this function. something is off here.

romange · 2024-01-02T17:33:35Z

src/server/zset_family.cc

+//              False: Using transaction->ScheduleSingleHopT()
+OpStatus MembersOfAllNeighbors(ConnectionContext* cntx, string_view key, const GeoHashRadius& n,
+                               const GeoShape& shape_ref, GeoArray* ga, unsigned long limit,
+                               bool write_mode = false) {


again, changing function behavior using write_mode is not a good approach, imho.
Instead, I would break this function into smaller function. For example, factor out the preparation code into a separate function that just returns range_specs.

romange · 2024-01-02T17:35:53Z

src/server/zset_family.cc

-    return false;
+  vector<OpResult<vector<ScoredArray>>> result_arrays;
+  if (write_mode) {
+    auto cb = [&](Transaction* t, EngineShard* shard) {


I would move the transaction related code to the caller (GeoSearchStoreGeneric). Basically, the division to the functions is sub-optimal imho.

romange · 2024-01-02T17:41:55Z

src/server/zset_family.cc

@@ -3042,24 +3148,51 @@ void ZSetFamily::GeoRadiusByMember(CmdArgList args, ConnectionContext* cntx) {
  string_view key = ArgS(args, 0);
  // member to latlong, set shape.xy
  string_view member = ArgS(args, 1);
+  bool member_score_set = false;


Every Dragonfly command has the command parsing part and the execution part. We never start the execution before the parsing is complete and all the arguments are validated.
it seems you start the transaction execution before the parsing is completed. We should avoid it if possible - this just complicates the function flow.

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

…eam/dragonfly into GeoRadiusByMember_store2

azuredream · 2024-01-02T23:51:24Z

Thanks for reviewing.
I've updated my PR based on your suggestions.

Couldn't fully remove "write_mode", but it's only used in GeoSearchStoreGeneric(). It doesn't change function behavior a lot, just relate with using Execute() or ScheduleSingleHopT()

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

azuredream · 2024-01-08T19:29:33Z

Hi @romange
Please let me know if there is any question on this PR.
Thanks!

romange · 2024-01-08T19:46:52Z

src/server/zset_family.cc

+  auto cb = [&](Transaction* t, EngineShard* shard) {
+    return OpKeyExisted(t->GetOpArgs(shard), key);
+  };
+  OpResult<void> result = cntx->transaction->ScheduleSingleHopT(std::move(cb));


I do not think the implementation of this function is correct. You use ScheduleSingleHop method on the transaction but this method is intended for operations that require only a single hop. Here you call it multiple times - which breaks the atomicity of the whole command and probably incorrect in general. The correct way is to use Schedule() once, and then Execute(..., false) one or more times and finalize with Execute(...., true) or Conclude method in case the transaction finishes prematurely.

romange · 2024-01-08T19:49:08Z

src/server/zset_family.cc

+//             True: Using transaction->Execute()
+//             False:  Using transaction->ScheduleSingleHopT()
+// store_key:  (Valid if geo_ops.store==GeoStoreType::kNoStore) Store result to store_key
+void GeoSearchStoreGeneric(ConnectionContext* cntx, const GeoShape& shape_ref, string_view key,


this function is always called on already scheduled transactions. That means it never should call ScheduleSingleHop because this function is only intended to be used for non-scheduled transactions

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

azuredream · 2024-01-09T02:13:48Z

Updated.
That simplified the workflow a lot.

This is my first time working on async code. Appreciate your patience!

romange · 2024-01-09T08:51:21Z

src/server/zset_family.cc

+      }
+      return OpStatus::OK;
+    };
+    cntx->transaction->Execute(std::move(store_cb), false);


Again, this hop is necessary a last hop, so here you should call Execute(..., true);

romange · 2024-01-09T08:52:01Z

src/server/zset_family.cc

  }
+  cntx->transaction->Conclude();


you can move this call into "nostore branch"

In fact you can decide how to call Execute at line 2895 based on whether it's nostore or not, i.e. for nostore option - to conclude it there, and in this case you can eliminate the call to Conclude altogether

romange · 2024-01-09T08:55:20Z

src/server/zset_family.cc

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


what's the expected behaviour for no matched result together with STORE option? do we have a test for that? are we compliant with redis behavior in this case?

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

azuredream · 2024-01-09T16:40:24Z

Updated.

Added one more test case where no matching result in search area. This can only happen in GEOSEARCH FROMLONGLAT because searching by member always has at least one result(the member itself).
Redis returns an empty array in this case, so I updated the code to do the same.

romange

Thank you very much for progressing with this!

azuredream · 2024-01-10T23:02:59Z

You are very welcome.
Please let me know if you need GEOSEARCHSTORE as well.

BTW, if you are planning to implement vector related functions, I'll be glad to help.

azuredream added 2 commits December 28, 2023 14:00

feat: Store and diststore for GeoRadiusByMember

026e193

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

feat: Store and diststore for GeoRadiusByMember

6f23416

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

azuredream commented Dec 28, 2023

View reviewed changes

romange reviewed Dec 30, 2023

View reviewed changes

src/server/zset_family.cc Outdated Show resolved Hide resolved

romange reviewed Dec 30, 2023

View reviewed changes

feat: Store and diststore for GeoRadiusByMember

2d78a85

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

azuredream added 3 commits December 31, 2023 12:22

feat: Store and diststore for GeoRadiusByMember

c3802b2

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

feat: Store and diststore for GeoRadiusByMember

1bfd881

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

Merge branch 'main' into GeoRadiusByMember_store2

518c9e3

azuredream marked this pull request as ready for review January 1, 2024 08:43

romange reviewed Jan 2, 2024

View reviewed changes

azuredream added 2 commits January 2, 2024 18:39

feat: Store and diststore for GeoRadiusByMember

fff3cee

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

Merge branch 'GeoRadiusByMember_store2' of https://github.com/azuredr…

a0a69bd

…eam/dragonfly into GeoRadiusByMember_store2

azuredream added 2 commits January 2, 2024 18:59

feat: Store and diststore for GeoRadiusByMember

6e9cdf5

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

Merge branch 'main' into GeoRadiusByMember_store2

8f745cc

romange reviewed Jan 8, 2024

View reviewed changes

feat: Store and diststore for GeoRadiusByMember

88af9ac

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

romange reviewed Jan 9, 2024

View reviewed changes

azuredream added 2 commits January 9, 2024 11:37

feat: Store and diststore for GeoRadiusByMember

cb973e6

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

Merge branch 'main' into GeoRadiusByMember_store2

dc7640d

romange approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into GeoRadiusByMember_store2

47bdc8b

romange merged commit d7f4265 into dragonflydb:main Jan 11, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Store and diststore for GeoRadiusByMember #2350

feat: Store and diststore for GeoRadiusByMember #2350

azuredream commented Dec 28, 2023 •

edited

Loading

azuredream Dec 28, 2023

romange Dec 30, 2023

azuredream Dec 30, 2023

azuredream Dec 30, 2023

azuredream Dec 30, 2023

romange Dec 30, 2023

azuredream commented Dec 31, 2023 •

edited

Loading

azuredream commented Jan 1, 2024 •

edited

Loading

romange left a comment

romange Jan 2, 2024

romange Jan 2, 2024

romange Jan 2, 2024

romange Jan 2, 2024

romange Jan 2, 2024

azuredream commented Jan 2, 2024

azuredream commented Jan 8, 2024

romange Jan 8, 2024

romange Jan 8, 2024

azuredream commented Jan 9, 2024

romange Jan 9, 2024

romange Jan 9, 2024

romange Jan 9, 2024

romange Jan 9, 2024

azuredream commented Jan 9, 2024 •

edited

Loading

romange left a comment

azuredream commented Jan 10, 2024

feat: Store and diststore for GeoRadiusByMember #2350

feat: Store and diststore for GeoRadiusByMember #2350

Conversation

azuredream commented Dec 28, 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

azuredream commented Dec 31, 2023 • edited Loading

azuredream commented Jan 1, 2024 • edited Loading

romange 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

azuredream commented Jan 2, 2024

azuredream commented Jan 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azuredream commented Jan 9, 2024

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 Jan 9, 2024 • edited Loading

romange left a comment

Choose a reason for hiding this comment

azuredream commented Jan 10, 2024

azuredream commented Dec 28, 2023 •

edited

Loading

azuredream commented Dec 31, 2023 •

edited

Loading

azuredream commented Jan 1, 2024 •

edited

Loading

azuredream commented Jan 9, 2024 •

edited

Loading