diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 8efb96101..27d5839ba 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -527,7 +527,7 @@ bool Expand::ExpandCursor::PullNode(const EdgeAccessor &new_edge, } ExpandVariable::ExpandVariable(Symbol node_symbol, Symbol edge_symbol, - EdgeAtom::Direction direction, + EdgeAtom::Direction direction, bool is_reverse, Expression *lower_bound, Expression *upper_bound, const std::shared_ptr &input, Symbol input_symbol, bool existing_node, @@ -535,7 +535,8 @@ ExpandVariable::ExpandVariable(Symbol node_symbol, Symbol edge_symbol, : ExpandCommon(node_symbol, edge_symbol, direction, input, input_symbol, existing_node, existing_edge, graph_view), lower_bound_(lower_bound), - upper_bound_(upper_bound) {} + upper_bound_(upper_bound), + is_reverse_(is_reverse) {} bool Expand::ExpandCursor::HandleExistingEdge(const EdgeAccessor &new_edge, Frame &frame) const { @@ -746,8 +747,10 @@ class ExpandVariableCursor : public Cursor { if (edges_on_frame.size() < edges_.size()) return EdgePlacementResult::MISMATCH; + int last_edge_index = + self_.is_reverse_ ? 0 : static_cast(edges_.size()) - 1; EdgeAccessor &edge_on_frame = - edges_on_frame[edges_.size() - 1].Value(); + edges_on_frame[last_edge_index].Value(); if (edge_on_frame != new_edge) return EdgePlacementResult::MISMATCH; // the new_edge matches the corresponding frame element @@ -763,8 +766,20 @@ class ExpandVariableCursor : public Cursor { // it is possible that there already exists an edge on the frame for // this level. if so first remove it debug_assert(edges_.size() > 0, "Edges are empty"); - edges_on_frame.resize(std::min(edges_on_frame.size(), edges_.size() - 1)); - edges_on_frame.emplace_back(new_edge); + if (self_.is_reverse_) { + // TODO: This is innefficient, we should look into replacing + // vector with something else for TypedValue::List. + size_t diff = edges_on_frame.size() - + std::min(edges_on_frame.size(), edges_.size() - 1U); + if (diff > 0U) + edges_on_frame.erase(edges_on_frame.begin(), + edges_on_frame.begin() + diff); + edges_on_frame.insert(edges_on_frame.begin(), new_edge); + } else { + edges_on_frame.resize( + std::min(edges_on_frame.size(), edges_.size() - 1U)); + edges_on_frame.emplace_back(new_edge); + } return EdgePlacementResult::YIELD; } } @@ -803,8 +818,18 @@ class ExpandVariableCursor : public Cursor { // elements as edges_ due to edge-uniqueness (when a whole layer // gets exhausted but no edges are valid). for that reason only // pop from edges_on_frame if they contain enough elements - if (!self_.existing_edge_) - edges_on_frame.resize(std::min(edges_on_frame.size(), edges_.size())); + if (!self_.existing_edge_) { + if (self_.is_reverse_) { + auto diff = edges_on_frame.size() - + std::min(edges_on_frame.size(), edges_.size()); + if (diff > 0) { + edges_on_frame.erase(edges_on_frame.begin(), + edges_on_frame.begin() + diff); + } + } else { + edges_on_frame.resize(std::min(edges_on_frame.size(), edges_.size())); + } + } // if we are here, we have a valid stack, // get the edge, increase the relevant iterator diff --git a/src/query/plan/operator.hpp b/src/query/plan/operator.hpp index f7227ad2c..d0a08c906 100644 --- a/src/query/plan/operator.hpp +++ b/src/query/plan/operator.hpp @@ -615,14 +615,17 @@ class ExpandVariable : public LogicalOperator, ExpandCommon { * Expansion length bounds are both inclusive (as in Neo's Cypher * implementation). * + * @param is_reverse Set to `true` if the edges written on frame should expand + * from `node_symbol` to `input_symbol`. Opposed to the usual expanding + * from `input_symbol` to `node_symbol`. * @param lower_bound An optional indicator of the minimum number of edges * that get expanded (inclusive). * @param upper_bound An optional indicator of the maximum number of edges * that get expanded (inclusive). */ ExpandVariable(Symbol node_symbol, Symbol edge_symbol, - EdgeAtom::Direction direction, Expression *lower_bound, - Expression *upper_bound, + EdgeAtom::Direction direction, bool is_reverse, + Expression *lower_bound, Expression *upper_bound, const std::shared_ptr &input, Symbol input_symbol, bool existing_node, bool existing_edge, GraphView graph_view = GraphView::AS_IS); @@ -635,6 +638,9 @@ class ExpandVariable : public LogicalOperator, ExpandCommon { // both are optional, defaults are (1, inf) Expression *lower_bound_; Expression *upper_bound_; + // True if the path should be written as expanding from node_symbol to + // input_symbol. + bool is_reverse_; }; /** diff --git a/src/query/plan/rule_based_planner.cpp b/src/query/plan/rule_based_planner.cpp index 33fbe68b8..ff2b8f580 100644 --- a/src/query/plan/rule_based_planner.cpp +++ b/src/query/plan/rule_based_planner.cpp @@ -877,6 +877,7 @@ LogicalOperator *PlanMatching(const Matching &matching, } else if (expansion.edge->has_range_) { last_op = new ExpandVariable( node_symbol, edge_symbol, expansion.direction, + expansion.direction != expansion.edge->direction_, expansion.edge->lower_bound_, expansion.edge->upper_bound_, std::shared_ptr(last_op), node1_symbol, existing_node, existing_edge, context.graph_view); diff --git a/tests/unit/query_cost_estimator.cpp b/tests/unit/query_cost_estimator.cpp index 16f1d7e86..8a666068e 100644 --- a/tests/unit/query_cost_estimator.cpp +++ b/tests/unit/query_cost_estimator.cpp @@ -153,7 +153,7 @@ TEST_F(QueryCostEstimator, Expand) { TEST_F(QueryCostEstimator, ExpandVariable) { MakeOp(NextSymbol(), NextSymbol(), EdgeAtom::Direction::IN, - nullptr, nullptr, last_op_, NextSymbol(), false, + false, nullptr, nullptr, last_op_, NextSymbol(), false, false); EXPECT_COST(CardParam::kExpandVariable * CostParam::kExpandVariable); } diff --git a/tests/unit/query_plan_match_filter_return.cpp b/tests/unit/query_plan_match_filter_return.cpp index d19b455a8..eb28b2d90 100644 --- a/tests/unit/query_plan_match_filter_return.cpp +++ b/tests/unit/query_plan_match_filter_return.cpp @@ -337,6 +337,9 @@ class QueryPlanExpandVariable : public testing::Test { * ops and plain Expand (depending on template param). * When creating plain Expand the bound arguments (lower, upper) are ignored. * + * @param is_reverse Set to true if ExpandVariable should produce the list of + * edges in reverse order. As if ExpandVariable starts from `node_to` and ends + * with `node_from`. * @return the last created logical op. */ template @@ -346,12 +349,13 @@ class QueryPlanExpandVariable : public testing::Test { std::experimental::optional lower, std::experimental::optional upper, Symbol edge_sym, bool existing_edge, const std::string &node_to, - GraphView graph_view = GraphView::AS_IS) { + GraphView graph_view = GraphView::AS_IS, bool is_reverse = false) { auto n_from = MakeScanAll(storage, symbol_table, node_from, input_op); auto filter_op = std::make_shared( - n_from.op_, storage.Create( - n_from.node_->identifier_, - std::vector{labels[layer]})); + n_from.op_, + storage.Create( + n_from.node_->identifier_, + std::vector{labels[layer]})); auto n_to = NODE(node_to); auto n_to_sym = symbol_table.CreateSymbol(node_to, true); @@ -363,8 +367,9 @@ class QueryPlanExpandVariable : public testing::Test { return bound ? LITERAL(static_cast(bound.value())) : nullptr; }; return std::make_shared( - n_to_sym, edge_sym, direction, convert(lower), convert(upper), - filter_op, n_from.sym_, false, existing_edge, graph_view); + n_to_sym, edge_sym, direction, is_reverse, convert(lower), + convert(upper), filter_op, n_from.sym_, false, existing_edge, + graph_view); } else return std::make_shared(n_to_sym, edge_sym, direction, filter_op, n_from.sym_, false, existing_edge, @@ -404,43 +409,59 @@ class QueryPlanExpandVariable : public testing::Test { TEST_F(QueryPlanExpandVariable, OneVariableExpansion) { auto test_expand = [&](int layer, EdgeAtom::Direction direction, std::experimental::optional lower, - std::experimental::optional upper) { + std::experimental::optional upper, + bool reverse) { auto e = Edge("r", direction); - return GetResults(AddMatch(nullptr, "n", layer, direction, - lower, upper, e, false, "m"), - e); + return GetResults( + AddMatch(nullptr, "n", layer, direction, lower, upper, + e, false, "m", GraphView::AS_IS, reverse), + e); }; - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, 0, 0), (map_int{{0, 2}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 0, 0), (map_int{{0, 2}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 0, 0), (map_int{{0, 2}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, 1, 1), (map_int{})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 1, 1), (map_int{{1, 4}})); - EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, 1, 1), (map_int{{1, 4}})); - EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, 1, 1), (map_int{{1, 4}})); - EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, 1, 1), (map_int{{1, 8}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 2, 2), (map_int{{2, 8}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 2, 3), (map_int{{2, 8}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 1, 2), - (map_int{{1, 4}, {2, 8}})); + for (int reverse = 0; reverse < 2; ++reverse) { + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, 0, 0, reverse), + (map_int{{0, 2}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 0, 0, reverse), + (map_int{{0, 2}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 0, 0, reverse), + (map_int{{0, 2}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::IN, 1, 1, reverse), + (map_int{})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 1, 1, reverse), + (map_int{{1, 4}})); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::IN, 1, 1, reverse), + (map_int{{1, 4}})); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::OUT, 1, 1, reverse), + (map_int{{1, 4}})); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, 1, 1, reverse), + (map_int{{1, 8}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 2, 2, reverse), + (map_int{{2, 8}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 2, 3, reverse), + (map_int{{2, 8}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, 1, 2, reverse), + (map_int{{1, 4}, {2, 8}})); - // the following tests also check edge-uniqueness (cyphermorphisim) - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 1, 2), - (map_int{{1, 4}, {2, 12}})); - EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, 4, 4), - (map_int{{4, 24}})); + // the following tests also check edge-uniqueness (cyphermorphisim) + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 1, 2, reverse), + (map_int{{1, 4}, {2, 12}})); + EXPECT_EQ(test_expand(1, EdgeAtom::Direction::BOTH, 4, 4, reverse), + (map_int{{4, 24}})); - // default bound values (lower default is 1, upper default is inf) - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, nullopt, 0), (map_int{})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, nullopt, 1), - (map_int{{1, 4}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, nullopt, 2), - (map_int{{1, 4}, {2, 8}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 7, nullopt), - (map_int{{7, 24}, {8, 24}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 8, nullopt), - (map_int{{8, 24}})); - EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 9, nullopt), (map_int{})); + // default bound values (lower default is 1, upper default is inf) + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, nullopt, 0, reverse), + (map_int{})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, nullopt, 1, reverse), + (map_int{{1, 4}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::OUT, nullopt, 2, reverse), + (map_int{{1, 4}, {2, 8}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 7, nullopt, reverse), + (map_int{{7, 24}, {8, 24}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 8, nullopt, reverse), + (map_int{{8, 24}})); + EXPECT_EQ(test_expand(0, EdgeAtom::Direction::BOTH, 9, nullopt, reverse), + (map_int{})); + } } TEST_F(QueryPlanExpandVariable, EdgeUniquenessSingleAndVariableExpansion) { diff --git a/tests/unit/query_variable_start_planner.cpp b/tests/unit/query_variable_start_planner.cpp index 4a170ef63..4a7e0dbe0 100644 --- a/tests/unit/query_variable_start_planner.cpp +++ b/tests/unit/query_variable_start_planner.cpp @@ -211,4 +211,28 @@ TEST(TestVariableStartPlanner, MatchWithMatchReturn) { }); } +TEST(TestVariableStartPlanner, MatchVariableExpand) { + Dbms dbms; + auto dba = dbms.active(); + // Graph (v1) -[:r1]-> (v2) -[:r2]-> (v3) + auto v1 = dba->InsertVertex(); + auto v2 = dba->InsertVertex(); + auto v3 = dba->InsertVertex(); + auto r1 = dba->InsertEdge(v1, v2, dba->EdgeType("r1")); + auto r2 = dba->InsertEdge(v2, v3, dba->EdgeType("r2")); + dba->AdvanceCommand(); + // Test MATCH (n) -[r*]-> (m) RETURN r + AstTreeStorage storage; + auto edge = EDGE("r", Direction::OUT); + edge->has_range_ = true; + QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("r")); + // We expect to get a single column with the following rows: + TypedValue r1_list(std::vector{r1}); // [r1] + TypedValue r2_list(std::vector{r2}); // [r2] + TypedValue r1_r2_list(std::vector{r1, r2}); // [r1, r2] + CheckPlansProduce(2, storage, *dba, [&](const auto &results) { + AssertRows(results, {{r1_list}, {r2_list}, {r1_r2_list}}); + }); +} + } // namespace