From 60ec94fb30317b8716ae5470eeefc8bffa395076 Mon Sep 17 00:00:00 2001 From: Lovro Lugovic Date: Thu, 18 Oct 2018 11:16:32 +0200 Subject: [PATCH] Fix `total_weight` not being returned with `RETURN *` Reviewers: teon.banek, mtomic Reviewed By: teon.banek Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D1665 --- src/query/frontend/ast/ast.lcp | 3 +- src/query/plan/rule_based_planner.hpp | 4 ++ src/utils/visitor.hpp | 5 +-- tests/unit/query_common.hpp | 50 ++++++++++++++++----- tests/unit/query_plan.cpp | 40 +++++++++++++++-- tests/unit/query_semantic.cpp | 12 +++-- tests/unit/query_variable_start_planner.cpp | 7 +-- 7 files changed, 93 insertions(+), 28 deletions(-) diff --git a/src/query/frontend/ast/ast.lcp b/src/query/frontend/ast/ast.lcp index 04cc8e41a..5bfe7ba5c 100644 --- a/src/query/frontend/ast/ast.lcp +++ b/src/query/frontend/ast/ast.lcp @@ -1623,8 +1623,7 @@ cpp<# (lcp:define-struct return-body () ((distinct :bool :initval "false" - :documentation "True if distinct results should be produced." - ) + :documentation "True if distinct results should be produced.") (all-identifiers :bool :initval "false" :documentation "True if asterisk was found in the return body.") (named-expressions "std::vector" diff --git a/src/query/plan/rule_based_planner.hpp b/src/query/plan/rule_based_planner.hpp index 0b1529a1e..2984b8e7c 100644 --- a/src/query/plan/rule_based_planner.hpp +++ b/src/query/plan/rule_based_planner.hpp @@ -462,6 +462,10 @@ class RuleBasedPlanner { bound_symbols.erase(filter_lambda.inner_edge_symbol); bound_symbols.erase(filter_lambda.inner_node_symbol); + if (total_weight) { + bound_symbols.insert(*total_weight); + } + // TODO: Pass weight lambda. last_op = std::make_unique( node_symbol, edge_symbol, edge->type_, expansion.direction, diff --git a/src/utils/visitor.hpp b/src/utils/visitor.hpp index 8f164485f..6d621c36f 100644 --- a/src/utils/visitor.hpp +++ b/src/utils/visitor.hpp @@ -68,7 +68,6 @@ class VisitorBase { virtual ReturnType Visit(T &) = 0; }; - template class CompositeVisitorBase; @@ -192,7 +191,7 @@ using LeafVisitor = Visitor; /// // Custom implementation for *composite* AddOp expression. /// } /// -/// void Visit(Identifier &identifier) override { +/// bool Visit(Identifier &identifier) override { /// // Custom implementation for *leaf* Identifier expression. /// } /// }; @@ -220,7 +219,7 @@ class CompositeVisitor : public detail::CompositeVisitorBase { /// @code /// class Expression : public Visitable { ... }; /// -/// class Identifier : public ExpressionVisitor { +/// class Identifier : public Expression { /// public: /// // Use default Accept implementation, since this is a *leaf* type. /// DEFVISITABLE(ExpressionVisitor) diff --git a/tests/unit/query_common.hpp b/tests/unit/query_common.hpp index 388b5e38f..ea94b849b 100644 --- a/tests/unit/query_common.hpp +++ b/tests/unit/query_common.hpp @@ -148,19 +148,41 @@ auto GetEdge(AstStorage &storage, const std::string &name, /// /// Name is used to create the Identifier which is assigned to the edge. auto GetEdgeVariable(AstStorage &storage, const std::string &name, + EdgeAtom::Type type = EdgeAtom::Type::DEPTH_FIRST, EdgeAtom::Direction dir = EdgeAtom::Direction::BOTH, const std::vector &edge_types = {}, - Identifier *inner_edge = nullptr, - Identifier *inner_node = nullptr) { - auto r_val = - storage.Create(storage.Create(name), - EdgeAtom::Type::DEPTH_FIRST, dir, edge_types); + Identifier *flambda_inner_edge = nullptr, + Identifier *flambda_inner_node = nullptr, + Identifier *wlambda_inner_edge = nullptr, + Identifier *wlambda_inner_node = nullptr, + Expression *wlambda_expression = nullptr, + Identifier *total_weight = nullptr) { + auto r_val = storage.Create(storage.Create(name), type, + dir, edge_types); + r_val->filter_lambda_.inner_edge = - inner_edge ? inner_edge - : storage.Create(utils::RandomString(20)); + flambda_inner_edge ? flambda_inner_edge + : storage.Create(utils::RandomString(20)); r_val->filter_lambda_.inner_node = - inner_node ? inner_node - : storage.Create(utils::RandomString(20)); + flambda_inner_node ? flambda_inner_node + : storage.Create(utils::RandomString(20)); + + if (type == EdgeAtom::Type::WEIGHTED_SHORTEST_PATH) { + r_val->weight_lambda_.inner_edge = + wlambda_inner_edge + ? wlambda_inner_edge + : storage.Create(utils::RandomString(20)); + r_val->weight_lambda_.inner_node = + wlambda_inner_node + ? wlambda_inner_node + : storage.Create(utils::RandomString(20)); + r_val->weight_lambda_.expression = + wlambda_expression ? wlambda_expression + : storage.Create(1); + + r_val->total_weight_ = total_weight; + } + return r_val; } @@ -268,9 +290,13 @@ void FillReturnBody(AstStorage &, ReturnBody &body, } void FillReturnBody(AstStorage &storage, ReturnBody &body, const std::string &name) { - auto *ident = storage.Create(name); - auto *named_expr = storage.Create(name, ident); - body.named_expressions.emplace_back(named_expr); + if (name == "*") { + body.all_identifiers = true; + } else { + auto *ident = storage.Create(name); + auto *named_expr = storage.Create(name, ident); + body.named_expressions.emplace_back(named_expr); + } } void FillReturnBody(AstStorage &, ReturnBody &body, Limit limit) { body.limit = limit.expression; diff --git a/tests/unit/query_plan.cpp b/tests/unit/query_plan.cpp index 60eb22fe5..4ced0754b 100644 --- a/tests/unit/query_plan.cpp +++ b/tests/unit/query_plan.cpp @@ -33,6 +33,7 @@ using query::SingleQuery; using query::Symbol; using query::SymbolGenerator; using query::SymbolTable; +using Type = query::EdgeAtom::Type; using Direction = query::EdgeAtom::Direction; using Bound = ScanAllByLabelPropertyRange::Bound; @@ -1183,7 +1184,7 @@ TYPED_TEST(TestPlanner, MatchExpandVariableInlinedFilter) { auto type = dba.EdgeType("type"); auto prop = PROPERTY_PAIR("prop"); AstStorage storage; - auto edge = EDGE_VARIABLE("r", Direction::BOTH, {type}); + auto edge = EDGE_VARIABLE("r", Type::DEPTH_FIRST, Direction::BOTH, {type}); edge->properties_[prop] = LITERAL(42); auto *query = QUERY( SINGLE_QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("r"))); @@ -1199,7 +1200,7 @@ TYPED_TEST(TestPlanner, MatchExpandVariableNotInlinedFilter) { auto type = dba.EdgeType("type"); auto prop = PROPERTY_PAIR("prop"); AstStorage storage; - auto edge = EDGE_VARIABLE("r", Direction::BOTH, {type}); + auto edge = EDGE_VARIABLE("r", Type::DEPTH_FIRST, Direction::BOTH, {type}); edge->properties_[prop] = EQ(PROPERTY_LOOKUP("m", prop), LITERAL(42)); auto *query = QUERY( SINGLE_QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("r"))); @@ -1207,10 +1208,41 @@ TYPED_TEST(TestPlanner, MatchExpandVariableNotInlinedFilter) { ExpectFilter(), ExpectProduce()); } +TYPED_TEST(TestPlanner, MatchExpandVariableTotalWeightSymbol) { + // Test MATCH p = (a {id: 0})-[r* wShortest (e, v | 1) total_weight]->(b) + // RETURN * + FakeDbAccessor dba; + AstStorage storage; + + auto edge = EDGE_VARIABLE("r", Type::WEIGHTED_SHORTEST_PATH, Direction::BOTH, + {}, nullptr, nullptr, nullptr, nullptr, nullptr, + IDENT("total_weight")); + auto *query = QUERY( + SINGLE_QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("*"))); + auto symbol_table = MakeSymbolTable(*query); + auto planner = MakePlanner(dba, storage, symbol_table, query); + auto *root = dynamic_cast(&planner.plan()); + + ASSERT_TRUE(root); + + const auto &nes = root->named_expressions_; + EXPECT_TRUE(nes.size() == 4); + + std::vector names(nes.size()); + std::transform(nes.begin(), nes.end(), names.begin(), + [](const auto *ne) { return ne->name_; }); + + EXPECT_TRUE(root->named_expressions_.size() == 4); + EXPECT_TRUE(utils::Contains(names, "m")); + EXPECT_TRUE(utils::Contains(names, "n")); + EXPECT_TRUE(utils::Contains(names, "r")); + EXPECT_TRUE(utils::Contains(names, "total_weight")); +} + TYPED_TEST(TestPlanner, UnwindMatchVariable) { // Test UNWIND [1,2,3] AS depth MATCH (n) -[r*d]-> (m) RETURN r AstStorage storage; - auto edge = EDGE_VARIABLE("r", Direction::OUT); + auto edge = EDGE_VARIABLE("r", Type::DEPTH_FIRST, Direction::OUT); edge->lower_bound_ = IDENT("d"); edge->upper_bound_ = IDENT("d"); auto *query = QUERY( @@ -1295,7 +1327,7 @@ TYPED_TEST(TestPlanner, MatchWhereAndSplit) { TYPED_TEST(TestPlanner, ReturnAsteriskOmitsLambdaSymbols) { // Test MATCH (n) -[r* (ie, in | true)]- (m) RETURN * AstStorage storage; - auto edge = EDGE_VARIABLE("r", Direction::BOTH); + auto edge = EDGE_VARIABLE("r", Type::DEPTH_FIRST, Direction::BOTH); edge->filter_lambda_.inner_edge = IDENT("ie"); edge->filter_lambda_.inner_node = IDENT("in"); edge->filter_lambda_.expression = LITERAL(true); diff --git a/tests/unit/query_semantic.cpp b/tests/unit/query_semantic.cpp index 85ce9ef70..1b508e077 100644 --- a/tests/unit/query_semantic.cpp +++ b/tests/unit/query_semantic.cpp @@ -723,7 +723,8 @@ TEST_F(TestSymbolGenerator, MatchVariablePathUsingUnboundIdentifier) { TEST_F(TestSymbolGenerator, CreateVariablePath) { // Test CREATE (n) -[r *]-> (m) raises a SemanticException, since variable // paths cannot be created. - auto edge = EDGE_VARIABLE("r", EdgeAtom::Direction::OUT); + auto edge = + EDGE_VARIABLE("r", EdgeAtom::Type::DEPTH_FIRST, EdgeAtom::Direction::OUT); auto query = QUERY(SINGLE_QUERY(CREATE(PATTERN(NODE("n"), edge, NODE("m"))))); EXPECT_THROW(query->Accept(symbol_generator), SemanticException); } @@ -731,7 +732,8 @@ TEST_F(TestSymbolGenerator, CreateVariablePath) { TEST_F(TestSymbolGenerator, MergeVariablePath) { // Test MERGE (n) -[r *]-> (m) raises a SemanticException, since variable // paths cannot be created. - auto edge = EDGE_VARIABLE("r", EdgeAtom::Direction::OUT); + auto edge = + EDGE_VARIABLE("r", EdgeAtom::Type::DEPTH_FIRST, EdgeAtom::Direction::OUT); auto query = QUERY(SINGLE_QUERY(MERGE(PATTERN(NODE("n"), edge, NODE("m"))))); EXPECT_THROW(query->Accept(symbol_generator), SemanticException); } @@ -741,7 +743,8 @@ TEST_F(TestSymbolGenerator, RedeclareVariablePath) { // This is just a temporary solution, before we add the support for using // variable paths with already declared symbols. In the future, this test // should be changed to check for type errors. - auto edge = EDGE_VARIABLE("n", EdgeAtom::Direction::OUT); + auto edge = + EDGE_VARIABLE("n", EdgeAtom::Type::DEPTH_FIRST, EdgeAtom::Direction::OUT); auto query = QUERY( SINGLE_QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("n"))); EXPECT_THROW(query->Accept(symbol_generator), RedeclareVariableError); @@ -752,7 +755,8 @@ TEST_F(TestSymbolGenerator, VariablePathSameIdentifier) { // `r` cannot be used inside the range expression, since it is bound by the // variable expansion itself. auto prop = dba.Property("prop"); - auto edge = EDGE_VARIABLE("r", EdgeAtom::Direction::OUT); + auto edge = + EDGE_VARIABLE("r", EdgeAtom::Type::DEPTH_FIRST, EdgeAtom::Direction::OUT); edge->lower_bound_ = PROPERTY_LOOKUP("r", prop); auto query = QUERY( SINGLE_QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("r"))); diff --git a/tests/unit/query_variable_start_planner.cpp b/tests/unit/query_variable_start_planner.cpp index c34978243..f712518c4 100644 --- a/tests/unit/query_variable_start_planner.cpp +++ b/tests/unit/query_variable_start_planner.cpp @@ -11,6 +11,7 @@ using namespace query::plan; using query::AstStorage; +using Type = query::EdgeAtom::Type; using Direction = query::EdgeAtom::Direction; namespace std { @@ -225,7 +226,7 @@ TEST(TestVariableStartPlanner, MatchVariableExpand) { dba->AdvanceCommand(); // Test MATCH (n) -[r*]-> (m) RETURN r AstStorage storage; - auto edge = EDGE_VARIABLE("r", Direction::OUT); + auto edge = EDGE_VARIABLE("r", Type::DEPTH_FIRST, Direction::OUT); auto *query = QUERY( SINGLE_QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("r"))); // We expect to get a single column with the following rows: @@ -254,7 +255,7 @@ TEST(TestVariableStartPlanner, MatchVariableExpandReferenceNode) { dba.AdvanceCommand(); // Test MATCH (n) -[r*..n.id]-> (m) RETURN r AstStorage storage; - auto edge = EDGE_VARIABLE("r", Direction::OUT); + auto edge = EDGE_VARIABLE("r", Type::DEPTH_FIRST, Direction::OUT); edge->upper_bound_ = PROPERTY_LOOKUP("n", id); auto *query = QUERY( SINGLE_QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), RETURN("r"))); @@ -280,7 +281,7 @@ TEST(TestVariableStartPlanner, MatchVariableExpandBoth) { dba->AdvanceCommand(); // Test MATCH (n {id:1}) -[r*]- (m) RETURN r AstStorage storage; - auto edge = EDGE_VARIABLE("r", Direction::BOTH); + auto edge = EDGE_VARIABLE("r", Type::DEPTH_FIRST, Direction::BOTH); auto node_n = NODE("n"); node_n->properties_[std::make_pair("id", id)] = LITERAL(1); auto *query =