[master < T221] Periodic iterate module #221

Josipmrden · 2023-06-12T09:34:10Z

Description

In the pull request, periodic.iterate module has been added to support periodic transactions over a query. It helps when having to allocate a lot of deltas that need to be read all over again during the each passing of the operator tree.

Pull request type

Algorithm/Module

######################################

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

######################################

antoniofilipovic · 2023-06-20T08:12:48Z

cpp/periodic_module/periodic_iterate.cpp

+
+    mg::Client::Init();
+
+    mg::Client::Params params{.host = "localhost", .port = 7687};


Can this be configurable?

I think I can do something with environment variables, let me check

antoniofilipovic

Good work, I left few comments and one optimization point to possibly benchmark

cpp/periodic_module/periodic_iterate.cpp

antoniofilipovic · 2023-06-20T08:27:34Z

cpp/periodic_module/periodic_iterate.cpp

+
+    mg::Client::Finalize();
+
+    record.Insert(kReturnSuccess, true);


Does it make sense for every batch executed to return success status, instead of returning one for all batches? This way we can test that batches are executed correctly.

If you decide to go in this direction, it makes sense to add one more test for batching, to check that batches are executed correctly

From my perspective, one success flag is better because even if some batch failed, what will user do with it? He will see false and then what. Regarding what @antoniofilipovic said

Maybe returning number of batches would be good. The user would explicitly see in how many batches, the query was executed so he/she could maybe tune it next time + you can test that the code is working as expected.

I will adjust this so it returns the number of batches

cpp/periodic_module/periodic_iterate.cpp

antoniofilipovic · 2023-06-20T08:33:06Z

cpp/periodic_module/periodic_iterate.cpp

+  return new_columns;
+}
+
+std::string ConstructQueryPreffix(ParamNames names) {


I am not sure what is faster, but one optimization idea is to check istream (input stream) and to fill input stream and then return that string that input stream created from function

Maybe you can do quick benchmark and report results, instead of using boost what we talked on daily

There is utils::Join in Memgraph also if the performance is problem with boost

I can't use Join since that's not in the included headers for mage so I just written my own

Maybe we can copy-paste it here?

cpp/periodic_module/periodic_iterate.cpp

Dockerfile

python/tests/requirements.txt

as51340

Nice job but I think there are things you can improve in your code. I would suggest adding test when batch is not 1000 or 1 because that would prove a bug that rows variable isn't incremented and test which works with relationships also not just vertices.

cpp/CMakeLists.txt

cpp/periodic_module/periodic_iterate.cpp

as51340 · 2023-06-20T09:13:33Z

cpp/periodic_module/periodic_iterate.cpp

+
+    mg::Client::Finalize();
+
+    record.Insert(kReturnSuccess, true);


From my perspective, one success flag is better because even if some batch failed, what will user do with it? He will see false and then what. Regarding what @antoniofilipovic said

cpp/periodic_module/periodic_iterate.cpp

e2e/periodic_iterate_test/test_periodic_iterate_invalid_batch_argument_negative/test.yml

as51340 · 2023-06-20T09:24:17Z

cpp/periodic_module/periodic_iterate.cpp

+
+    mg::Client::Finalize();
+
+    record.Insert(kReturnSuccess, true);


Maybe returning number of batches would be good. The user would explicitly see in how many batches, the query was executed so he/she could maybe tune it next time + you can test that the code is working as expected.

Josipmrden · 2023-06-25T09:42:30Z

cpp/periodic_module/periodic_iterate.cpp

+
+  auto query_params = ConstructParams(columns, batch);
+
+  mg::Client::Params session_params{.host = "localhost", .port = 7687};


yes but I was thinking more in sense that I want to make session open as short as possible?

python/tests/requirements.txt

Josipmrden · 2023-06-25T09:48:21Z

cpp/periodic_module/periodic_iterate.cpp

+
+    mg::Client::Init();
+
+    mg::Client::Params params{.host = "localhost", .port = 7687};


I think I can do something with environment variables, let me check

Josipmrden · 2023-06-25T12:34:43Z

cpp/periodic_module/periodic_iterate.cpp

+  return new_columns;
+}
+
+std::string ConstructQueryPreffix(ParamNames names) {


I can't use Join since that's not in the included headers for mage so I just written my own

Josipmrden · 2023-06-25T12:35:30Z

cpp/periodic_module/periodic_iterate.cpp

+    if (!client->Execute(input_query)) {
+      record.Insert(kReturnSuccess, false);
+      return;


CREATE INDEX ON :SuperNode; CREATE INDEX ON :Node; CREATE INDEX ON :Node(id); CREATE (:SuperNode); FOREACH (i IN range(1, 1000000) | CREATE (:Node {id: i})); // UNWIND range(1, 1000000) as x // MATCH (s:SuperNode), (n:Node {id: x}) // CREATE (s)-[:HAS_REL_TO]->(n); CALL periodic.iterate( "UNWIND range(1, 1000000) AS x RETURN x", "MATCH (s:SuperNode), (n:Node {id: x}) CREATE (s)-[:HAS_REL_TO]->(n)", {batch_size: 5000}) YIELD * RETURN *;

wanted to write this in the documentation, which is a clear example why periodic iterate helps (until we maybe do some changes in the frame, which solve it out of the box)

Josipmrden · 2023-06-25T12:40:58Z

cpp/periodic_module/periodic_iterate.cpp

+    if (!client->Execute(input_query)) {
+      record.Insert(kReturnSuccess, false);
+      return;


the problem does not occur if MATCH is the first command since it happens only once. However, in a LOAD CSV heavy environment where some of the values can be dynamic, and supernodes are mostly fetched in the same manner, it is beneficial.

Josipmrden · 2023-06-25T12:51:39Z

cpp/periodic_module/periodic_iterate.cpp

+
+    mg::Client::Finalize();
+
+    record.Insert(kReturnSuccess, true);


I will adjust this so it returns the number of batches

Josipmrden · 2023-06-25T14:12:00Z

.gitmodules

@@ -1,4 +1,4 @@
 [submodule "cpp/memgraph"]
 	path = cpp/memgraph
 	url = https://github.com/memgraph/memgraph.git
-	branch = release/2.8
+	branch = fix-creating-mgp-value-from-nullptr


This will be rewritten to release/v2.9 once it's up

Yes, we can bump this up then.

Josipmrden · 2023-06-25T14:13:25Z

cpp/periodic_module/periodic_iterate.cpp

+    std::vector<std::vector<mg::Value>> batch;
+    batch.reserve(batch_size);
+    int rows = 0;
+    while (const auto maybe_result = client->FetchOne()) {


but it should exist since const auto maybe_result = client->FetchOne() will evaluate to bool

Josipmrden · 2023-06-25T14:13:38Z

cpp/periodic_module/periodic_iterate.cpp

+    std::vector<std::vector<mg::Value>> batch;
+    batch.reserve(batch_size);
+    int rows = 0;
+    while (const auto maybe_result = client->FetchOne()) {


that's why it's in the while condition

as51340 · 2023-06-26T08:13:09Z

cpp/periodic_module/periodic_iterate.cpp

+
+  auto query_params = ConstructParams(columns, batch);
+
+  mg::Client::Params session_params{.host = "localhost", .port = 7687};


I am quite sure that both ways are correct, what I wrote made more sense to me but do as you want 😄

as51340 · 2023-06-26T08:16:10Z

cpp/periodic_module/periodic_iterate.cpp

+    std::vector<std::vector<mg::Value>> batch;
+    batch.reserve(batch_size);
+    int rows = 0;
+    while (const auto maybe_result = client->FetchOne()) {


shouldn't then the condition be:
while (const auto maybe_result = client->FetchOne(); maybe_result)

as51340 · 2023-06-26T08:21:21Z

cpp/periodic_module/periodic_iterate.cpp

+  return res;
+}
+
+auto Join(const std::vector<std::string> &strings, const std::string &delimiter) {


std::string instead of auto

Yes, it makes sense to do that here as we are not just returning output from other function

will do that in every method

as51340 · 2023-06-26T08:21:53Z

cpp/periodic_module/periodic_iterate.cpp

+
+  auto query_params = ConstructParams(columns, batch);
+
+  mg::Client::Params session_params{.host = "localhost", .port = 7687};


Both ways are correct, do as you want 😄

as51340 · 2023-06-26T08:23:28Z

cpp/periodic_module/periodic_iterate.cpp

+    std::vector<std::vector<mg::Value>> batch;
+    batch.reserve(batch_size);
+    int rows = 0;
+    while (const auto maybe_result = client->FetchOne()) {


But which type is then maybe_result?

as51340 · 2023-06-26T08:29:14Z

cpp/periodic_module/periodic_iterate.cpp

+auto ConstructWithStatement(const ParamNames &names) {
+  std::vector<std::string> with_entity_vector;
+  for (const auto &node_name : names.node_names) {
+    with_entity_vector.push_back(GetGraphFirstClassEntityAlias(kBatchRowInternalName, node_name));


And without GetGraphFirstClassEntityAlias as we want to not use ctor and move ctor then

as51340 · 2023-06-26T08:29:19Z

cpp/periodic_module/periodic_iterate.cpp

+    with_entity_vector.push_back(GetGraphFirstClassEntityAlias(kBatchRowInternalName, node_name));
+  }
+  for (const auto &rel_name : names.relationship_names) {
+    with_entity_vector.push_back(GetGraphFirstClassEntityAlias(kBatchRowInternalName, rel_name));


as51340 · 2023-06-26T08:29:25Z

cpp/periodic_module/periodic_iterate.cpp

+    with_entity_vector.push_back(GetGraphFirstClassEntityAlias(kBatchRowInternalName, rel_name));
+  }
+  for (const auto &prim_name : names.primitive_names) {
+    with_entity_vector.push_back(GetPrimitiveEntityAlias(kBatchRowInternalName, prim_name));


as51340 · 2023-06-26T08:29:33Z

cpp/periodic_module/periodic_iterate.cpp

+    with_entity_vector.push_back(GetPrimitiveEntityAlias(kBatchRowInternalName, prim_name));
+  }
+
+  auto with_variables = fmt::format("WITH {}", Join(with_entity_vector, ", "));


return immediately

as51340 · 2023-06-26T08:30:55Z

cpp/periodic_module/periodic_iterate.cpp

+  return res;
+}
+
+auto Join(const std::vector<std::string> &strings, const std::string &delimiter) {


more efficient version of this would be reserve the total size of the string upfront but 👍

Yes, check how is this implemented in memgraph. in src/utils/string.hpp

antoniofilipovic · 2023-06-26T09:56:32Z

cpp/periodic_module/periodic_iterate.cpp

+  std::vector<std::string> primitive_names;
+};
+
+auto ExtractParamNames(const std::vector<std::string> &columns, const std::vector<mg::Value> &batch_row) {


I would also put ParamNames as the return type instead of auto as we are returning type from within the function not just returning the output of other functions.

antoniofilipovic · 2023-06-26T10:08:54Z

@Josipmrden fix comments which @as51340 suggested, but otherwise looks good to me after that is fixed

Josipmrden · 2023-06-27T11:26:23Z

@vpavicic I still need docs for this, will do tomorrow

vpavicic · 2023-06-28T07:03:03Z

@vpavicic I still need docs for this, will do tomorrow

@Josipmrden release note as well please thank you you're awesome

vpavicic · 2023-06-28T12:04:25Z

@Josipmrden ok?
When executing complex queries, the period module allows batching results
from one query into another to improve execution time.

Add periodic iterate module

44d4171

Josipmrden changed the title ~~Periodic iterate module~~ [master < #221] Periodic iterate module Jun 12, 2023

Josipmrden changed the title ~~[master < #221] Periodic iterate module~~ [master < T221] Periodic iterate module Jun 12, 2023

Josipmrden self-assigned this Jun 12, 2023

Josipmrden added good first issue good first issue In progress type: enhancement and removed good first issue good first issue labels Jun 12, 2023

Josipmrden added this to the 1.8.0 milestone Jun 12, 2023

Josipmrden linked an issue Jun 12, 2023 that may be closed by this pull request

[ENHANCEMENT] Add periodic iterate module #224

Closed

Josipmrden added 2 commits June 19, 2023 11:01

Add e2e tests to periodic iterate

de6cea7

Add e2e tests for multiple batches

06969a7

Josipmrden requested review from as51340 and antoniofilipovic June 19, 2023 09:10

Josipmrden marked this pull request as ready for review June 19, 2023 09:10

delete dockerfile blank line

05025ba

antoniofilipovic reviewed Jun 20, 2023

View reviewed changes

antoniofilipovic requested changes Jun 20, 2023

View reviewed changes

antoniofilipovic reviewed Jun 20, 2023

View reviewed changes

python/tests/requirements.txt Show resolved Hide resolved

as51340 requested changes Jun 20, 2023

View reviewed changes

Josipmrden added 7 commits June 25, 2023 11:46

Address PR review comments

4810343

Add back Join custom function

c35cda3

Add arguments to client params

af6e8a6

Fix dereferencing a null pointer

5ed4735

Add number of batches to execution result

fd30ecb

Address PR comments regarding nested comments

f73b45e

Remove local variable

3012300

Josipmrden requested review from antoniofilipovic and as51340 June 25, 2023 14:36

Josipmrden commented Jun 25, 2023

View reviewed changes

as51340 requested changes Jun 26, 2023

View reviewed changes

antoniofilipovic reviewed Jun 26, 2023

View reviewed changes

antoniofilipovic approved these changes Jun 26, 2023

View reviewed changes

Address PR comments

d981d48

as51340 approved these changes Jun 26, 2023

View reviewed changes

Josipmrden merged commit 09e3354 into main Jun 27, 2023
4 checks passed

Josipmrden deleted the add-periodic-iterate-cpp-module branch June 27, 2023 11:25

Josipmrden mentioned this pull request Jun 27, 2023

[master < T634] Improve reading speed of deltas memgraph/memgraph#919

Closed

4 tasks

gvolfing mentioned this pull request Sep 15, 2023

[BUG] libmgclient.so is being copied with the query modules into the target location during build. #357

Closed


		mg::Client::Init();

		mg::Client::Params params{.host = "localhost", .port = 7687};


		auto query_params = ConstructParams(columns, batch);

		mg::Client::Params session_params{.host = "localhost", .port = 7687};

[master < T221] Periodic iterate module #221

[master < T221] Periodic iterate module #221

Conversation

Josipmrden commented Jun 12, 2023 • edited Loading

Description

Pull request type

Reviewer checklist (the reviewer checks this part)

Module/Algorithm

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniofilipovic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

as51340 Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

as51340 Jun 20, 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

as51340 Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

as51340 left a comment

Choose a reason for hiding this comment

as51340 Jun 20, 2023 • edited Loading

Choose a reason for hiding this comment

as51340 Jun 20, 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

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

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

antoniofilipovic commented Jun 26, 2023

Josipmrden commented Jun 27, 2023

vpavicic commented Jun 28, 2023

vpavicic commented Jun 28, 2023

Josipmrden commented Jun 12, 2023 •

edited

Loading

as51340 Jun 20, 2023 •

edited

Loading

as51340 Jun 20, 2023 •

edited

Loading

as51340 Jun 20, 2023 •

edited

Loading

as51340 Jun 20, 2023 •

edited

Loading

as51340 Jun 20, 2023 •

edited

Loading