From 9a20ac494d4c1b187e7b9ce8b1f96e9aff456acf Mon Sep 17 00:00:00 2001 From: Aidar Samerkhanov Date: Thu, 22 Feb 2024 09:34:08 +0400 Subject: [PATCH] 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 | 55 +++++++++++-------- .../tests/memgraph_V1/features/match.feature | 4 ++ .../memgraph_V1/features/memgraph_bfs.feature | 12 ++++ .../features/memgraph_wshortest.feature | 12 ++++ .../memgraph_V1/graphs/graph_edges.cypher | 1 + 5 files changed, 60 insertions(+), 24 deletions(-) diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 8dfaac81f..75b531261 100644 --- a/src/query/plan/operator.cpp +++ b/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]() { - if (self_.filter_lambda_.accumulated_path_symbol) { + auto restore_frame_state_after_expansion = [this, &frame](bool was_expanded) { + 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()); - restore_frame_state_after_expansion(); + bool was_expanded = expand_pair(edge, edge.To()); + 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()); - restore_frame_state_after_expansion(); + bool was_expanded = expand_pair(edge, edge.From()); + 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]( - const EdgeAccessor &edge, const VertexAccessor &vertex, const TypedValue &total_weight, - 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 - + auto expand_pair = [this, &evaluator, &frame, &create_state](const EdgeAccessor &edge, const VertexAccessor &vertex, + const TypedValue &total_weight, int64_t depth) { 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(); } diff --git a/tests/gql_behave/tests/memgraph_V1/features/match.feature b/tests/gql_behave/tests/memgraph_V1/features/match.feature index eaf8d3f44..47da2fadf 100644 --- a/tests/gql_behave/tests/memgraph_V1/features/match.feature +++ b/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 diff --git a/tests/gql_behave/tests/memgraph_V1/features/memgraph_bfs.feature b/tests/gql_behave/tests/memgraph_V1/features/memgraph_bfs.feature index 23edc69cd..01855e548 100644 --- a/tests/gql_behave/tests/memgraph_V1/features/memgraph_bfs.feature +++ b/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}) > | diff --git a/tests/gql_behave/tests/memgraph_V1/features/memgraph_wshortest.feature b/tests/gql_behave/tests/memgraph_V1/features/memgraph_wshortest.feature index afd484696..a160e471a 100644 --- a/tests/gql_behave/tests/memgraph_V1/features/memgraph_wshortest.feature +++ b/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}) > | diff --git a/tests/gql_behave/tests/memgraph_V1/graphs/graph_edges.cypher b/tests/gql_behave/tests/memgraph_V1/graphs/graph_edges.cypher index 3657b855b..1aa081cc1 100644 --- a/tests/gql_behave/tests/memgraph_V1/graphs/graph_edges.cypher +++ b/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);