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

@@ -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<GraphDbTypes::EdgeType> edge_types_;
   // TODO: change to unordered_map
   std::map<GraphDbTypes::Property, Expression *> properties_;
+  bool has_range_ = false;
+  Expression *lower_bound_ = nullptr;
+  Expression *upper_bound_ = nullptr;
 
  protected:
   using PatternAtom::PatternAtom;

src/query/frontend/ast/cypher_main_visitor.cpp

@@ -4,6 +4,7 @@
 #include <climits>
 #include <codecvt>
 #include <cstring>
+#include <limits>
 #include <string>
 #include <unordered_map>
 #include <utility>
@@ -434,17 +435,15 @@ antlrcpp::Any CypherMainVisitor::visitRelationshipPattern(
               .as<std::map<GraphDbTypes::Property, Expression *>>();
     }
     if (ctx->relationshipDetail()->rangeLiteral()) {
-      // TODO: implement other clauses.
+      edge->has_range_ = true;
-      throw utils::NotYetImplemented("variable relationship length");
+      auto range = ctx->relationshipDetail()
+                       ->rangeLiteral()
+                       ->accept(this)
+                       .as<std::pair<Expression *, Expression *>>();
+      edge->lower_bound_ = range.first;
+      edge->upper_bound_ = range.second;
     }
   }
-  //    relationship.has_range = true;
-  //    auto range = ctx->relationshipDetail()
-  //                     ->rangeLiteral()
-  //                     ->accept(this)
-  //                     .as<std::pair<int64_t, int64_t>>();
-  //    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) {
+  debug_assert(ctx->expression().size() <= 2U,
-    // -[*]-
+               "Expected 0, 1 or 2 bounds in range literal.");
-    return std::pair<int64_t, int64_t>(1LL, LLONG_MAX);
+  if (ctx->expression().size() == 0U) {
-  } else if (ctx->integerLiteral().size() == 1U) {
+    // Case -[*]-
+    return std::pair<Expression *, Expression *>(nullptr, nullptr);
+  } else if (ctx->expression().size() == 1U) {
     auto dots_tokens = ctx->getTokens(kDotsTokenId);
-    int64_t bound = ctx->integerLiteral()[0]->accept(this).as<int64_t>();
+    Expression *bound = ctx->expression()[0]->accept(this);
     if (!dots_tokens.size()) {
-      // -[*2]-
+      // Case -[*bound]-
-      return std::pair<int64_t, int64_t>(bound, bound);
+      return std::pair<Expression *, Expression *>(bound, bound);
     }
     if (dots_tokens[0]->getSourceInterval().startsAfter(
-            ctx->integerLiteral()[0]->getSourceInterval())) {
+            ctx->expression()[0]->getSourceInterval())) {
-      // -[*2..]-
+      // Case -[*bound..]-
-      return std::pair<int64_t, int64_t>(bound, LLONG_MAX);
+      return std::pair<Expression *, Expression *>(bound, nullptr);
     } else {
-      // -[*..2]-
+      // Case -[*..bound]-
-      return std::pair<int64_t, int64_t>(1LL, bound);
+      return std::pair<Expression *, Expression *>(nullptr, bound);
     }
   } else {
-    int64_t lbound = ctx->integerLiteral()[0]->accept(this).as<int64_t>();
+    // Case -[*lbound..rbound]-
-    int64_t rbound = ctx->integerLiteral()[1]->accept(this).as<int64_t>();
+    Expression *lbound = ctx->expression()[0]->accept(this);
-    // -[*2..5]-
+    Expression *rbound = ctx->expression()[1]->accept(this);
-    return std::pair<int64_t, int64_t>(lbound, rbound);
+    return std::pair<Expression *, Expression *>(lbound, rbound);
   }
 }
 

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 ;
 

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_) {

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.

tests/unit/cypher_main_visitor.cpp

@@ -1,5 +1,6 @@
 #include <algorithm>
 #include <climits>
+#include <limits>
 #include <string>
 #include <unordered_map>
 #include <vector>
@@ -120,7 +121,7 @@ typedef ::testing::Types<AstGenerator, OriginalAfterCloningAstGenerator,
 TYPED_TEST_CASE(CypherMainVisitorTest, AstGeneratorTypes);
 
 TYPED_TEST(CypherMainVisitorTest, SyntaxException) {
-  ASSERT_THROW(TypeParam("CREATE ()-[*1...2]-()"), SyntaxException);
+  ASSERT_THROW(TypeParam("CREATE ()-[*1....2]-()"), SyntaxException);
 }
 
 TYPED_TEST(CypherMainVisitorTest, SyntaxExceptionOnTrailingText) {
@@ -885,54 +886,105 @@ TYPED_TEST(CypherMainVisitorTest, RelationshipPatternVariable) {
   EXPECT_TRUE(edge->identifier_->user_declared_);
 }
 
-// // Relationship with unbounded variable range.
+// Assert that match has a single pattern with a single edge atom and store it
-// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUnbounded) {
+// in edge parameter.
-//   ParserTables parser("CREATE ()-[*]-()");
+void AssertMatchSingleEdgeAtom(Match *match, EdgeAtom *&edge) {
-//   ASSERT_EQ(parser.identifiers_map_.size(), 0U);
+  ASSERT_TRUE(match);
-//   ASSERT_EQ(parser.relationships_.size(), 1U);
+  ASSERT_EQ(match->patterns_.size(), 1U);
-//   CompareRelationships(*parser.relationships_.begin(),
+  ASSERT_EQ(match->patterns_[0]->atoms_.size(), 3U);
-//                        Relationship::Direction::BOTH, {}, {}, true, 1,
+  edge = dynamic_cast<EdgeAtom *>(match->patterns_[0]->atoms_[1]);
-//                        LLONG_MAX);
+  ASSERT_TRUE(edge);
-// }
+}
-//
+
-// // Relationship with lower bounded variable range.
+TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUnbounded) {
-// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerBounded) {
+  TypeParam ast_generator("MATCH ()-[r*]->() RETURN r");
-//   ParserTables parser("CREATE ()-[*5..]-()");
+  auto *query = ast_generator.query_;
-//   ASSERT_EQ(parser.identifiers_map_.size(), 0U);
+  auto *match = dynamic_cast<Match *>(query->clauses_[0]);
-//   ASSERT_EQ(parser.relationships_.size(), 1U);
+  EdgeAtom *edge = nullptr;
-//   CompareRelationships(*parser.relationships_.begin(),
+  AssertMatchSingleEdgeAtom(match, edge);
-//                        Relationship::Direction::BOTH, {}, {}, true, 5,
+  EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT);
-//                        LLONG_MAX);
+  EXPECT_TRUE(edge->has_range_);
-// }
+  EXPECT_EQ(edge->lower_bound_, nullptr);
-//
+  EXPECT_EQ(edge->upper_bound_, nullptr);
-// // Relationship with upper bounded variable range.
+}
-// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUpperBounded) {
+
-//   ParserTables parser("CREATE ()-[*..10]-()");
+TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerBounded) {
-//   ASSERT_EQ(parser.identifiers_map_.size(), 0U);
+  TypeParam ast_generator("MATCH ()-[r*42..]->() RETURN r");
-//   ASSERT_EQ(parser.relationships_.size(), 1U);
+  auto *query = ast_generator.query_;
-//   CompareRelationships(*parser.relationships_.begin(),
+  auto *match = dynamic_cast<Match *>(query->clauses_[0]);
-//                        Relationship::Direction::BOTH, {}, {}, true, 1, 10);
+  EdgeAtom *edge = nullptr;
-// }
+  AssertMatchSingleEdgeAtom(match, edge);
-//
+  EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT);
-// // Relationship with lower and upper bounded variable range.
+  EXPECT_TRUE(edge->has_range_);
-// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerUpperBounded) {
+  auto *lower_bound = dynamic_cast<PrimitiveLiteral *>(edge->lower_bound_);
-//   ParserTables parser("CREATE ()-[*5..10]-()");
+  ASSERT_TRUE(lower_bound);
-//   ASSERT_EQ(parser.identifiers_map_.size(), 0U);
+  EXPECT_TRUE(lower_bound->value_.Value<int64_t>() == 42);
-//   ASSERT_EQ(parser.relationships_.size(), 1U);
+  EXPECT_EQ(edge->upper_bound_, nullptr);
-//   CompareRelationships(*parser.relationships_.begin(),
+}
-//                        Relationship::Direction::BOTH, {}, {}, true, 5, 10);
+
-// }
+TYPED_TEST(CypherMainVisitorTest, RelationshipPatternUpperBounded) {
-//
+  TypeParam ast_generator("MATCH ()-[r*..42]->() RETURN r");
-// // Relationship with fixed number of edges.
+  auto *query = ast_generator.query_;
-// TYPED_TEST(CypherMainVisitorTest, RelationshipPatternFixedRange) {
+  auto *match = dynamic_cast<Match *>(query->clauses_[0]);
-//   ParserTables parser("CREATE ()-[*10]-()");
+  EdgeAtom *edge = nullptr;
-//   ASSERT_EQ(parser.identifiers_map_.size(), 0U);
+  AssertMatchSingleEdgeAtom(match, edge);
-//   ASSERT_EQ(parser.relationships_.size(), 1U);
+  EXPECT_EQ(edge->direction_, EdgeAtom::Direction::OUT);
-//   CompareRelationships(*parser.relationships_.begin(),
+  EXPECT_TRUE(edge->has_range_);
-//                        Relationship::Direction::BOTH, {}, {}, true, 10, 10);
+  EXPECT_EQ(edge->lower_bound_, nullptr);
-// }
+  auto upper_bound = dynamic_cast<PrimitiveLiteral *>(edge->upper_bound_);
-//
+  ASSERT_TRUE(upper_bound);
-//
+  EXPECT_EQ(upper_bound->value_.Value<int64_t>(), 42);
+}
+
+TYPED_TEST(CypherMainVisitorTest, RelationshipPatternLowerUpperBounded) {
+  TypeParam ast_generator("MATCH ()-[r*24..42]->() RETURN r");
+  auto *query = ast_generator.query_;
+  auto *match = dynamic_cast<Match *>(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<PrimitiveLiteral *>(edge->lower_bound_);
+  ASSERT_TRUE(lower_bound);
+  EXPECT_EQ(lower_bound->value_.Value<int64_t>(), 24);
+  auto upper_bound = dynamic_cast<PrimitiveLiteral *>(edge->upper_bound_);
+  ASSERT_TRUE(upper_bound);
+  EXPECT_EQ(upper_bound->value_.Value<int64_t>(), 42);
+}
+
+TYPED_TEST(CypherMainVisitorTest, RelationshipPatternFixedRange) {
+  TypeParam ast_generator("MATCH ()-[r*42]->() RETURN r");
+  auto *query = ast_generator.query_;
+  auto *match = dynamic_cast<Match *>(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<PrimitiveLiteral *>(edge->lower_bound_);
+  ASSERT_TRUE(lower_bound);
+  EXPECT_EQ(lower_bound->value_.Value<int64_t>(), 42);
+  auto upper_bound = dynamic_cast<PrimitiveLiteral *>(edge->upper_bound_);
+  ASSERT_TRUE(upper_bound);
+  EXPECT_EQ(upper_bound->value_.Value<int64_t>(), 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<Match *>(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<PrimitiveLiteral *>(edge->lower_bound_);
+  ASSERT_TRUE(lower_bound);
+  EXPECT_EQ(lower_bound->value_.Value<int64_t>(), 1);
+  auto upper_bound = dynamic_cast<PrimitiveLiteral *>(edge->upper_bound_);
+  ASSERT_TRUE(upper_bound);
+  EXPECT_EQ(upper_bound->value_.Value<double>(), 0.2);
+}
+
 // // PatternPart with variable.
 // TYPED_TEST(CypherMainVisitorTest, PatternPartVariable) {
 //   ParserTables parser("CREATE var=()--()");