In BFS expansion filter by path we should shrink path to restore state prior to expansion only if the path was changed. (#1745)

src/query/plan/operator.cpp

@@ -1549,15 +1549,15 @@ class SingleSourceShortestPathCursor : public query::plan::Cursor {
 
     // for the given (edge, vertex) pair checks if they satisfy the
     // "where" condition. if so, places them in the to_visit_ structure.
-    auto expand_pair = [this, &evaluator, &frame, &context](EdgeAccessor edge, VertexAccessor vertex) {
+    auto expand_pair = [this, &evaluator, &frame, &context](EdgeAccessor edge, VertexAccessor vertex) -> bool {
       // if we already processed the given vertex it doesn't get expanded
-      if (processed_.find(vertex) != processed_.end()) return;
+      if (processed_.find(vertex) != processed_.end()) return false;
 #ifdef MG_ENTERPRISE
       if (license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker &&
           !(context.auth_checker->Has(vertex, storage::View::OLD,
                                       memgraph::query::AuthQuery::FineGrainedPrivilege::READ) &&
             context.auth_checker->Has(edge, memgraph::query::AuthQuery::FineGrainedPrivilege::READ))) {
-        return;
+        return false;
       }
 #endif
       frame[self_.filter_lambda_.inner_edge_symbol] = edge;
@@ -1576,9 +1576,9 @@ class SingleSourceShortestPathCursor : public query::plan::Cursor {
         TypedValue result = self_.filter_lambda_.expression->Accept(evaluator);
         switch (result.type()) {
           case TypedValue::Type::Null:
-            return;
+            return true;
           case TypedValue::Type::Bool:
-            if (!result.ValueBool()) return;
+            if (!result.ValueBool()) return true;
             break;
           default:
             throw QueryRuntimeException("Expansion condition must evaluate to boolean or null.");
@@ -1586,10 +1586,11 @@ class SingleSourceShortestPathCursor : public query::plan::Cursor {
       }
       to_visit_next_.emplace_back(edge, vertex, std::move(curr_acc_path));
       processed_.emplace(vertex, edge);
+      return true;
     };
 
-    auto restore_frame_state_after_expansion = [this, &frame]() {
+    auto restore_frame_state_after_expansion = [this, &frame](bool was_expanded) {
-      if (self_.filter_lambda_.accumulated_path_symbol) {
+      if (was_expanded && self_.filter_lambda_.accumulated_path_symbol) {
         frame[self_.filter_lambda_.accumulated_path_symbol.value()].ValuePath().Shrink();
       }
     };
@@ -1601,15 +1602,15 @@ class SingleSourceShortestPathCursor : public query::plan::Cursor {
       if (self_.common_.direction != EdgeAtom::Direction::IN) {
         auto out_edges = UnwrapEdgesResult(vertex.OutEdges(storage::View::OLD, self_.common_.edge_types)).edges;
         for (const auto &edge : out_edges) {
-          expand_pair(edge, edge.To());
+          bool was_expanded = expand_pair(edge, edge.To());
-          restore_frame_state_after_expansion();
+          restore_frame_state_after_expansion(was_expanded);
         }
       }
       if (self_.common_.direction != EdgeAtom::Direction::OUT) {
         auto in_edges = UnwrapEdgesResult(vertex.InEdges(storage::View::OLD, self_.common_.edge_types)).edges;
         for (const auto &edge : in_edges) {
-          expand_pair(edge, edge.From());
+          bool was_expanded = expand_pair(edge, edge.From());
-          restore_frame_state_after_expansion();
+          restore_frame_state_after_expansion(was_expanded);
         }
       }
     };
@@ -1800,18 +1801,8 @@ class ExpandWeightedShortestPathCursor : public query::plan::Cursor {
     // For the given (edge, vertex, weight, depth) tuple checks if they
     // satisfy the "where" condition. if so, places them in the priority
     // queue.
-    auto expand_pair = [this, &evaluator, &frame, &create_state, &context](
+    auto expand_pair = [this, &evaluator, &frame, &create_state](const EdgeAccessor &edge, const VertexAccessor &vertex,
-                           const EdgeAccessor &edge, const VertexAccessor &vertex, const TypedValue &total_weight,
+                                                                 const TypedValue &total_weight, int64_t depth) {
-                           int64_t depth) {
-#ifdef MG_ENTERPRISE
-      if (license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker &&
-          !(context.auth_checker->Has(vertex, storage::View::OLD,
-                                      memgraph::query::AuthQuery::FineGrainedPrivilege::READ) &&
-            context.auth_checker->Has(edge, memgraph::query::AuthQuery::FineGrainedPrivilege::READ))) {
-        return;
-      }
-#endif
-
       frame[self_.weight_lambda_->inner_edge_symbol] = edge;
       frame[self_.weight_lambda_->inner_node_symbol] = vertex;
       TypedValue next_weight = CalculateNextWeight(self_.weight_lambda_, total_weight, evaluator);
@@ -1854,11 +1845,19 @@ class ExpandWeightedShortestPathCursor : public query::plan::Cursor {
     // Populates the priority queue structure with expansions
     // from the given vertex. skips expansions that don't satisfy
     // the "where" condition.
-    auto expand_from_vertex = [this, &expand_pair, &restore_frame_state_after_expansion](
+    auto expand_from_vertex = [this, &context, &expand_pair, &restore_frame_state_after_expansion](
                                   const VertexAccessor &vertex, const TypedValue &weight, int64_t depth) {
       if (self_.common_.direction != EdgeAtom::Direction::IN) {
         auto out_edges = UnwrapEdgesResult(vertex.OutEdges(storage::View::OLD, self_.common_.edge_types)).edges;
         for (const auto &edge : out_edges) {
+#ifdef MG_ENTERPRISE
+          if (license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker &&
+              !(context.auth_checker->Has(edge.To(), storage::View::OLD,
+                                          memgraph::query::AuthQuery::FineGrainedPrivilege::READ) &&
+                context.auth_checker->Has(edge, memgraph::query::AuthQuery::FineGrainedPrivilege::READ))) {
+            continue;
+          }
+#endif
           expand_pair(edge, edge.To(), weight, depth);
           restore_frame_state_after_expansion();
         }
@@ -1866,6 +1865,14 @@ class ExpandWeightedShortestPathCursor : public query::plan::Cursor {
       if (self_.common_.direction != EdgeAtom::Direction::OUT) {
         auto in_edges = UnwrapEdgesResult(vertex.InEdges(storage::View::OLD, self_.common_.edge_types)).edges;
         for (const auto &edge : in_edges) {
+#ifdef MG_ENTERPRISE
+          if (license::global_license_checker.IsEnterpriseValidFast() && context.auth_checker &&
+              !(context.auth_checker->Has(edge.From(), storage::View::OLD,
+                                          memgraph::query::AuthQuery::FineGrainedPrivilege::READ) &&
+                context.auth_checker->Has(edge, memgraph::query::AuthQuery::FineGrainedPrivilege::READ))) {
+            continue;
+          }
+#endif
           expand_pair(edge, edge.From(), weight, depth);
           restore_frame_state_after_expansion();
         }

tests/gql_behave/tests/memgraph_V1/features/match.feature

@@ -761,6 +761,7 @@ Feature: Match
         Then the result should be:
             | path                                        |
             | <(:label1 {id: 1})-[:type2 {id: 10}]->(:label3 {id: 3})> |
+            | <(:label1 {id: 1})-[:same {id: 30}]->(:label1 {id: 1})-[:type2 {id: 10}]->(:label3 {id: 3})> |
 
     Scenario: Test DFS variable expand using IN edges with filter by edge type1
         Given graph "graph_edges"
@@ -771,6 +772,7 @@ Feature: Match
         Then the result should be:
             | path                                        |
             | <(:label3 {id: 3})<-[:type2 {id: 10}]-(:label1 {id: 1})> |
+            | <(:label3 {id: 3})<-[:type2 {id: 10}]-(:label1 {id: 1})-[:same {id: 30}]->(:label1 {id: 1})> |
 
     Scenario: Test DFS variable expand with filter by edge type2
         Given graph "graph_edges"
@@ -781,6 +783,7 @@ Feature: Match
         Then the result should be:
             | path                                        |
             | <(:label1 {id: 1})-[:type1 {id: 1}]->(:label2 {id: 2})-[:type1 {id: 2}]->(:label3 {id: 3})> |
+            | <(:label1 {id: 1})-[:same {id: 30}]->(:label1 {id: 1})-[:type1 {id: 1}]->(:label2 {id: 2})-[:type1 {id: 2}]->(:label3 {id: 3})> |
 
     Scenario: Test DFS variable expand using IN edges with filter by edge type2
         Given graph "graph_edges"
@@ -791,6 +794,7 @@ Feature: Match
         Then the result should be:
             | path                                        |
             | <(:label3 {id: 3})<-[:type1 {id: 2}]-(:label2 {id: 2})<-[:type1 {id: 1}]-(:label1 {id: 1})> |
+            | <(:label3 {id: 3})<-[:type1 {id: 2}]-(:label2 {id: 2})<-[:type1 {id: 1}]-(:label1 {id: 1})-[:same {id: 30}]->(:label1 {id: 1})> |
 
     Scenario: Using path indentifier from CREATE in MERGE
         Given an empty graph

tests/gql_behave/tests/memgraph_V1/features/memgraph_bfs.feature

@@ -249,3 +249,15 @@ Feature: Bfs
         Then the result should be:
             | path                                        |
             | <(:label1 {id: 1})-[:type1 {id: 1}]->(:label2 {id: 2})-[:type1 {id: 2}]->(:label3 {id: 3})> |
+
+    Scenario: Test BFS variable expand with already processed vertex and loop with filter by path
+        Given graph "graph_edges"
+        When executing query:
+            """
+            MATCH path=(:label1)-[*BFS 1..1 (e, n, p | True)]-() RETURN path;
+            """
+        Then the result should be:
+            | path                                        |
+            | < (:label1 {id: 1})-[:type3 {id: 20}]->(:label5 {id: 5}) > |
+            | < (:label1 {id: 1})-[:type2 {id: 10}]->(:label3 {id: 3}) > |
+            | < (:label1 {id: 1})-[:type1 {id: 1}]->(:label2 {id: 2}) >  |

tests/gql_behave/tests/memgraph_V1/features/memgraph_wshortest.feature

@@ -269,3 +269,15 @@ Feature: Weighted Shortest Path
       Then the result should be:
           | path   | total_weight   |
           | <(:station {arrival: 08:00:00.000000000, departure: 08:15:00.000000000, name: 'A'})-[:ride {duration: PT1H5M, id: 1}]->(:station {arrival: 09:20:00.000000000, departure: 09:30:00.000000000, name: 'B'})-[:ride {duration: PT30M, id: 2}]->(:station {arrival: 10:00:00.000000000, departure: 10:20:00.000000000, name: 'C'})> | PT2H20M  |
+
+    Scenario: Test wShortest variable expand with already processed vertex and loop with filter by path
+        Given graph "graph_edges"
+        When executing query:
+            """
+            MATCH path=(:label1)-[*WSHORTEST ..1 (r, n | r.id) total_weight (e, n, p | True)]-() RETURN path;
+            """
+        Then the result should be:
+            | path                                        |
+            | < (:label1 {id: 1})-[:type3 {id: 20}]->(:label5 {id: 5}) > |
+            | < (:label1 {id: 1})-[:type2 {id: 10}]->(:label3 {id: 3}) > |
+            | < (:label1 {id: 1})-[:type1 {id: 1}]->(:label2 {id: 2}) >  |

tests/gql_behave/tests/memgraph_V1/graphs/graph_edges.cypher

@@ -1,3 +1,4 @@
 CREATE (:label1 {id: 1})-[:type1 {id:1}]->(:label2 {id: 2})-[:type1 {id: 2}]->(:label3 {id: 3})-[:type1 {id: 3}]->(:label4 {id: 4});
 MATCH (n :label1), (m :label3) CREATE (n)-[:type2 {id: 10}]->(m);
 MATCH (n :label1) CREATE (n)-[:type3 {id: 20}]->(:label5 { id: 5 });
+MATCH (n :label1) CREATE (n)-[:same {id: 30}]->(n);