diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index d0035091e..0086506ae 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -533,7 +533,7 @@ bool CheckExistingNode(const VertexAccessor &new_node, const TypedValue &existing_node = frame[existing_node_sym]; if (existing_node.IsNull()) return false; ExpectType(existing_node_sym, existing_node, TypedValue::Type::Vertex); - return existing_node.ValueVertex() != new_node; + return existing_node.ValueVertex() == new_node; } template @@ -834,11 +834,12 @@ class ExpandVariableCursor : public Cursor { // if lower bound is zero we also yield empty paths if (lower_bound_ == 0) { auto &start_vertex = frame[self_.input_symbol_].ValueVertex(); - if (!self_.common_.existing_node || - CheckExistingNode(start_vertex, self_.common_.node_symbol, - frame)) { + if (!self_.common_.existing_node) { frame[self_.common_.node_symbol] = start_vertex; return true; + } else if (CheckExistingNode(start_vertex, self_.common_.node_symbol, + frame)) { + return true; } } // if lower bound is not zero, we just continue, the next @@ -1016,11 +1017,9 @@ class ExpandVariableCursor : public Cursor { ? current_edge.first.From() : current_edge.first.To(); - if (self_.common_.existing_node && - !CheckExistingNode(current_vertex, self_.common_.node_symbol, frame)) - continue; - - frame[self_.common_.node_symbol] = current_vertex; + if (!self_.common_.existing_node) { + frame[self_.common_.node_symbol] = current_vertex; + } // Skip expanding out of filtered expansion. frame[self_.filter_lambda_.inner_edge_symbol] = current_edge.first; @@ -1039,6 +1038,10 @@ class ExpandVariableCursor : public Cursor { edges_it_.emplace_back(edges_.back().begin()); } + if (self_.common_.existing_node && + !CheckExistingNode(current_vertex, self_.common_.node_symbol, frame)) + continue; + // We only yield true if we satisfy the lower bound. if (static_cast(edges_on_frame.size()) >= lower_bound_) return true; diff --git a/tests/qa/tests/memgraph_V1/features/match.feature b/tests/qa/tests/memgraph_V1/features/match.feature index eaede5fc9..feae3d6aa 100644 --- a/tests/qa/tests/memgraph_V1/features/match.feature +++ b/tests/qa/tests/memgraph_V1/features/match.feature @@ -513,3 +513,111 @@ Feature: Match | size(path) | | 0 | | 1 | + + Scenario: Variable expand to existing symbol 1 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 3})-[:KNOWS*2]->(pers) RETURN path; + """ + Then the result should be empty + + Scenario: Variable expand to existing symbol 2 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 3})-[:KNOWS*]->(pers) RETURN path + """ + Then the result should be: + | path | + | <(:Person{id:3})-[:KNOWS]->(:Person{id:4})-[:KNOWS]->(:Person{id:1})-[:KNOWS]->(:Person{id:2})-[:KNOWS]->(:Person{id:3})> | + + Scenario: Variable expand to existing symbol 3 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 3})-[:KNOWS*0..]->(pers) RETURN path + """ + Then the result should be: + | path | + | <(:Person{id:3})> | + | <(:Person{id:3})-[:KNOWS]->(:Person{id:4})-[:KNOWS]->(:Person{id:1})-[:KNOWS]->(:Person{id:2})-[:KNOWS]->(:Person{id:3})> | + + Scenario: Variable expand to existing symbol 4 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 3})-[:KNOWS*2..6]->(pers) RETURN path + """ + Then the result should be: + | path | + | <(:Person{id:3})-[:KNOWS]->(:Person{id:4})-[:KNOWS]->(:Person{id:1})-[:KNOWS]->(:Person{id:2})-[:KNOWS]->(:Person{id:3})> | + + Scenario: Variable expand to existing symbol 5 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(:Person {id: 2})-[:KNOWS]->(:Person {id: 3})-[:KNOWS]->(:Person {id: 4})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 3})-[:KNOWS*5..]->(pers) RETURN path + """ + Then the result should be empty + + Scenario: Variable expand to existing symbol 6 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 1})-[:KNOWS*]->(pers) RETURN path + """ + Then the result should be + | path | + | <(:Person{id:1})-[:KNOWS]->(:Person{id:1})> | + + Scenario: Variable expand to existing symbol 7 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 1})-[:KNOWS*0..]->(pers) RETURN path + """ + Then the result should be + | path | + | <(:Person{id:1})> | + | <(:Person{id:1})-[:KNOWS]->(:Person{id:1})> | + + Scenario: Variable expand to existing symbol 8 + Given an empty graph + And having executed: + """ + CREATE (p1:Person {id: 1})-[:KNOWS]->(p1); + """ + When executing query: + """ + MATCH path = (pers:Person {id: 1})-[:KNOWS*2..]->(pers) RETURN path + """ + Then the result should be empty diff --git a/tests/unit/query_plan_match_filter_return.cpp b/tests/unit/query_plan_match_filter_return.cpp index c8d9e1e4d..de1033d6a 100644 --- a/tests/unit/query_plan_match_filter_return.cpp +++ b/tests/unit/query_plan_match_filter_return.cpp @@ -803,6 +803,247 @@ TEST_F(QueryPlanExpandVariable, NamedPath) { expected_paths.begin())); } +TEST_F(QueryPlanExpandVariable, ExpandToSameSymbol) { + auto test_expand = [&](int layer, EdgeAtom::Direction direction, + std::optional lower, + std::optional upper, bool reverse) { + auto e = Edge("r", direction); + + auto node = NODE("n"); + auto symbol = symbol_table.CreateSymbol("n", true); + node->identifier_->MapTo(symbol); + auto logical_op = + std::make_shared(nullptr, symbol, storage::View::OLD); + auto n_from = ScanAllTuple{node, logical_op, symbol}; + + auto filter_op = std::make_shared( + n_from.op_, + storage.Create( + n_from.node_->identifier_, std::vector{storage.GetLabelIx( + dba.LabelToName(labels[layer]))})); + + // convert optional ints to optional expressions + auto convert = [this](std::optional bound) { + return bound ? LITERAL(static_cast(bound.value())) : nullptr; + }; + + return GetEdgeListSizes( + std::make_shared( + filter_op, symbol, symbol, e, EdgeAtom::Type::DEPTH_FIRST, + direction, std::vector{}, reverse, + convert(lower), convert(upper), /* existing = */ true, + ExpansionLambda{symbol_table.CreateSymbol("inner_edge", false), + symbol_table.CreateSymbol("inner_node", false), + nullptr}, + std::nullopt, std::nullopt), + e); + }; + + // The graph is a double chain: + // chain 0: (v:0)-(v:1)-(v:2) + // X X + // chain 1: (v:0)-(v:1)-(v:2) + + // Expand from chain 0 v:0 to itself. + // + // It has a total of 3 cycles: + // 1. C0 v:0 -> C0 v:1 -> C1 v:2 -> C1 v:1 -> C0 v:0 + // 2. C0 v:0 -> C0 v:1 -> C0 v:2 -> C1 v:1 -> C0 v:0 + // 3. C0 v:0 -> C0 v:1 -> C1 v:0 -> C1 v:1 -> C0 v:0 + // + // Each cycle can be in two directions, also, we have two starting nodes: one + // in chain 0 and the other in chain 1. + for (auto reverse : {false, true}) { + // Tests with both bounds set. + for (int lower_bound = 0; lower_bound < 10; ++lower_bound) { + for (int upper_bound = lower_bound; upper_bound < 10; ++upper_bound) { + map_int expected_directed; + map_int expected_undirected; + if (lower_bound == 0) { + expected_directed.emplace(0, 2); + expected_undirected.emplace(0, 2); + } + if (lower_bound <= 4 && upper_bound >= 4) { + expected_undirected.emplace(4, 12); + } + if (lower_bound <= 8 && upper_bound >= 8) { + expected_undirected.emplace(8, 24); + } + + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, lower_bound, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, lower_bound, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, lower_bound, + upper_bound, reverse), + expected_undirected); + } + } + + // Test only upper bound. + for (int upper_bound = 0; upper_bound < 10; ++upper_bound) { + map_int expected_directed; + map_int expected_undirected; + if (upper_bound >= 4) { + expected_undirected.emplace(4, 12); + } + if (upper_bound >= 8) { + expected_undirected.emplace(8, 24); + } + + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, std::nullopt, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, std::nullopt, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, std::nullopt, + upper_bound, reverse), + expected_undirected); + } + + // Test only lower bound. + for (int lower_bound = 0; lower_bound < 10; ++lower_bound) { + map_int expected_directed; + map_int expected_undirected; + if (lower_bound == 0) { + expected_directed.emplace(0, 2); + expected_undirected.emplace(0, 2); + } + if (lower_bound <= 4) { + expected_undirected.emplace(4, 12); + } + if (lower_bound <= 8) { + expected_undirected.emplace(8, 24); + } + + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, lower_bound, + std::nullopt, reverse), + expected_directed); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, lower_bound, + std::nullopt, reverse), + expected_directed); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, lower_bound, + std::nullopt, reverse), + expected_undirected); + } + + // Test no bounds. + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, std::nullopt, + std::nullopt, reverse), + (map_int{})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, std::nullopt, + std::nullopt, reverse), + (map_int{})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, std::nullopt, + std::nullopt, reverse), + (map_int{{4, 12}, {8, 24}})); + } + + // Expand from chain 0 v:1 to itself. + // + // It has a total of 6 cycles: + // 1. C0 v:1 -> C1 v:0 -> C1 v:1 -> C1 v:2 -> C0 v:1 + // 2. C0 v:1 -> C1 v:0 -> C1 v:1 -> C0 v:2 -> C0 v:1 + // 3. C0 v:1 -> C0 v:0 -> C1 v:1 -> C1 v:2 -> C0 v:1 + // 4. C0 v:1 -> C0 v:0 -> C1 v:1 -> C0 v:2 -> C0 v:1 + // 5. C0 v:1 -> C1 v:0 -> C1 v:1 -> C0 v:0 -> C0 v:1 + // 6. C0 v:1 -> C1 v:2 -> C1 v:1 -> C0 v:2 -> C0 v:1 + // + // Each cycle can be in two directions, also, we have two starting nodes: one + // in chain 0 and the other in chain 1. + for (auto reverse : {false, true}) { + // Tests with both bounds set. + for (int lower_bound = 0; lower_bound < 10; ++lower_bound) { + for (int upper_bound = lower_bound; upper_bound < 10; ++upper_bound) { + map_int expected_directed; + map_int expected_undirected; + if (lower_bound == 0) { + expected_directed.emplace(0, 2); + expected_undirected.emplace(0, 2); + } + if (lower_bound <= 4 && upper_bound >= 4) { + expected_undirected.emplace(4, 24); + } + if (lower_bound <= 8 && upper_bound >= 8) { + expected_undirected.emplace(8, 48); + } + + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, lower_bound, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, lower_bound, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, lower_bound, + upper_bound, reverse), + expected_undirected); + } + } + + // Test only upper bound. + for (int upper_bound = 0; upper_bound < 10; ++upper_bound) { + map_int expected_directed; + map_int expected_undirected; + if (upper_bound >= 4) { + expected_undirected.emplace(4, 24); + } + if (upper_bound >= 8) { + expected_undirected.emplace(8, 48); + } + + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, std::nullopt, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, std::nullopt, + upper_bound, reverse), + expected_directed); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, std::nullopt, + upper_bound, reverse), + expected_undirected); + } + + // Test only lower bound. + for (int lower_bound = 0; lower_bound < 10; ++lower_bound) { + map_int expected_directed; + map_int expected_undirected; + if (lower_bound == 0) { + expected_directed.emplace(0, 2); + expected_undirected.emplace(0, 2); + } + if (lower_bound <= 4) { + expected_undirected.emplace(4, 24); + } + if (lower_bound <= 8) { + expected_undirected.emplace(8, 48); + } + + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, lower_bound, + std::nullopt, reverse), + expected_directed); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, lower_bound, + std::nullopt, reverse), + expected_directed); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, lower_bound, + std::nullopt, reverse), + expected_undirected); + } + + // Test no bounds. + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, std::nullopt, + std::nullopt, reverse), + (map_int{})); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, std::nullopt, + std::nullopt, reverse), + (map_int{})); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, std::nullopt, + std::nullopt, reverse), + (map_int{{4, 24}, {8, 48}})); + } +} + namespace std { template <> struct hash> {