diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b7415c8e..2035f44a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * `counter` and `counterSet` functions added. * `indexInfo` function added. * `collect` aggregation now supports Map collection. +* Changed the BFS syntax. ### Bug Fixes and Other Changes @@ -20,6 +21,7 @@ * Keywords appearing in header (named expressions) keep original case. * Our Bolt protocol implementation is now completely compatible with the protocol version 1 specification. (https://boltprotocol.org/v1/) * Added a log warning when running out of memory and the `memory_warning_threshold` flag +* Edges are no longer additionally filtered after expansion. ## v0.7.0 diff --git a/docs/user_technical/open-cypher.md b/docs/user_technical/open-cypher.md index 03e87a4dd..bb421ce6f 100644 --- a/docs/user_technical/open-cypher.md +++ b/docs/user_technical/open-cypher.md @@ -410,30 +410,25 @@ Memgraph decided to offer the same functionality using the edge expansion syntax Finding the shortest path between nodes can be done using breadth-first expansion: - MATCH (a {id: 723})-bfs[r:Type](e, n | true, 10)-(b {id : 882}) RETURN * + MATCH (a {id: 723})-[r:Type \*bfs..10]-(b {id : 882}) RETURN * The above query will find all paths of length up to 10 between nodes `a` and `b`. -Just like in variable-length expansion, a single edge element in the query -pattern denotes a possibly longer path. +The edge type and maximum path length are used in the same way like in variable +length expansion. To find only the shortest path, simply append LIMIT 1 to the RETURN clause. - MATCH (a {id: 723})-bfs[r:Type](e, n | true, 10)-(b {id : 882}) RETURN * LIMIT 1 + MATCH (a {id: 723})-[r:Type \*bfs..10]-(b {id : 882}) RETURN * LIMIT 1 Breadth-fist expansion allows an arbitrary expression filter that determines -if an expansion is allowed. In the above query that expression is `true`, meaning -that all expansions are allowed. Following is an example in which expansion is +if an expansion is allowed. Following is an example in which expansion is allowed only over edges whose `x` property is greater then `12` and nodes `y` whose property is lesser then `3`: - MATCH (a {id: 723})-bfs[](e, n | e.x > 12 and n.y < 3, 10)-() RETURN * + MATCH (a {id: 723})-[\*bfs..10 (e, n | e.x > 12 and n.y < 3)]-() RETURN * -Notice how the filtering expression uses `e` and `n` symbols. These symbols -don't have a fixed name, which is exactly why they have to be declared as the first -two arguments of the breadth-first expansion. - -The edge symbol and edge types in the square brackets can be omitted, just like -in ordinary expansion. All other arguments (in the round brackets) are obligatory. +The filter is defined as a lambda function over `e` and `n`, which denote the edge +and node being expanded over in the breadth first search. There are a few benefits of the breadth-first expansion approach, as compared to the `shortestPath` function of Neo4j. For one, it is possible to inject @@ -443,8 +438,9 @@ nodes regardless of their length. Also, it is possible to simply go through a no neighbourhood in breadth-first manner. It is fair to say there are a few drawbacks too. Currently, it isn't possible to get -all shortest paths to a single node using Memgraph's breadth-first expansion. Also, -the syntax is a bit unwieldy. +all shortest paths to a single node using Memgraph's breadth-first expansion. Also +property maps (in curly brackets) are not supported with a BFS. These features will +most likely be included in subsequent Memgraph releases. #### UNWIND diff --git a/src/query/frontend/ast/ast.hpp b/src/query/frontend/ast/ast.hpp index 2e6f98aab..323f57b96 100644 --- a/src/query/frontend/ast/ast.hpp +++ b/src/query/frontend/ast/ast.hpp @@ -16,14 +16,14 @@ namespace query { #define CLONE_BINARY_EXPRESSION \ auto Clone(AstTreeStorage &storage) const->std::remove_const< \ - std::remove_pointer::type>::type *override { \ + std::remove_pointer::type>::type * override { \ return storage.Create< \ std::remove_cv::type>::type>( \ expression1_->Clone(storage), expression2_->Clone(storage)); \ } #define CLONE_UNARY_EXPRESSION \ auto Clone(AstTreeStorage &storage) const->std::remove_const< \ - std::remove_pointer::type>::type *override { \ + std::remove_pointer::type>::type * override { \ return storage.Create< \ std::remove_cv::type>::type>( \ expression_->Clone(storage)); \ @@ -737,35 +737,6 @@ class LabelsTest : public Expression { : Expression(uid), expression_(expression), labels_(labels) {} }; -class EdgeTypeTest : public Expression { - friend class AstTreeStorage; - - public: - DEFVISITABLE(TreeVisitor); - bool Accept(HierarchicalTreeVisitor &visitor) override { - if (visitor.PreVisit(*this)) { - expression_->Accept(visitor); - } - return visitor.PostVisit(*this); - } - - EdgeTypeTest *Clone(AstTreeStorage &storage) const override { - return storage.Create(expression_->Clone(storage), - edge_types_); - } - - Expression *expression_ = nullptr; - std::vector edge_types_; - - protected: - EdgeTypeTest(int uid, Expression *expression, - std::vector edge_types) - : Expression(uid), expression_(expression), edge_types_(edge_types) { - debug_assert(edge_types.size(), - "EdgeTypeTest must have at least one edge_type"); - } -}; - class Function : public Expression { friend class AstTreeStorage; @@ -1053,35 +1024,44 @@ class EdgeAtom : public PatternAtom { }; class BreadthFirstAtom : public EdgeAtom { - // TODO: Reconsider inheriting from EdgeAtom, since only `direction_` and - // `edge_types_` are used. friend class AstTreeStorage; + template + static TPtr *CloneOpt(TPtr *ptr, AstTreeStorage &storage) { + return ptr ? ptr->Clone(storage) : nullptr; + } + public: DEFVISITABLE(TreeVisitor); bool Accept(HierarchicalTreeVisitor &visitor) override { if (visitor.PreVisit(*this)) { - identifier_->Accept(visitor) && - traversed_edge_identifier_->Accept(visitor) && - next_node_identifier_->Accept(visitor) && - filter_expression_->Accept(visitor) && max_depth_->Accept(visitor); + bool cont = identifier_->Accept(visitor); + if (cont && traversed_edge_identifier_) + cont = traversed_edge_identifier_->Accept(visitor); + if (cont && next_node_identifier_) + cont = next_node_identifier_->Accept(visitor); + if (cont && filter_expression_) + cont = filter_expression_->Accept(visitor); } return visitor.PostVisit(*this); } BreadthFirstAtom *Clone(AstTreeStorage &storage) const override { - return storage.Create( + auto bfs_atom = storage.Create( identifier_->Clone(storage), direction_, edge_types_, - traversed_edge_identifier_->Clone(storage), - next_node_identifier_->Clone(storage), - filter_expression_->Clone(storage), max_depth_->Clone(storage)); + CloneOpt(traversed_edge_identifier_, storage), + CloneOpt(next_node_identifier_, storage), + CloneOpt(filter_expression_, storage)); + bfs_atom->has_range_ = has_range_; + bfs_atom->lower_bound_ = CloneOpt(lower_bound_, storage); + bfs_atom->upper_bound_ = CloneOpt(upper_bound_, storage); + return bfs_atom; } Identifier *traversed_edge_identifier_ = nullptr; Identifier *next_node_identifier_ = nullptr; // Expression which evaluates to true in order to continue the BFS. Expression *filter_expression_ = nullptr; - Expression *max_depth_ = nullptr; protected: using EdgeAtom::EdgeAtom; @@ -1089,12 +1069,13 @@ class BreadthFirstAtom : public EdgeAtom { const std::vector &edge_types, Identifier *traversed_edge_identifier, Identifier *next_node_identifier, - Expression *filter_expression, Expression *max_depth) + Expression *filter_expression) : EdgeAtom(uid, identifier, direction, edge_types), traversed_edge_identifier_(traversed_edge_identifier), next_node_identifier_(next_node_identifier), - filter_expression_(filter_expression), - max_depth_(max_depth) {} + filter_expression_(filter_expression) { + has_range_ = true; + } }; class Pattern : public Tree { diff --git a/src/query/frontend/ast/ast_visitor.hpp b/src/query/frontend/ast/ast_visitor.hpp index c92ad9c66..2cd2f594b 100644 --- a/src/query/frontend/ast/ast_visitor.hpp +++ b/src/query/frontend/ast/ast_visitor.hpp @@ -10,7 +10,6 @@ class NamedExpression; class Identifier; class PropertyLookup; class LabelsTest; -class EdgeTypeTest; class Aggregation; class Function; class All; @@ -66,10 +65,10 @@ using TreeCompositeVisitor = ::utils::CompositeVisitor< LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator, InListOperator, ListMapIndexingOperator, ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral, - MapLiteral, PropertyLookup, LabelsTest, EdgeTypeTest, Aggregation, Function, - All, Create, Match, Return, With, Pattern, NodeAtom, EdgeAtom, - BreadthFirstAtom, Delete, Where, SetProperty, SetProperties, SetLabels, - RemoveProperty, RemoveLabels, Merge, Unwind>; + MapLiteral, PropertyLookup, LabelsTest, Aggregation, Function, All, Create, + Match, Return, With, Pattern, NodeAtom, EdgeAtom, BreadthFirstAtom, Delete, + Where, SetProperty, SetProperties, SetLabels, RemoveProperty, RemoveLabels, + Merge, Unwind>; using TreeLeafVisitor = ::utils::LeafVisitor; @@ -91,10 +90,10 @@ using TreeVisitor = ::utils::Visitor< LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator, InListOperator, ListMapIndexingOperator, ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral, - MapLiteral, PropertyLookup, LabelsTest, EdgeTypeTest, Aggregation, Function, - All, ParameterLookup, Create, Match, Return, With, Pattern, NodeAtom, - EdgeAtom, BreadthFirstAtom, Delete, Where, SetProperty, SetProperties, - SetLabels, RemoveProperty, RemoveLabels, Merge, Unwind, Identifier, - PrimitiveLiteral, CreateIndex>; + MapLiteral, PropertyLookup, LabelsTest, Aggregation, Function, All, + ParameterLookup, Create, Match, Return, With, Pattern, NodeAtom, EdgeAtom, + BreadthFirstAtom, Delete, Where, SetProperty, SetProperties, SetLabels, + RemoveProperty, RemoveLabels, Merge, Unwind, Identifier, PrimitiveLiteral, + CreateIndex>; } // namespace query diff --git a/src/query/frontend/ast/cypher_main_visitor.cpp b/src/query/frontend/ast/cypher_main_visitor.cpp index ff79ee504..fa2e06ca1 100644 --- a/src/query/frontend/ast/cypher_main_visitor.cpp +++ b/src/query/frontend/ast/cypher_main_visitor.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -417,65 +418,21 @@ antlrcpp::Any CypherMainVisitor::visitPatternElementChain( antlrcpp::Any CypherMainVisitor::visitRelationshipPattern( CypherParser::RelationshipPatternContext *ctx) { - auto *edge = ctx->bfsDetail() ? storage_.Create() - : storage_.Create(); - if (ctx->bfsDetail()) { - if (ctx->bfsDetail()->bfs_variable) { - std::string variable = ctx->bfsDetail()->bfs_variable->accept(this); - edge->identifier_ = storage_.Create(variable); - users_identifiers.insert(variable); - } - auto *bf_atom = dynamic_cast(edge); - std::string traversed_edge_variable = - ctx->bfsDetail()->traversed_edge->accept(this); - if (ctx->bfsDetail()->relationshipTypes()) { - bf_atom->edge_types_ = ctx->bfsDetail() - ->relationshipTypes() - ->accept(this) - .as>(); - } - bf_atom->traversed_edge_identifier_ = - storage_.Create(traversed_edge_variable); - std::string next_node_variable = ctx->bfsDetail()->next_node->accept(this); - bf_atom->next_node_identifier_ = - storage_.Create(next_node_variable); - bf_atom->filter_expression_ = - ctx->bfsDetail()->expression()[0]->accept(this); - bf_atom->max_depth_ = ctx->bfsDetail()->expression()[1]->accept(this); - } else if (ctx->relationshipDetail()) { - if (ctx->relationshipDetail()->variable()) { - std::string variable = - ctx->relationshipDetail()->variable()->accept(this); - edge->identifier_ = storage_.Create(variable); - users_identifiers.insert(variable); - } - if (ctx->relationshipDetail()->relationshipTypes()) { - edge->edge_types_ = ctx->relationshipDetail() - ->relationshipTypes() - ->accept(this) - .as>(); - } - if (ctx->relationshipDetail()->properties()) { - edge->properties_ = - ctx->relationshipDetail() - ->properties() - ->accept(this) - .as, - Expression *>>(); - } - if (ctx->relationshipDetail()->rangeLiteral()) { - edge->has_range_ = true; - auto range = ctx->relationshipDetail() - ->rangeLiteral() - ->accept(this) - .as>(); - edge->lower_bound_ = range.first; - edge->upper_bound_ = range.second; - } - } - if (!edge->identifier_) { - anonymous_identifiers.push_back(&edge->identifier_); - } + auto relationshipDetail = ctx->relationshipDetail(); + auto *variableExpansion = + relationshipDetail ? relationshipDetail->variableExpansion() : nullptr; + bool is_bfs = false; + // Range expressions, only used in variable length and BFS. + Expression *lower = nullptr; + Expression *upper = nullptr; + if (variableExpansion) + std::tie(is_bfs, lower, upper) = + variableExpansion->accept(this) + .as>(); + + auto *edge = (variableExpansion && is_bfs) + ? storage_.Create() + : storage_.Create(); if (ctx->leftArrowHead() && !ctx->rightArrowHead()) { edge->direction_ = EdgeAtom::Direction::IN; @@ -486,6 +443,80 @@ antlrcpp::Any CypherMainVisitor::visitRelationshipPattern( // grammar. edge->direction_ = EdgeAtom::Direction::BOTH; } + + if (!relationshipDetail) { + anonymous_identifiers.push_back(&edge->identifier_); + return edge; + } + + if (relationshipDetail->variable()) { + std::string variable = relationshipDetail->variable()->accept(this); + edge->identifier_ = storage_.Create(variable); + users_identifiers.insert(variable); + } else { + anonymous_identifiers.push_back(&edge->identifier_); + } + + if (relationshipDetail->relationshipTypes()) { + edge->edge_types_ = ctx->relationshipDetail() + ->relationshipTypes() + ->accept(this) + .as>(); + } + + auto relationshipLambdas = relationshipDetail->relationshipLambda(); + if (variableExpansion) { + edge->has_range_ = true; + edge->lower_bound_ = lower; + edge->upper_bound_ = upper; + + switch (relationshipLambdas.size()) { + case 0: + break; + case 1: { + if (!is_bfs) + throw SemanticException("Relationship lambda only supported in BFS"); + + auto *lambda = relationshipLambdas[0]; + auto *edge_bfs = dynamic_cast(edge); + std::string traversed_edge_variable = + lambda->traversed_edge->accept(this); + edge_bfs->traversed_edge_identifier_ = + storage_.Create(traversed_edge_variable); + std::string traversed_node_variable = + lambda->traversed_node->accept(this); + edge_bfs->next_node_identifier_ = + storage_.Create(traversed_node_variable); + edge_bfs->filter_expression_ = lambda->expression()->accept(this); + break; + }; + default: + throw SemanticException("Only one relationship lambda allowed"); + } + } else if (!relationshipLambdas.empty()) { + throw SemanticException("Relationship lambda only supported in BFS"); + } + + auto properties = relationshipDetail->properties(); + switch (properties.size()) { + case 0: + break; + case 1: { + // TODO support property filters in BFS + if (is_bfs) + throw SemanticException( + "BFS expansion and property maps not supported"); + edge->properties_ = + properties[0] + ->accept(this) + .as, + Expression *>>(); + break; + } + default: + throw SemanticException("Only one property map supported in edge"); + } + return edge; } @@ -495,8 +526,8 @@ antlrcpp::Any CypherMainVisitor::visitRelationshipDetail( return 0; } -antlrcpp::Any CypherMainVisitor::visitBfsDetail( - CypherParser::BfsDetailContext *) { +antlrcpp::Any CypherMainVisitor::visitRelationshipLambda( + CypherParser::RelationshipLambdaContext *) { debug_assert(false, "Should never be called. See documentation in hpp."); return 0; } @@ -510,34 +541,41 @@ antlrcpp::Any CypherMainVisitor::visitRelationshipTypes( return types; } -antlrcpp::Any CypherMainVisitor::visitRangeLiteral( - CypherParser::RangeLiteralContext *ctx) { +antlrcpp::Any CypherMainVisitor::visitVariableExpansion( + CypherParser::VariableExpansionContext *ctx) { debug_assert(ctx->expression().size() <= 2U, "Expected 0, 1 or 2 bounds in range literal."); + + bool is_bfs = !ctx->getTokens(CypherParser::BFS).empty(); + Expression *lower = nullptr; + Expression *upper = nullptr; + if (ctx->expression().size() == 0U) { // Case -[*]- - return std::pair(nullptr, nullptr); } else if (ctx->expression().size() == 1U) { auto dots_tokens = ctx->getTokens(kDotsTokenId); Expression *bound = ctx->expression()[0]->accept(this); if (!dots_tokens.size()) { // Case -[*bound]- - return std::pair(bound, bound); - } - if (dots_tokens[0]->getSourceInterval().startsAfter( - ctx->expression()[0]->getSourceInterval())) { + lower = bound; + upper = bound; + } else if (dots_tokens[0]->getSourceInterval().startsAfter( + ctx->expression()[0]->getSourceInterval())) { // Case -[*bound..]- - return std::pair(bound, nullptr); + lower = bound; } else { // Case -[*..bound]- - return std::pair(nullptr, bound); + upper = bound; } } else { // Case -[*lbound..rbound]- - Expression *lbound = ctx->expression()[0]->accept(this); - Expression *rbound = ctx->expression()[1]->accept(this); - return std::pair(lbound, rbound); + lower = ctx->expression()[0]->accept(this); + upper = ctx->expression()[1]->accept(this); } + + if (is_bfs && lower) + throw SemanticException("BFS does not support lower bounds"); + return std::make_tuple(is_bfs, lower, upper); } antlrcpp::Any CypherMainVisitor::visitExpression( diff --git a/src/query/frontend/ast/cypher_main_visitor.hpp b/src/query/frontend/ast/cypher_main_visitor.hpp index 7afae403b..82cf0f492 100644 --- a/src/query/frontend/ast/cypher_main_visitor.hpp +++ b/src/query/frontend/ast/cypher_main_visitor.hpp @@ -272,7 +272,8 @@ class CypherMainVisitor : public antlropencypher::CypherBaseVisitor { * This should never be called. Everything is done directly in * visitRelationshipPattern. */ - antlrcpp::Any visitBfsDetail(CypherParser::BfsDetailContext *ctx) override; + antlrcpp::Any visitRelationshipLambda( + CypherParser::RelationshipLambdaContext *ctx) override; /** * @return vector @@ -281,10 +282,10 @@ class CypherMainVisitor : public antlropencypher::CypherBaseVisitor { CypherParser::RelationshipTypesContext *ctx) override; /** - * @return pair. + * @return std::tuple. */ - antlrcpp::Any visitRangeLiteral( - CypherParser::RangeLiteralContext *ctx) override; + antlrcpp::Any visitVariableExpansion( + CypherParser::VariableExpansionContext *ctx) override; /** * Top level expression, does nothing. diff --git a/src/query/frontend/ast/named_antlr_tokens.hpp b/src/query/frontend/ast/named_antlr_tokens.hpp index 635be2bad..499037baf 100644 --- a/src/query/frontend/ast/named_antlr_tokens.hpp +++ b/src/query/frontend/ast/named_antlr_tokens.hpp @@ -8,7 +8,7 @@ using antlropencypher::CypherParser; // grammar change since even changes in ordering of rules will cause antlr to // generate different constants for unnamed tokens. const auto kReturnAllTokenId = CypherParser::T__4; // * -const auto kDotsTokenId = CypherParser::T__11; // .. +const auto kDotsTokenId = CypherParser::T__10; // .. const auto kEqTokenId = CypherParser::T__2; // = const auto kNeTokenId1 = CypherParser::T__18; // <> const auto kNeTokenId2 = CypherParser::T__19; // != diff --git a/src/query/frontend/opencypher/grammar/Cypher.g4 b/src/query/frontend/opencypher/grammar/Cypher.g4 index 39bcf405b..41deb0f7d 100644 --- a/src/query/frontend/opencypher/grammar/Cypher.g4 +++ b/src/query/frontend/opencypher/grammar/Cypher.g4 @@ -117,17 +117,19 @@ nodePattern : '(' SP? ( variable SP? )? ( nodeLabels SP? )? ( properties SP? )? patternElementChain : relationshipPattern SP? nodePattern ; -relationshipPattern : ( leftArrowHead SP? dash SP? ( bfsDetail | relationshipDetail )? SP? dash SP? rightArrowHead ) - | ( leftArrowHead SP? dash SP? ( bfsDetail | relationshipDetail )? SP? dash ) - | ( dash SP? ( bfsDetail | relationshipDetail )? SP? dash SP? rightArrowHead ) - | ( dash SP? ( bfsDetail | relationshipDetail )? SP? dash ) +relationshipPattern : ( leftArrowHead SP? dash SP? ( relationshipDetail )? SP? dash SP? rightArrowHead ) + | ( leftArrowHead SP? dash SP? ( relationshipDetail )? SP? dash ) + | ( dash SP? ( relationshipDetail )? SP? dash SP? rightArrowHead ) + | ( dash SP? ( relationshipDetail )? SP? dash ) ; -bfsDetail : BFS SP? ( '[' SP? ( bfs_variable=variable SP? )? ( relationshipTypes SP? )? SP? ']' )? SP? '(' SP? traversed_edge=variable SP? ',' SP? next_node=variable SP? '|' SP? expression SP? ',' SP? expression SP? ')' ; +relationshipDetail : '[' SP? ( variable SP? )? ( relationshipTypes SP? )? ( variableExpansion SP? )? properties SP? ']' + | '[' SP? ( variable SP? )? ( relationshipTypes SP? )? ( variableExpansion SP? )? relationshipLambda SP? ']' + | '[' SP? ( variable SP? )? ( relationshipTypes SP? )? ( variableExpansion SP? )? ( (properties SP?) | (relationshipLambda SP?) )* ']'; -relationshipDetail : '[' SP? ( variable SP? )? ( relationshipTypes SP? )? ( rangeLiteral SP? )? properties SP? ']' - | '[' SP? ( variable SP? )? ( relationshipTypes SP? )? ( rangeLiteral SP? )? ( properties SP? )? ']' - ; +relationshipLambda: '(' SP? traversed_edge=variable SP? ',' SP? traversed_node=variable SP? '|' SP? expression SP? ')'; + +variableExpansion : '*' SP? (BFS)? SP? ( expression SP? )? ( '..' ( SP? expression )? )? ; properties : mapLiteral | parameter @@ -139,8 +141,6 @@ nodeLabels : nodeLabel ( SP? nodeLabel )* ; nodeLabel : ':' SP? labelName ; -rangeLiteral : '*' SP? ( expression SP? )? ( '..' ( SP? expression )? )? ; - labelName : symbolicName ; relTypeName : symbolicName ; diff --git a/src/query/frontend/semantic/symbol_generator.cpp b/src/query/frontend/semantic/symbol_generator.cpp index 2cc6f009f..bf3097e0d 100644 --- a/src/query/frontend/semantic/symbol_generator.cpp +++ b/src/query/frontend/semantic/symbol_generator.cpp @@ -196,34 +196,25 @@ SymbolGenerator::ReturnType SymbolGenerator::Visit(Identifier &ident) { symbol = GetOrCreateSymbol(ident.name_, ident.user_declared_, Symbol::Type::Path); } else if (scope_.in_pattern && scope_.in_pattern_atom_identifier) { - // Patterns can bind new symbols or reference already bound. But there - // are the following special cases: - // 1) Patterns used to create nodes and edges cannot redeclare already - // established bindings. Declaration only happens in single node - // patterns and in edge patterns. OpenCypher example, - // `MATCH (n) CREATE (n)` should throw an error that `n` is already - // declared. While `MATCH (n) CREATE (n) -[:R]-> (n)` is allowed, - // since `n` now references the bound node instead of declaring it. - // Additionally, we will support edge referencing in pattern: - // `MATCH (n) - [r] -> (n) - [r] -> (n) RETURN r`, which would - // usually raise redeclaration of `r`. + // Patterns used to create nodes and edges cannot redeclare already + // established bindings. Declaration only happens in single node + // patterns and in edge patterns. OpenCypher example, + // `MATCH (n) CREATE (n)` should throw an error that `n` is already + // declared. While `MATCH (n) CREATE (n) -[:R]-> (n)` is allowed, + // since `n` now references the bound node instead of declaring it. if ((scope_.in_create_node || scope_.in_create_edge) && HasSymbol(ident.name_)) { - // Case 1) throw RedeclareVariableError(ident.name_); } auto type = Symbol::Type::Vertex; if (scope_.visiting_edge) { - if (scope_.visiting_edge->has_range_ && HasSymbol(ident.name_)) { - // TOOD: Support using variable paths with already obtained results from - // an existing symbol. + // Edge referencing is not allowed (like in Neo4j): + // `MATCH (n) - [r] -> (n) - [r] -> (n) RETURN r` is not allowed. + if (HasSymbol(ident.name_)) { throw RedeclareVariableError(ident.name_); } - if (scope_.visiting_edge->has_range_) { - type = Symbol::Type::EdgeList; - } else { - type = Symbol::Type::Edge; - } + type = scope_.visiting_edge->has_range_ ? Symbol::Type::EdgeList + : Symbol::Type::Edge; } symbol = GetOrCreateSymbol(ident.name_, ident.user_declared_, type); } else if (scope_.in_pattern && !scope_.in_pattern_atom_identifier && @@ -424,19 +415,20 @@ bool SymbolGenerator::PreVisit(BreadthFirstAtom &bf_atom) { if (scope_.in_create || scope_.in_merge) { throw SemanticException("BFS cannot be used to create edges."); } - // Visiting BFS filter and max_depth expressions is not a pattern. + // Visiting BFS filter and upper bound expressions is not a pattern. scope_.in_pattern = false; - bf_atom.max_depth_->Accept(*this); - VisitWithIdentifiers( - *bf_atom.filter_expression_, - {bf_atom.traversed_edge_identifier_, bf_atom.next_node_identifier_}); + if (bf_atom.upper_bound_) { + bf_atom.upper_bound_->Accept(*this); + } + if (bf_atom.filter_expression_) { + VisitWithIdentifiers( + *bf_atom.filter_expression_, + {bf_atom.traversed_edge_identifier_, bf_atom.next_node_identifier_}); + } scope_.in_pattern = true; - // XXX: Make BFS symbol be EdgeList. - bf_atom.has_range_ = true; scope_.in_pattern_atom_identifier = true; bf_atom.identifier_->Accept(*this); scope_.in_pattern_atom_identifier = false; - bf_atom.has_range_ = false; return false; } diff --git a/src/query/interpret/eval.hpp b/src/query/interpret/eval.hpp index b74fcd405..f0e96a0ea 100644 --- a/src/query/interpret/eval.hpp +++ b/src/query/interpret/eval.hpp @@ -305,26 +305,6 @@ class ExpressionEvaluator : public TreeVisitor { } } - TypedValue Visit(EdgeTypeTest &edge_type_test) override { - auto expression_result = edge_type_test.expression_->Accept(*this); - switch (expression_result.type()) { - case TypedValue::Type::Null: - return TypedValue::Null; - case TypedValue::Type::Edge: { - auto real_edge_type = - expression_result.Value().EdgeType(); - for (const auto edge_type : edge_type_test.edge_types_) { - if (edge_type == real_edge_type) { - return true; - } - } - return false; - } - default: - throw QueryRuntimeException("Expected Edge in edge type test"); - } - } - TypedValue Visit(PrimitiveLiteral &literal) override { // TODO: no need to evaluate constants, we can write it to frame in one // of the previous phases. diff --git a/src/query/plan/operator.cpp b/src/query/plan/operator.cpp index 4ef2d5f8f..623f09c01 100644 --- a/src/query/plan/operator.cpp +++ b/src/query/plan/operator.cpp @@ -536,10 +536,10 @@ bool Expand::ExpandCursor::InitEdges(Frame &frame, Context &context) { ExpectType(self_.node_symbol_, existing_node, TypedValue::Type::Vertex); in_edges_.emplace( - vertex.in_with_destination(existing_node.ValueVertex())); + vertex.in(existing_node.ValueVertex(), &self_.edge_types())); } } else { - in_edges_.emplace(vertex.in_with_types(&self_.edge_types())); + in_edges_.emplace(vertex.in(&self_.edge_types())); } in_edges_it_.emplace(in_edges_->begin()); } @@ -553,10 +553,10 @@ bool Expand::ExpandCursor::InitEdges(Frame &frame, Context &context) { ExpectType(self_.node_symbol_, existing_node, TypedValue::Type::Vertex); out_edges_.emplace( - vertex.out_with_destination(existing_node.ValueVertex())); + vertex.out(existing_node.ValueVertex(), &self_.edge_types())); } } else { - out_edges_.emplace(vertex.out_with_types(&self_.edge_types())); + out_edges_.emplace(vertex.out(&self_.edge_types())); } out_edges_it_.emplace(out_edges_->begin()); } @@ -608,14 +608,14 @@ auto ExpandFromVertex(const VertexAccessor &vertex, std::vector chain_elements; if (direction != EdgeAtom::Direction::OUT && vertex.in_degree() > 0) { - auto edges = vertex.in_with_types(&edge_types); + auto edges = vertex.in(&edge_types); if (edges.begin() != edges.end()) { chain_elements.emplace_back( wrapper(EdgeAtom::Direction::IN, std::move(edges))); } } if (direction != EdgeAtom::Direction::IN && vertex.out_degree() > 0) { - auto edges = vertex.out_with_types(&edge_types); + auto edges = vertex.out(&edge_types); if (edges.begin() != edges.end()) { chain_elements.emplace_back( wrapper(EdgeAtom::Direction::OUT, std::move(edges))); @@ -939,9 +939,11 @@ std::unique_ptr ExpandVariable::MakeCursor(GraphDbAccessor &db) const { ExpandBreadthFirst::ExpandBreadthFirst( Symbol node_symbol, Symbol edge_list_symbol, EdgeAtom::Direction direction, const std::vector &edge_types, - Expression *max_depth, Symbol inner_node_symbol, Symbol inner_edge_symbol, - Expression *where, const std::shared_ptr &input, - Symbol input_symbol, bool existing_node, GraphView graph_view) + Expression *max_depth, + std::experimental::optional inner_node_symbol, + std::experimental::optional inner_edge_symbol, Expression *where, + const std::shared_ptr &input, Symbol input_symbol, + bool existing_node, GraphView graph_view) : node_symbol_(node_symbol), edge_list_symbol_(edge_list_symbol), direction_(direction), @@ -981,22 +983,23 @@ bool ExpandBreadthFirst::Cursor::Pull(Frame &frame, Context &context) { SwitchAccessor(edge, self_.graph_view_); SwitchAccessor(vertex, self_.graph_view_); - // to evaluate the where expression we need the inner - // values on the frame - frame[self_.inner_edge_symbol_] = edge; - frame[self_.inner_node_symbol_] = vertex; - TypedValue result = self_.where_->Accept(evaluator); - switch (result.type()) { - case TypedValue::Type::Null: - return; - case TypedValue::Type::Bool: - if (!result.Value()) return; - break; - default: - throw QueryRuntimeException( - "Expansion condition must be boolean or null"); + if (self_.where_) { + // to evaluate the where expression we need the inner + // values on the frame + if (self_.inner_edge_symbol_) frame[*self_.inner_edge_symbol_] = edge; + if (self_.inner_node_symbol_) frame[*self_.inner_node_symbol_] = vertex; + TypedValue result = self_.where_->Accept(evaluator); + switch (result.type()) { + case TypedValue::Type::Null: + return; + case TypedValue::Type::Bool: + if (!result.Value()) return; + break; + default: + throw QueryRuntimeException( + "Expansion condition must be boolean or null"); + } } - to_visit_next_.emplace_back(edge, vertex); processed_.emplace(vertex, edge); }; @@ -1006,11 +1009,11 @@ bool ExpandBreadthFirst::Cursor::Pull(Frame &frame, Context &context) { // the "where" condition. auto expand_from_vertex = [this, &expand_pair](VertexAccessor &vertex) { if (self_.direction_ != EdgeAtom::Direction::IN) { - for (const EdgeAccessor &edge : vertex.out_with_types(&self_.edge_types_)) + for (const EdgeAccessor &edge : vertex.out(&self_.edge_types_)) expand_pair(edge, edge.to()); } if (self_.direction_ != EdgeAtom::Direction::OUT) { - for (const EdgeAccessor &edge : vertex.in_with_types(&self_.edge_types_)) + for (const EdgeAccessor &edge : vertex.in(&self_.edge_types_)) expand_pair(edge, edge.from()); } }; @@ -1032,8 +1035,10 @@ bool ExpandBreadthFirst::Cursor::Pull(Frame &frame, Context &context) { SwitchAccessor(vertex, self_.graph_view_); processed_.emplace(vertex, std::experimental::nullopt); expand_from_vertex(vertex); - max_depth_ = EvaluateInt(evaluator, self_.max_depth_, - "Max depth in breadth-first expansion"); + max_depth_ = self_.max_depth_ + ? EvaluateInt(evaluator, self_.max_depth_, + "Max depth in breadth-first expansion") + : std::numeric_limits::max(); if (max_depth_ < 1) throw QueryRuntimeException( "Max depth in breadth-first expansion must be greater then zero"); diff --git a/src/query/plan/operator.hpp b/src/query/plan/operator.hpp index 4e6ffcf8d..01c15d433 100644 --- a/src/query/plan/operator.hpp +++ b/src/query/plan/operator.hpp @@ -672,8 +672,10 @@ class ExpandBreadthFirst : public LogicalOperator { ExpandBreadthFirst(Symbol node_symbol, Symbol edge_list_symbol, EdgeAtom::Direction direction, const std::vector &edge_types, - Expression *max_depth, Symbol inner_node_symbol, - Symbol inner_edge_symbol, Expression *where, + Expression *max_depth, + std::experimental::optional inner_node_symbol, + std::experimental::optional inner_edge_symbol, + Expression *where, const std::shared_ptr &input, Symbol input_symbol, bool existing_node, GraphView graph_view = GraphView::AS_IS); @@ -717,8 +719,8 @@ class ExpandBreadthFirst : public LogicalOperator { Expression *max_depth_; // symbols for a single node and edge that are currently getting expanded - const Symbol inner_node_symbol_; - const Symbol inner_edge_symbol_; + const std::experimental::optional inner_node_symbol_; + const std::experimental::optional inner_edge_symbol_; // a filtering expression for skipping expansions during expansion // can refer to inner node and edges Expression *where_; diff --git a/src/query/plan/rule_based_planner.cpp b/src/query/plan/rule_based_planner.cpp index bd6dbfc89..511473402 100644 --- a/src/query/plan/rule_based_planner.cpp +++ b/src/query/plan/rule_based_planner.cpp @@ -530,13 +530,16 @@ std::vector NormalizePatterns( } if (auto *bf_atom = dynamic_cast(edge)) { // Get used symbols inside bfs filter expression and max depth. - bf_atom->filter_expression_->Accept(collector); - bf_atom->max_depth_->Accept(collector); + if (bf_atom->filter_expression_) + bf_atom->filter_expression_->Accept(collector); + if (bf_atom->upper_bound_) bf_atom->upper_bound_->Accept(collector); // Remove symbols which are bound by the bfs itself. - collector.symbols_.erase( - symbol_table.at(*bf_atom->traversed_edge_identifier_)); - collector.symbols_.erase( - symbol_table.at(*bf_atom->next_node_identifier_)); + if (bf_atom->traversed_edge_identifier_) { + collector.symbols_.erase( + symbol_table.at(*bf_atom->traversed_edge_identifier_)); + collector.symbols_.erase( + symbol_table.at(*bf_atom->next_node_identifier_)); + } } expansions.emplace_back(Expansion{prev_node, edge, edge->direction_, false, collector.symbols_, current_node}); @@ -974,35 +977,6 @@ void Filters::CollectPatternFilters(Pattern &pattern, SymbolTable &symbol_table, add_properties_filter(node); }; auto add_expand_filter = [&](NodeAtom *, EdgeAtom *edge, NodeAtom *node) { - const auto &edge_symbol = symbol_table.at(*edge->identifier_); - if (!edge->edge_types_.empty()) { - edge_type_filters_[edge_symbol].insert(edge->edge_types_.begin(), - edge->edge_types_.end()); - if (edge->has_range_) { - // We need a new identifier and symbol for All. - auto *ident_in_all = edge->identifier_->Clone(storage); - symbol_table[*ident_in_all] = - symbol_table.CreateSymbol(ident_in_all->name_, false); - auto *edge_type_test = - storage.Create(ident_in_all, edge->edge_types_); - all_filters_.emplace_back(FilterInfo{ - storage.Create(ident_in_all, edge->identifier_, - storage.Create(edge_type_test)), - std::unordered_set{edge_symbol}, true}); - } else if (auto *bf_atom = dynamic_cast(edge)) { - // BFS filters will be inlined inside the filter expression, so create - // EdgeTypeTest which relies on traversed edge identifier. Set of - // used symbols treats this as the original edge symbol. - all_filters_.emplace_back(FilterInfo{ - storage.Create(bf_atom->traversed_edge_identifier_, - bf_atom->edge_types_), - std::unordered_set{edge_symbol}, true}); - } else { - all_filters_.emplace_back(FilterInfo{ - storage.Create(edge->identifier_, edge->edge_types_), - std::unordered_set{edge_symbol}}); - } - } add_properties_filter(edge, edge->has_range_); add_node_filter(node); }; diff --git a/src/query/plan/rule_based_planner.hpp b/src/query/plan/rule_based_planner.hpp index 33e4738cc..664471e96 100644 --- a/src/query/plan/rule_based_planner.hpp +++ b/src/query/plan/rule_based_planner.hpp @@ -1,6 +1,8 @@ /// @file #pragma once +#include + #include "gflags/gflags.h" #include "query/frontend/ast/ast.hpp" @@ -533,17 +535,14 @@ class RuleBasedPlanner { } else { match_context.new_symbols.emplace_back(edge_symbol); } - const auto edge_types_set = - utils::FindOr(matching.filters.edge_type_filters(), edge_symbol, - std::unordered_set()) - .first; - const std::vector edge_types( - edge_types_set.begin(), edge_types_set.end()); if (auto *bf_atom = dynamic_cast(expansion.edge)) { - const auto &traversed_edge_symbol = - symbol_table.at(*bf_atom->traversed_edge_identifier_); - const auto &next_node_symbol = - symbol_table.at(*bf_atom->next_node_identifier_); + std::experimental::optional traversed_edge_symbol; + if (bf_atom->traversed_edge_identifier_) + traversed_edge_symbol = + symbol_table.at(*bf_atom->traversed_edge_identifier_); + std::experimental::optional next_node_symbol; + if (bf_atom->next_node_identifier_) + next_node_symbol = symbol_table.at(*bf_atom->next_node_identifier_); // Inline BFS edge filtering together with its filter expression. auto *filter_expr = impl::BoolJoin( storage, impl::ExtractMultiExpandFilter( @@ -551,8 +550,8 @@ class RuleBasedPlanner { bf_atom->filter_expression_); last_op = new ExpandBreadthFirst( node_symbol, edge_symbol, expansion.direction, - std::move(edge_types), bf_atom->max_depth_, next_node_symbol, - traversed_edge_symbol, filter_expr, + expansion.edge->edge_types_, bf_atom->upper_bound_, + next_node_symbol, traversed_edge_symbol, filter_expr, std::shared_ptr(last_op), node1_symbol, existing_node, match_context.graph_view); } else if (expansion.edge->has_range_) { @@ -560,7 +559,7 @@ class RuleBasedPlanner { bound_symbols, node_symbol, all_filters, storage); last_op = new ExpandVariable( node_symbol, edge_symbol, expansion.direction, - std::move(edge_types), expansion.is_flipped, + expansion.edge->edge_types_, expansion.is_flipped, expansion.edge->lower_bound_, expansion.edge->upper_bound_, std::shared_ptr(last_op), node1_symbol, existing_node, existing_edge, match_context.graph_view, @@ -583,7 +582,7 @@ class RuleBasedPlanner { } } last_op = new Expand(node_symbol, edge_symbol, expansion.direction, - std::move(edge_types), + expansion.edge->edge_types_, std::shared_ptr(last_op), node1_symbol, existing_node, existing_edge, match_context.graph_view); diff --git a/src/storage/edges.hpp b/src/storage/edges.hpp index fd58c5127..7d969d4cf 100644 --- a/src/storage/edges.hpp +++ b/src/storage/edges.hpp @@ -44,25 +44,18 @@ class Edges { * * @param iterator - Iterator in the underlying storage. * @param end - End iterator in the underlying storage. - * @param vertex - The destination vertex vlist pointer. - */ - Iterator(std::vector::const_iterator position, - std::vector::const_iterator end, vertex_ptr_t vertex) - : position_(position), end_(end), vertex_(vertex) { - update_position(); - } - - /** Ctor used for creating the beginning iterator with known edge types. - * - * @param iterator - Iterator in the underlying storage. - * @param end - End iterator in the underlying storage. + * @param vertex - The destination vertex vlist pointer. If nullptr the + * edges are not filtered on destination. * @param edge_types - The edge types at least one of which must be matched. - * If nullptr all edges are valid. + * If nullptr edges are not filtered on type. */ Iterator(std::vector::const_iterator position, - std::vector::const_iterator end, + std::vector::const_iterator end, vertex_ptr_t vertex, const std::vector *edge_types) - : position_(position), end_(end), edge_types_(edge_types) { + : position_(position), + end_(end), + vertex_(vertex), + edge_types_(edge_types) { update_position(); } @@ -85,7 +78,8 @@ class Edges { // end_ is used only in update_position() to limit find. std::vector::const_iterator end_; - // Optional predicates. If set they define which edges are skipped by the + // Optional predicates. If set they define which edges are skipped by + // the // iterator. Only one can be not-null in the current implementation. vertex_ptr_t vertex_{nullptr}; // For edge types we use a vector pointer because it's optional. @@ -94,14 +88,16 @@ class Edges { /** Helper function that skips edges that don't satisfy the predicate * present in this iterator. */ void update_position() { - if (vertex_) + if (vertex_) { position_ = std::find_if( position_, end_, [v = this->vertex_](const Element &e) { return e.vertex == v; }); - else if (edge_types_) + } + if (edge_types_) { position_ = std::find_if(position_, end_, [this](const Element &e) { return utils::Contains(*edge_types_, e.edge_type); }); + } } }; @@ -109,8 +105,8 @@ class Edges { /** * Adds an edge to this structure. * - * @param vertex - The destination vertex of the edge. That's the one opposite - * from the vertex that contains this `Edges` instance. + * @param vertex - The destination vertex of the edge. That's the one + * opposite from the vertex that contains this `Edges` instance. * @param edge - The edge. * @param edge_type - Type of the edge. */ @@ -137,24 +133,18 @@ class Edges { auto end() const { return Iterator(storage_.end()); } /** - * Creates a beginning iterator that will skip edges whose destination vertex - * is not equal to the given vertex. + * Creates a beginning iterator that will skip edges whose destination + * vertex is not equal to the given vertex. + * + * @param vertex - The destination vertex vlist pointer. If nullptr the + * edges are not filtered on destination. + * @param edge_types - The edge types at least one of which must be matched. + * If nullptr edges are not filtered on type. */ - auto begin(vertex_ptr_t vertex) const { - return Iterator(storage_.begin(), storage_.end(), vertex); - } - - /* - * Creates a beginning iterator that will skip edges whose edge type is not - * among the given. If none are given, or the pointer is null, then all edges - * are valid. Relies on the fact that edge types are immutable during the - * whole edge lifetime. - */ - auto begin(const std::vector *edge_types) const { - if (edge_types && !edge_types->empty()) - return Iterator(storage_.begin(), storage_.end(), edge_types); - else - return Iterator(storage_.begin()); + auto begin(vertex_ptr_t vertex, + const std::vector *edge_types) const { + if (edge_types && edge_types->empty()) edge_types = nullptr; + return Iterator(storage_.begin(), storage_.end(), vertex, edge_types); } private: diff --git a/src/storage/vertex_accessor.hpp b/src/storage/vertex_accessor.hpp index 1a032b7e1..c526dfaf4 100644 --- a/src/storage/vertex_accessor.hpp +++ b/src/storage/vertex_accessor.hpp @@ -73,23 +73,30 @@ class VertexAccessor : public RecordAccessor { } /** - * Returns EdgeAccessors for all incoming edges whose destination is the given - * vertex. + * Returns EdgeAccessors for all incoming edges. + * + * @param dest - The destination vertex filter. + * @param edge_types - Edge types filter. At least one be matched. If nullptr + * or empty, the parameter is ignored. */ - auto in_with_destination(const VertexAccessor &dest) const { + auto in( + const VertexAccessor &dest, + const std::vector *edge_types = nullptr) const { return MakeAccessorIterator( - current().in_.begin(dest.vlist_), current().in_.end(), db_accessor()); + current().in_.begin(dest.vlist_, edge_types), current().in_.end(), + db_accessor()); } /** - * Returns EdgeAccessors for all incoming edges whose type is one of the - * given. If the given collection of types is nullptr or empty, all edge types - * are valid. + * Returns EdgeAccessors for all incoming edges. + * + * @param edge_types - Edge types filter. At least one be matched. If nullptr + * or empty, the parameter is ignored. */ - auto in_with_types( - const std::vector *edge_types) const { + auto in(const std::vector *edge_types) const { return MakeAccessorIterator( - current().in_.begin(edge_types), current().in_.end(), db_accessor()); + current().in_.begin(nullptr, edge_types), current().in_.end(), + db_accessor()); } /** @@ -103,21 +110,29 @@ class VertexAccessor : public RecordAccessor { /** * Returns EdgeAccessors for all outgoing edges whose destination is the given * vertex. + * + * @param dest - The destination vertex filter. + * @param edge_types - Edge types filter. At least one be matched. If nullptr + * or empty, the parameter is ignored. */ - auto out_with_destination(const VertexAccessor &dest) const { + auto out( + const VertexAccessor &dest, + const std::vector *edge_types = nullptr) const { return MakeAccessorIterator( - current().out_.begin(dest.vlist_), current().out_.end(), db_accessor()); + current().out_.begin(dest.vlist_, edge_types), current().out_.end(), + db_accessor()); } /** - * Returns EdgeAccessors for all outgoing edges whose type is one of the - * given. If the given collection of types is nullptr or empty, all edge types - * are valid. + * Returns EdgeAccessors for all outgoing edges. + * + * @param edge_types - Edge types filter. At least one be matched. If nullptr + * or empty, the parameter is ignored. */ - auto out_with_types( - const std::vector *edge_types) const { + auto out(const std::vector *edge_types) const { return MakeAccessorIterator( - current().out_.begin(edge_types), current().out_.end(), db_accessor()); + current().out_.begin(nullptr, edge_types), current().out_.end(), + db_accessor()); } }; diff --git a/tests/qa/tck_engine/tests/memgraph_V1/features/memgraph_bfs.feature b/tests/qa/tck_engine/tests/memgraph_V1/features/memgraph_bfs.feature index 10cf6fc5a..b3162343d 100644 --- a/tests/qa/tck_engine/tests/memgraph_V1/features/memgraph_bfs.feature +++ b/tests/qa/tck_engine/tests/memgraph_V1/features/memgraph_bfs.feature @@ -8,7 +8,7 @@ Feature: Bfs """ When executing query: """ - MATCH (n {a:'0'})-bfs(e, m| true, 1)->(m) RETURN n.a, m.a + MATCH (n {a:'0'})-[*bfs..1]->(m) RETURN n.a, m.a """ Then the result should be: | n.a | m.a | @@ -23,7 +23,7 @@ Feature: Bfs """ When executing query: """ - MATCH (n {a:'0'})-bfs(e, m| m.a = '1.1' OR m.a = '0', 10)->(m) RETURN n.a, m.a + MATCH (n {a:'0'})-[*bfs..10 (e, m| m.a = '1.1' OR m.a = '0')]->(m) RETURN n.a, m.a """ Then the result should be: | n.a | m.a | @@ -37,7 +37,7 @@ Feature: Bfs """ When executing query: """ - MATCH (:Start)-bfs[r](e, m| true, 10)->(m) WHERE size(r) > 3 + MATCH (:Start)-[r *bfs..10]->(m) WHERE size(r) > 3 RETURN size(r) AS s, (r[0]).id AS r0, (r[2]).id AS r2 """ Then the result should be: @@ -52,7 +52,7 @@ Feature: Bfs """ When executing query: """ - MATCH ()-bfs[r :r0](e, m| true, 10)->(m) + MATCH ()-[r:r0 *bfs..10]->(m) RETURN size(r) AS s, (r[0]).id AS r0 """ Then the result should be: @@ -67,7 +67,7 @@ Feature: Bfs """ When executing query: """ - MATCH ()-bfs[r :r0|:r1](e, m| true, 10)->(m) WHERE size(r) > 1 + MATCH ()-[r :r0|:r1 *bfs..10 ]->(m) WHERE size(r) > 1 RETURN size(r) AS s, (r[0]).id AS r0, (r[1]).id AS r1 """ Then the result should be: diff --git a/tests/unit/cypher_main_visitor.cpp b/tests/unit/cypher_main_visitor.cpp index f35da20a2..cbc0c0cdb 100644 --- a/tests/unit/cypher_main_visitor.cpp +++ b/tests/unit/cypher_main_visitor.cpp @@ -1378,7 +1378,7 @@ TYPED_TEST(CypherMainVisitorTest, ReturnAll) { TYPED_TEST(CypherMainVisitorTest, MatchBfsReturn) { TypeParam ast_generator( - "MATCH (n) -bfs[r:type1|type2](e, n|e.prop = 42, 10)-> (m) RETURN r"); + "MATCH (n) -[r:type1|type2 *bfs..10 (e, n|e.prop = 42)]-> (m) RETURN r"); auto *query = ast_generator.query_; ASSERT_EQ(query->clauses_.size(), 2U); auto *match = dynamic_cast(query->clauses_[0]); @@ -1387,6 +1387,7 @@ TYPED_TEST(CypherMainVisitorTest, MatchBfsReturn) { ASSERT_EQ(match->patterns_[0]->atoms_.size(), 3U); auto *bfs = dynamic_cast(match->patterns_[0]->atoms_[1]); ASSERT_TRUE(bfs); + EXPECT_TRUE(bfs->has_range_); EXPECT_EQ(bfs->direction_, EdgeAtom::Direction::OUT); EXPECT_THAT( bfs->edge_types_, @@ -1395,7 +1396,7 @@ TYPED_TEST(CypherMainVisitorTest, MatchBfsReturn) { EXPECT_EQ(bfs->identifier_->name_, "r"); EXPECT_EQ(bfs->traversed_edge_identifier_->name_, "e"); EXPECT_EQ(bfs->next_node_identifier_->name_, "n"); - CheckLiteral(ast_generator.context_, bfs->max_depth_, 10); + CheckLiteral(ast_generator.context_, bfs->upper_bound_, 10); auto *eq = dynamic_cast(bfs->filter_expression_); ASSERT_TRUE(eq); } diff --git a/tests/unit/interpreter.cpp b/tests/unit/interpreter.cpp index 1f3a051d6..c959a188d 100644 --- a/tests/unit/interpreter.cpp +++ b/tests/unit/interpreter.cpp @@ -222,7 +222,7 @@ TEST(Interpreter, Bfs) { auto dba = dbms.active(); interpreter.Interpret( - "MATCH (n {id: 0})-bfs[r](e, n | n.reachable and e.reachable, 5)->(m) " + "MATCH (n {id: 0})-[r *bfs..5 (e, n | n.reachable and e.reachable)]->(m) " "RETURN r", *dba, stream, {}); diff --git a/tests/unit/query_expression_evaluator.cpp b/tests/unit/query_expression_evaluator.cpp index 3f2a6a5fa..118e88d16 100644 --- a/tests/unit/query_expression_evaluator.cpp +++ b/tests/unit/query_expression_evaluator.cpp @@ -662,43 +662,6 @@ TEST(ExpressionEvaluator, LabelsTest) { } } -TEST(ExpressionEvaluator, EdgeTypeTest) { - AstTreeStorage storage; - NoContextExpressionEvaluator eval; - Dbms dbms; - auto dba = dbms.active(); - auto v1 = dba->InsertVertex(); - auto v2 = dba->InsertVertex(); - auto e = dba->InsertEdge(v1, v2, dba->EdgeType("TYPE1")); - auto *identifier = storage.Create("e"); - auto edge_symbol = eval.symbol_table.CreateSymbol("e", true); - eval.symbol_table[*identifier] = edge_symbol; - eval.frame[edge_symbol] = e; - { - auto *op = storage.Create( - identifier, std::vector{ - dba->EdgeType("TYPE0"), dba->EdgeType("TYPE1"), - dba->EdgeType("TYPE2")}); - auto value = op->Accept(eval.eval); - EXPECT_EQ(value.Value(), true); - } - { - auto *op = storage.Create( - identifier, std::vector{ - dba->EdgeType("TYPE0"), dba->EdgeType("TYPE2")}); - auto value = op->Accept(eval.eval); - EXPECT_EQ(value.Value(), false); - } - { - eval.frame[edge_symbol] = TypedValue::Null; - auto *op = storage.Create( - identifier, std::vector{ - dba->EdgeType("TYPE0"), dba->EdgeType("TYPE2")}); - auto value = op->Accept(eval.eval); - EXPECT_TRUE(value.IsNull()); - } -} - TEST(ExpressionEvaluator, Aggregation) { AstTreeStorage storage; auto aggr = storage.Create(storage.Create(42), diff --git a/tests/unit/query_plan_edge_cases.cpp b/tests/unit/query_plan_edge_cases.cpp index 97a202c65..3ac6e0d6d 100644 --- a/tests/unit/query_plan_edge_cases.cpp +++ b/tests/unit/query_plan_edge_cases.cpp @@ -55,7 +55,7 @@ TEST_F(QueryExecution, MissingOptionalIntoExpand) { std::string expand = "-->"; std::string variable = "-[*1]->"; - std::string bfs = "-bfs[](n, e | true, 1)->"; + std::string bfs = "-[*bfs..1]->"; EXPECT_EQ(Exec(false, expand), 1); EXPECT_EQ(Exec(true, expand), 1); diff --git a/tests/unit/query_plan_match_filter_return.cpp b/tests/unit/query_plan_match_filter_return.cpp index 34883a5e3..58a75c968 100644 --- a/tests/unit/query_plan_match_filter_return.cpp +++ b/tests/unit/query_plan_match_filter_return.cpp @@ -1357,18 +1357,13 @@ TEST(QueryPlan, EdgeFilterMultipleTypes) { // make a scan all auto n = MakeScanAll(storage, symbol_table, "n"); - auto r_m = MakeExpand(storage, symbol_table, n.op_, n.sym_, "r", - EdgeAtom::Direction::OUT, {}, false, "m", false); - // add an edge type filter - r_m.edge_->edge_types_.push_back(type_1); - r_m.edge_->edge_types_.push_back(type_2); - auto *filter_expr = storage.Create(r_m.edge_->identifier_, - r_m.edge_->edge_types_); - auto edge_filter = std::make_shared(r_m.op_, filter_expr); + auto r_m = + MakeExpand(storage, symbol_table, n.op_, n.sym_, "r", + EdgeAtom::Direction::OUT, {type_1, type_2}, false, "m", false); // make a named expression and a produce auto output = NEXPR("m", IDENT("m")); - auto produce = MakeProduce(edge_filter, output); + auto produce = MakeProduce(r_m.op_, output); // fill up the symbol table symbol_table[*output] = symbol_table.CreateSymbol("named_expression_1", true); diff --git a/tests/unit/query_planner.cpp b/tests/unit/query_planner.cpp index 9b33d08d3..d9fc183af 100644 --- a/tests/unit/query_planner.cpp +++ b/tests/unit/query_planner.cpp @@ -408,7 +408,7 @@ TEST(TestLogicalPlanner, MatchPathReturn) { auto relationship = dba->EdgeType("relationship"); QUERY(MATCH(PATTERN(NODE("n"), EDGE("r", relationship), NODE("m"))), RETURN("n")); - CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectFilter(), + CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectProduce()); } @@ -421,7 +421,7 @@ TEST(TestLogicalPlanner, MatchNamedPatternReturn) { QUERY( MATCH(NAMED_PATTERN("p", NODE("n"), EDGE("r", relationship), NODE("m"))), RETURN("n")); - CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectFilter(), + CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectConstructNamedPath(), ExpectProduce()); } @@ -434,7 +434,7 @@ TEST(TestLogicalPlanner, MatchNamedPatternWithPredicateReturn) { QUERY( MATCH(NAMED_PATTERN("p", NODE("n"), EDGE("r", relationship), NODE("m"))), WHERE(EQ(LITERAL(2), IDENT("p"))), RETURN("n")); - CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectFilter(), + CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectConstructNamedPath(), ExpectFilter(), ExpectProduce()); } @@ -541,28 +541,6 @@ TEST(TestLogicalPlanner, MultiMatchSameStart) { CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectProduce()); } -TEST(TestLogicalPlanner, MatchExistingEdge) { - // Test MATCH (n) -[r]- (m) -[r]- (j) RETURN n - AstTreeStorage storage; - QUERY(MATCH(PATTERN(NODE("n"), EDGE("r"), NODE("m"), EDGE("r"), NODE("j"))), - RETURN("n")); - // There is no ExpandUniquenessFilter for referencing the same edge. - CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectExpand(), - ExpectProduce()); -} - -TEST(TestLogicalPlanner, MultiMatchExistingEdgeOtherEdge) { - // Test MATCH (n) -[r]- (m) MATCH (m) -[r]- (j) -[e]- (l) RETURN n - AstTreeStorage storage; - QUERY(MATCH(PATTERN(NODE("n"), EDGE("r"), NODE("m"))), - MATCH(PATTERN(NODE("m"), EDGE("r"), NODE("j"), EDGE("e"), NODE("l"))), - RETURN("n")); - // We need ExpandUniquenessFilter for edge `e` against `r` in second MATCH. - CheckPlan(storage, ExpectScanAll(), ExpectExpand(), ExpectExpand(), - ExpectExpand(), ExpectExpandUniquenessFilter(), - ExpectProduce()); -} - TEST(TestLogicalPlanner, MatchWithReturn) { // Test MATCH (old) WITH old AS new RETURN new AstTreeStorage storage; @@ -772,7 +750,7 @@ TEST(TestLogicalPlanner, MatchMerge) { ON_MATCH(SET(PROPERTY_LOOKUP("n", prop), LITERAL(42))), ON_CREATE(SET("m", IDENT("n")))), RETURN(ident_n, AS("n"))); - std::list on_match{new ExpectExpand(), new ExpectFilter(), + std::list on_match{new ExpectExpand(), new ExpectSetProperty()}; std::list on_create{new ExpectCreateExpand(), new ExpectSetProperties()}; @@ -1345,7 +1323,8 @@ TEST(TestLogicalPlanner, MatchBreadthFirst) { auto *bfs = storage.Create( IDENT("r"), Direction::OUT, std::vector{edge_type}, IDENT("r"), IDENT("n"), - IDENT("n"), LITERAL(10)); + IDENT("n")); + bfs->upper_bound_ = LITERAL(10); QUERY(MATCH(PATTERN(NODE("n"), bfs, NODE("m"))), RETURN("r")); CheckPlan(storage, ExpectScanAll(), ExpectExpandBreadthFirst(), ExpectProduce()); diff --git a/tests/unit/query_semantic.cpp b/tests/unit/query_semantic.cpp index 566753119..5d457c2eb 100644 --- a/tests/unit/query_semantic.cpp +++ b/tests/unit/query_semantic.cpp @@ -45,8 +45,8 @@ TEST(TestSymbolGenerator, MatchNamedPattern) { SymbolTable symbol_table; AstTreeStorage storage; // MATCH p = (node_atom_1) RETURN node_atom_1 - auto query_ast = QUERY(MATCH(NAMED_PATTERN("p", NODE("node_atom_1"))), - RETURN("p")); + auto query_ast = + QUERY(MATCH(NAMED_PATTERN("p", NODE("node_atom_1"))), RETURN("p")); SymbolGenerator symbol_generator(symbol_table); query_ast->Accept(symbol_generator); // symbols for p, node_atom_1 and named_expr in return @@ -80,49 +80,6 @@ TEST(TestSymbolGenerator, MatchNodeUnboundReturn) { EXPECT_THROW(query_ast->Accept(symbol_generator), UnboundVariableError); } -TEST(TestSymbolGenerator, MatchSameEdge) { - SymbolTable symbol_table; - AstTreeStorage storage; - // AST with match pattern referencing an edge multiple times: - // MATCH (n) -[r]- (n) -[r]- (n) RETURN r - // This usually throws a redeclaration error, but we support it. - auto query_ast = QUERY( - MATCH(PATTERN(NODE("n"), EDGE("r"), NODE("n"), EDGE("r"), NODE("n"))), - RETURN("r")); - SymbolGenerator symbol_generator(symbol_table); - query_ast->Accept(symbol_generator); - // symbols for pattern, `n`, `r` and named_expr in return - EXPECT_EQ(symbol_table.max_position(), 4); - auto match = dynamic_cast(query_ast->clauses_[0]); - auto pattern = match->patterns_[0]; - std::vector node_symbols; - std::vector edge_symbols; - bool is_node{true}; - for (auto &atom : pattern->atoms_) { - auto symbol = symbol_table[*atom->identifier_]; - if (is_node) { - node_symbols.emplace_back(symbol); - } else { - edge_symbols.emplace_back(symbol); - } - is_node = !is_node; - } - auto &node_symbol = node_symbols.front(); - EXPECT_EQ(node_symbol.type(), Symbol::Type::Vertex); - for (auto &symbol : node_symbols) { - EXPECT_EQ(node_symbol, symbol); - } - auto &edge_symbol = edge_symbols.front(); - EXPECT_EQ(edge_symbol.type(), Symbol::Type::Edge); - for (auto &symbol : edge_symbols) { - EXPECT_EQ(edge_symbol, symbol); - } - auto ret = dynamic_cast(query_ast->clauses_[1]); - auto named_expr = ret->body_.named_expressions[0]; - auto ret_symbol = symbol_table[*named_expr->expression_]; - EXPECT_EQ(edge_symbol, ret_symbol); -} - TEST(TestSymbolGenerator, CreatePropertyUnbound) { SymbolTable symbol_table; AstTreeStorage storage; @@ -1048,10 +1005,10 @@ TEST(TestSymbolGenerator, MatchBfsReturn) { auto *node_n = NODE("n"); auto *r_prop = PROPERTY_LOOKUP("r", prop); auto *n_prop = PROPERTY_LOOKUP("n", prop); - auto *bfs = - storage.Create(IDENT("r"), EdgeAtom::Direction::OUT, - std::vector{}, - IDENT("r"), IDENT("n"), r_prop, n_prop); + auto *bfs = storage.Create( + IDENT("r"), EdgeAtom::Direction::OUT, + std::vector{}, IDENT("r"), IDENT("n"), r_prop); + bfs->upper_bound_ = n_prop; auto *ret_r = IDENT("r"); auto *query = QUERY(MATCH(PATTERN(node_n, bfs, NODE("m"))), RETURN(ret_r, AS("r"))); @@ -1074,10 +1031,11 @@ TEST(TestSymbolGenerator, MatchBfsReturn) { TEST(TestSymbolGenerator, MatchBfsUsesEdgeSymbolError) { // Test MATCH (n) -bfs[r](e, n | r, 10)-> (m) RETURN r AstTreeStorage storage; - auto *bfs = storage.Create( - IDENT("r"), EdgeAtom::Direction::OUT, - std::vector{}, IDENT("e"), IDENT("n"), IDENT("r"), - LITERAL(10)); + auto *bfs = + storage.Create(IDENT("r"), EdgeAtom::Direction::OUT, + std::vector{}, + IDENT("e"), IDENT("n"), IDENT("r")); + bfs->upper_bound_ = LITERAL(10); auto *query = QUERY(MATCH(PATTERN(NODE("n"), bfs, NODE("m"))), RETURN("r")); SymbolTable symbol_table; SymbolGenerator symbol_generator(symbol_table); @@ -1088,10 +1046,11 @@ TEST(TestSymbolGenerator, MatchBfsUsesPreviousOuterSymbol) { // Test MATCH (a) -bfs[r](e, n | a, 10)-> (m) RETURN r AstTreeStorage storage; auto *node_a = NODE("a"); - auto *bfs = storage.Create( - IDENT("r"), EdgeAtom::Direction::OUT, - std::vector{}, IDENT("e"), IDENT("n"), IDENT("a"), - LITERAL(10)); + auto *bfs = + storage.Create(IDENT("r"), EdgeAtom::Direction::OUT, + std::vector{}, + IDENT("e"), IDENT("n"), IDENT("a")); + bfs->upper_bound_ = LITERAL(10); auto *query = QUERY(MATCH(PATTERN(node_a, bfs, NODE("m"))), RETURN("r")); SymbolTable symbol_table; SymbolGenerator symbol_generator(symbol_table); @@ -1103,10 +1062,11 @@ TEST(TestSymbolGenerator, MatchBfsUsesPreviousOuterSymbol) { TEST(TestSymbolGenerator, MatchBfsUsesLaterSymbolError) { // Test MATCH (n) -bfs[r](e, n | m, 10)-> (m) RETURN r AstTreeStorage storage; - auto *bfs = storage.Create( - IDENT("r"), EdgeAtom::Direction::OUT, - std::vector{}, IDENT("e"), IDENT("n"), IDENT("m"), - LITERAL(10)); + auto *bfs = + storage.Create(IDENT("r"), EdgeAtom::Direction::OUT, + std::vector{}, + IDENT("e"), IDENT("n"), IDENT("m")); + bfs->upper_bound_ = LITERAL(10); auto *query = QUERY(MATCH(PATTERN(NODE("n"), bfs, NODE("m"))), RETURN("r")); SymbolTable symbol_table; SymbolGenerator symbol_generator(symbol_table); diff --git a/tests/unit/query_variable_start_planner.cpp b/tests/unit/query_variable_start_planner.cpp index ecbadecc0..9c7635088 100644 --- a/tests/unit/query_variable_start_planner.cpp +++ b/tests/unit/query_variable_start_planner.cpp @@ -308,8 +308,8 @@ TEST(TestVariableStartPlanner, MatchBfs) { AstTreeStorage storage; auto *bfs = storage.Create( IDENT("r"), Direction::OUT, std::vector{}, - IDENT("r"), IDENT("n"), NEQ(PROPERTY_LOOKUP("n", id), LITERAL(3)), - LITERAL(10)); + IDENT("r"), IDENT("n"), NEQ(PROPERTY_LOOKUP("n", id), LITERAL(3))); + bfs->upper_bound_ = LITERAL(10); QUERY(MATCH(PATTERN(NODE("n"), bfs, NODE("m"))), RETURN("r")); // We expect to get a single column with the following rows: TypedValue r1_list(std::vector{r1}); // [r1] diff --git a/tests/unit/record_edge_vertex_accessor.cpp b/tests/unit/record_edge_vertex_accessor.cpp index a9e890873..e792c6f59 100644 --- a/tests/unit/record_edge_vertex_accessor.cpp +++ b/tests/unit/record_edge_vertex_accessor.cpp @@ -261,25 +261,25 @@ TEST(RecordAccessor, VertexEdgeConnectionsWithExistingVertex) { auto e32 = dba->InsertEdge(v3, v2, edge_type); dba->AdvanceCommand(); - TEST_EDGE_ITERABLE(v1.out_with_destination(v1)); - TEST_EDGE_ITERABLE(v1.out_with_destination(v2), {e12}); - TEST_EDGE_ITERABLE(v1.out_with_destination(v3)); - TEST_EDGE_ITERABLE(v2.out_with_destination(v1)); - TEST_EDGE_ITERABLE(v2.out_with_destination(v2), {e22}); - TEST_EDGE_ITERABLE(v2.out_with_destination(v3), {e23a, e23b}); - TEST_EDGE_ITERABLE(v3.out_with_destination(v1)); - TEST_EDGE_ITERABLE(v3.out_with_destination(v2), {e32}); - TEST_EDGE_ITERABLE(v3.out_with_destination(v3)); + TEST_EDGE_ITERABLE(v1.out(v1)); + TEST_EDGE_ITERABLE(v1.out(v2), {e12}); + TEST_EDGE_ITERABLE(v1.out(v3)); + TEST_EDGE_ITERABLE(v2.out(v1)); + TEST_EDGE_ITERABLE(v2.out(v2), {e22}); + TEST_EDGE_ITERABLE(v2.out(v3), {e23a, e23b}); + TEST_EDGE_ITERABLE(v3.out(v1)); + TEST_EDGE_ITERABLE(v3.out(v2), {e32}); + TEST_EDGE_ITERABLE(v3.out(v3)); - TEST_EDGE_ITERABLE(v1.in_with_destination(v1)); - TEST_EDGE_ITERABLE(v1.in_with_destination(v2)); - TEST_EDGE_ITERABLE(v1.in_with_destination(v3)); - TEST_EDGE_ITERABLE(v2.in_with_destination(v1), {e12}); - TEST_EDGE_ITERABLE(v2.in_with_destination(v2), {e22}); - TEST_EDGE_ITERABLE(v2.in_with_destination(v3), {e32}); - TEST_EDGE_ITERABLE(v3.in_with_destination(v1)); - TEST_EDGE_ITERABLE(v3.in_with_destination(v2), {e23a, e23b}); - TEST_EDGE_ITERABLE(v3.in_with_destination(v3)); + TEST_EDGE_ITERABLE(v1.in(v1)); + TEST_EDGE_ITERABLE(v1.in(v2)); + TEST_EDGE_ITERABLE(v1.in(v3)); + TEST_EDGE_ITERABLE(v2.in(v1), {e12}); + TEST_EDGE_ITERABLE(v2.in(v2), {e22}); + TEST_EDGE_ITERABLE(v2.in(v3), {e32}); + TEST_EDGE_ITERABLE(v3.in(v1)); + TEST_EDGE_ITERABLE(v3.in(v2), {e23a, e23b}); + TEST_EDGE_ITERABLE(v3.in(v3)); } TEST(RecordAccessor, VertexEdgeConnectionsWithEdgeType) { @@ -302,15 +302,15 @@ TEST(RecordAccessor, VertexEdgeConnectionsWithEdgeType) { std::vector edges_a{a}; std::vector edges_b{b}; std::vector edges_ac{a, c}; - TEST_EDGE_ITERABLE(v1.in_with_types(&edges_a)); - TEST_EDGE_ITERABLE(v1.in_with_types(&edges_b), {eb_1, eb_2}); - TEST_EDGE_ITERABLE(v1.out_with_types(&edges_a), {ea}); - TEST_EDGE_ITERABLE(v1.out_with_types(&edges_b)); - TEST_EDGE_ITERABLE(v1.out_with_types(&edges_ac), {ea, ec}); - TEST_EDGE_ITERABLE(v2.in_with_types(&edges_a), {ea}); - TEST_EDGE_ITERABLE(v2.in_with_types(&edges_b)); - TEST_EDGE_ITERABLE(v2.out_with_types(&edges_a)); - TEST_EDGE_ITERABLE(v2.out_with_types(&edges_b), {eb_1, eb_2}); + TEST_EDGE_ITERABLE(v1.in(&edges_a)); + TEST_EDGE_ITERABLE(v1.in(&edges_b), {eb_1, eb_2}); + TEST_EDGE_ITERABLE(v1.out(&edges_a), {ea}); + TEST_EDGE_ITERABLE(v1.out(&edges_b)); + TEST_EDGE_ITERABLE(v1.out(&edges_ac), {ea, ec}); + TEST_EDGE_ITERABLE(v2.in(&edges_a), {ea}); + TEST_EDGE_ITERABLE(v2.in(&edges_b)); + TEST_EDGE_ITERABLE(v2.out(&edges_a)); + TEST_EDGE_ITERABLE(v2.out(&edges_b), {eb_1, eb_2}); } #undef TEST_EDGE_ITERABLE