From b4d2d1ff81a5166dfc9857628c3b03345671f212 Mon Sep 17 00:00:00 2001 From: Teon Banek Date: Wed, 19 Jul 2017 15:48:43 +0200 Subject: [PATCH] Support variable length path in CypherMainVisitor Summary: Allow expressions for variable length path bounds Replace test which expected a syntax exception Since we now allow variable length to have an arbitrary expression, the test case is obsolete. It was replaced with something that excepts an expression which wasn't allowed before. Reviewers: florijan, mislav.bradac, buda Reviewed By: mislav.bradac Subscribers: lion, pullbot Differential Revision: https://phabricator.memgraph.io/D568 --- src/query/frontend/ast/ast.hpp | 12 ++ .../frontend/ast/cypher_main_visitor.cpp | 51 +++--- .../frontend/opencypher/grammar/Cypher.g4 | 2 +- .../frontend/semantic/symbol_generator.cpp | 5 + src/query/plan/rule_based_planner.cpp | 4 + tests/unit/cypher_main_visitor.cpp | 150 ++++++++++++------ 6 files changed, 149 insertions(+), 75 deletions(-) diff --git a/src/query/frontend/ast/ast.hpp b/src/query/frontend/ast/ast.hpp index 38f168541..8f3ac8c9b 100644 --- a/src/query/frontend/ast/ast.hpp +++ b/src/query/frontend/ast/ast.hpp @@ -860,6 +860,12 @@ class EdgeAtom : public PatternAtom { cont = property.second->Accept(visitor); } } + if (cont && lower_bound_) { + cont = lower_bound_->Accept(visitor); + } + if (cont && upper_bound_) { + cont = upper_bound_->Accept(visitor); + } } return visitor.PostVisit(*this); } @@ -871,6 +877,9 @@ class EdgeAtom : public PatternAtom { for (auto property : properties_) { edge_atom->properties_[property.first] = property.second->Clone(storage); } + edge_atom->has_range_ = has_range_; + edge_atom->lower_bound_ = lower_bound_ ? lower_bound_->Clone(storage) : nullptr; + edge_atom->upper_bound_ = upper_bound_ ? upper_bound_->Clone(storage) : nullptr; return edge_atom; } @@ -878,6 +887,9 @@ class EdgeAtom : public PatternAtom { std::vector edge_types_; // TODO: change to unordered_map std::map properties_; + bool has_range_ = false; + Expression *lower_bound_ = nullptr; + Expression *upper_bound_ = nullptr; protected: using PatternAtom::PatternAtom; diff --git a/src/query/frontend/ast/cypher_main_visitor.cpp b/src/query/frontend/ast/cypher_main_visitor.cpp index a1b53863e..0937a1b79 100644 --- a/src/query/frontend/ast/cypher_main_visitor.cpp +++ b/src/query/frontend/ast/cypher_main_visitor.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -434,17 +435,15 @@ antlrcpp::Any CypherMainVisitor::visitRelationshipPattern( .as>(); } if (ctx->relationshipDetail()->rangeLiteral()) { - // TODO: implement other clauses. - throw utils::NotYetImplemented("variable relationship length"); + edge->has_range_ = true; + auto range = ctx->relationshipDetail() + ->rangeLiteral() + ->accept(this) + .as>(); + edge->lower_bound_ = range.first; + edge->upper_bound_ = range.second; } } - // relationship.has_range = true; - // auto range = ctx->relationshipDetail() - // ->rangeLiteral() - // ->accept(this) - // .as>(); - // relationship.lower_bound = range.first; - // relationship.upper_bound = range.second; if (!edge->identifier_) { anonymous_identifiers.push_back(&edge->identifier_); } @@ -478,29 +477,31 @@ antlrcpp::Any CypherMainVisitor::visitRelationshipTypes( antlrcpp::Any CypherMainVisitor::visitRangeLiteral( CypherParser::RangeLiteralContext *ctx) { - if (ctx->integerLiteral().size() == 0U) { - // -[*]- - return std::pair(1LL, LLONG_MAX); - } else if (ctx->integerLiteral().size() == 1U) { + debug_assert(ctx->expression().size() <= 2U, + "Expected 0, 1 or 2 bounds in range literal."); + if (ctx->expression().size() == 0U) { + // Case -[*]- + return std::pair(nullptr, nullptr); + } else if (ctx->expression().size() == 1U) { auto dots_tokens = ctx->getTokens(kDotsTokenId); - int64_t bound = ctx->integerLiteral()[0]->accept(this).as(); + Expression *bound = ctx->expression()[0]->accept(this); if (!dots_tokens.size()) { - // -[*2]- - return std::pair(bound, bound); + // Case -[*bound]- + return std::pair(bound, bound); } if (dots_tokens[0]->getSourceInterval().startsAfter( - ctx->integerLiteral()[0]->getSourceInterval())) { - // -[*2..]- - return std::pair(bound, LLONG_MAX); + ctx->expression()[0]->getSourceInterval())) { + // Case -[*bound..]- + return std::pair(bound, nullptr); } else { - // -[*..2]- - return std::pair(1LL, bound); + // Case -[*..bound]- + return std::pair(nullptr, bound); } } else { - int64_t lbound = ctx->integerLiteral()[0]->accept(this).as(); - int64_t rbound = ctx->integerLiteral()[1]->accept(this).as(); - // -[*2..5]- - return std::pair(lbound, rbound); + // Case -[*lbound..rbound]- + Expression *lbound = ctx->expression()[0]->accept(this); + Expression *rbound = ctx->expression()[1]->accept(this); + return std::pair(lbound, rbound); } } diff --git a/src/query/frontend/opencypher/grammar/Cypher.g4 b/src/query/frontend/opencypher/grammar/Cypher.g4 index 615959554..6d7dd18e6 100644 --- a/src/query/frontend/opencypher/grammar/Cypher.g4 +++ b/src/query/frontend/opencypher/grammar/Cypher.g4 @@ -128,7 +128,7 @@ nodeLabels : nodeLabel ( SP? nodeLabel )* ; nodeLabel : ':' SP? labelName ; -rangeLiteral : '*' SP? ( integerLiteral SP? )? ( '..' SP? ( integerLiteral SP? )? )? ; +rangeLiteral : '*' SP? ( expression SP? )? ( '..' SP? ( expression SP? )? )? ; labelName : symbolicName ; diff --git a/src/query/frontend/semantic/symbol_generator.cpp b/src/query/frontend/semantic/symbol_generator.cpp index 3d9cc31de..c561e1f48 100644 --- a/src/query/frontend/semantic/symbol_generator.cpp +++ b/src/query/frontend/semantic/symbol_generator.cpp @@ -307,6 +307,11 @@ bool SymbolGenerator::PreVisit(EdgeAtom &edge_atom) { "Bidirectional relationship are not supported " "when creating an edge"); } + if (edge_atom.has_range_) { + throw SemanticException( + "Variable length relationships are not supported when creating an " + "edge."); + } } scope_.in_property_map = true; for (auto kv : edge_atom.properties_) { diff --git a/src/query/plan/rule_based_planner.cpp b/src/query/plan/rule_based_planner.cpp index f79f82e8c..20bfeb902 100644 --- a/src/query/plan/rule_based_planner.cpp +++ b/src/query/plan/rule_based_planner.cpp @@ -792,6 +792,10 @@ LogicalOperator *PlanMatching(const Matching &matching, } // We have an edge, so generate Expand. if (expansion.edge) { + if (expansion.edge->has_range_) { + throw utils::NotYetImplemented( + "planning variable length relationships"); + } // If the expand symbols were already bound, then we need to indicate // that they exist. The Expand will then check whether the pattern holds // instead of writing the expansion to symbols. diff --git a/tests/unit/cypher_main_visitor.cpp b/tests/unit/cypher_main_visitor.cpp index 7e9d9a5f1..950890189 100644 --- a/tests/unit/cypher_main_visitor.cpp +++ b/tests/unit/cypher_main_visitor.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -120,7 +121,7 @@ typedef ::testing::Typesidentifier_->user_declared_); } -// // Relationship with unbounded variable range. -// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUnbounded) { -// ParserTables parser("CREATE ()-[*]-()"); -// ASSERT_EQ(parser.identifiers_map_.size(), 0U); -// ASSERT_EQ(parser.relationships_.size(), 1U); -// CompareRelationships(*parser.relationships_.begin(), -// Relationship::Direction::BOTH, {}, {}, true, 1, -// LLONG_MAX); -// } -// -// // Relationship with lower bounded variable range. -// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerBounded) { -// ParserTables parser("CREATE ()-[*5..]-()"); -// ASSERT_EQ(parser.identifiers_map_.size(), 0U); -// ASSERT_EQ(parser.relationships_.size(), 1U); -// CompareRelationships(*parser.relationships_.begin(), -// Relationship::Direction::BOTH, {}, {}, true, 5, -// LLONG_MAX); -// } -// -// // Relationship with upper bounded variable range. -// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUpperBounded) { -// ParserTables parser("CREATE ()-[*..10]-()"); -// ASSERT_EQ(parser.identifiers_map_.size(), 0U); -// ASSERT_EQ(parser.relationships_.size(), 1U); -// CompareRelationships(*parser.relationships_.begin(), -// Relationship::Direction::BOTH, {}, {}, true, 1, 10); -// } -// -// // Relationship with lower and upper bounded variable range. -// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerUpperBounded) { -// ParserTables parser("CREATE ()-[*5..10]-()"); -// ASSERT_EQ(parser.identifiers_map_.size(), 0U); -// ASSERT_EQ(parser.relationships_.size(), 1U); -// CompareRelationships(*parser.relationships_.begin(), -// Relationship::Direction::BOTH, {}, {}, true, 5, 10); -// } -// -// // Relationship with fixed number of edges. -// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternFixedRange) { -// ParserTables parser("CREATE ()-[*10]-()"); -// ASSERT_EQ(parser.identifiers_map_.size(), 0U); -// ASSERT_EQ(parser.relationships_.size(), 1U); -// CompareRelationships(*parser.relationships_.begin(), -// Relationship::Direction::BOTH, {}, {}, true, 10, 10); -// } -// -// +// Assert that match has a single pattern with a single edge atom and store it +// in edge parameter. +void AssertMatchSingleEdgeAtom(Match *match, EdgeAtom *&edge) { + ASSERT_TRUE(match); + ASSERT_EQ(match->patterns_.size(), 1U); + ASSERT_EQ(match->patterns_[0]->atoms_.size(), 3U); + edge = dynamic_cast(match->patterns_[0]->atoms_[1]); + ASSERT_TRUE(edge); +} + +TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUnbounded) { + TypeParam ast_generator("MATCH ()-[r*]->() RETURN r"); + auto *query = ast_generator.query_; + auto *match = dynamic_cast(query->clauses_[0]); + EdgeAtom *edge = nullptr; + AssertMatchSingleEdgeAtom(match, edge); + EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT); + EXPECT_TRUE(edge->has_range_); + EXPECT_EQ(edge->lower_bound_, nullptr); + EXPECT_EQ(edge->upper_bound_, nullptr); +} + +TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerBounded) { + TypeParam ast_generator("MATCH ()-[r*42..]->() RETURN r"); + auto *query = ast_generator.query_; + auto *match = dynamic_cast(query->clauses_[0]); + EdgeAtom *edge = nullptr; + AssertMatchSingleEdgeAtom(match, edge); + EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT); + EXPECT_TRUE(edge->has_range_); + auto *lower_bound = dynamic_cast(edge->lower_bound_); + ASSERT_TRUE(lower_bound); + EXPECT_TRUE(lower_bound->value_.Value() == 42); + EXPECT_EQ(edge->upper_bound_, nullptr); +} + +TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUpperBounded) { + TypeParam ast_generator("MATCH ()-[r*..42]->() RETURN r"); + auto *query = ast_generator.query_; + auto *match = dynamic_cast(query->clauses_[0]); + EdgeAtom *edge = nullptr; + AssertMatchSingleEdgeAtom(match, edge); + EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT); + EXPECT_TRUE(edge->has_range_); + EXPECT_EQ(edge->lower_bound_, nullptr); + auto upper_bound = dynamic_cast(edge->upper_bound_); + ASSERT_TRUE(upper_bound); + EXPECT_EQ(upper_bound->value_.Value(), 42); +} + +TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerUpperBounded) { + TypeParam ast_generator("MATCH ()-[r*24..42]->() RETURN r"); + auto *query = ast_generator.query_; + auto *match = dynamic_cast(query->clauses_[0]); + EdgeAtom *edge = nullptr; + AssertMatchSingleEdgeAtom(match, edge); + EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT); + EXPECT_TRUE(edge->has_range_); + auto lower_bound = dynamic_cast(edge->lower_bound_); + ASSERT_TRUE(lower_bound); + EXPECT_EQ(lower_bound->value_.Value(), 24); + auto upper_bound = dynamic_cast(edge->upper_bound_); + ASSERT_TRUE(upper_bound); + EXPECT_EQ(upper_bound->value_.Value(), 42); +} + +TYPED_TEST(CypherMainVisitorTest, RelationshipPatternFixedRange) { + TypeParam ast_generator("MATCH ()-[r*42]->() RETURN r"); + auto *query = ast_generator.query_; + auto *match = dynamic_cast(query->clauses_[0]); + EdgeAtom *edge = nullptr; + AssertMatchSingleEdgeAtom(match, edge); + EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT); + EXPECT_TRUE(edge->has_range_); + auto lower_bound = dynamic_cast(edge->lower_bound_); + ASSERT_TRUE(lower_bound); + EXPECT_EQ(lower_bound->value_.Value(), 42); + auto upper_bound = dynamic_cast(edge->upper_bound_); + ASSERT_TRUE(upper_bound); + EXPECT_EQ(upper_bound->value_.Value(), 42); +} + +TYPED_TEST(CypherMainVisitorTest, RelationshipPatternFloatingUpperBound) { + // [r*1...2] should be parsed as [r*1..0.2] + TypeParam ast_generator("MATCH ()-[r*1...2]->() RETURN r"); + auto *query = ast_generator.query_; + auto *match = dynamic_cast(query->clauses_[0]); + EdgeAtom *edge = nullptr; + AssertMatchSingleEdgeAtom(match, edge); + EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT); + EXPECT_TRUE(edge->has_range_); + auto lower_bound = dynamic_cast(edge->lower_bound_); + ASSERT_TRUE(lower_bound); + EXPECT_EQ(lower_bound->value_.Value(), 1); + auto upper_bound = dynamic_cast(edge->upper_bound_); + ASSERT_TRUE(upper_bound); + EXPECT_EQ(upper_bound->value_.Value(), 0.2); +} + // // PatternPart with variable. // TYPED_TEST(CypherMainVisitorTest, PatternPartVariable) { // ParserTables parser("CREATE var=()--()");