Add tests for symbol generation in CREATE clause

Summary:
Add tests for symbol generation in CREATE clause and correctly (hopefully)
check symbols in create clause and properties.

Reviewers: florijan, mislav.bradac, buda

Reviewed By: mislav.bradac

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D135

src/query/exceptions.hpp

@@ -26,6 +26,18 @@ class SemanticException : public BasicException {
   SemanticException() : BasicException("") {}
 };
 
+class UnboundVariableError : public SemanticException {
+ public:
+  UnboundVariableError(const std::string &name)
+      : SemanticException("Unbound variable: " + name) {}
+};
+
+class RedeclareVariableError : public SemanticException {
+ public:
+  RedeclareVariableError(const std::string& name)
+      : SemanticException("Redeclaring variable: " + name) {}
+};
+
 class CppCodeGeneratorException : public StacktraceException {
  public:
   using StacktraceException::StacktraceException;

src/query/frontend/ast/ast.hpp

@@ -82,8 +82,12 @@ class NamedExpression : public Tree {
 
 class PatternAtom : public Tree {
   friend class AstTreeStorage;
+ public:
+  Identifier* identifier_ = nullptr;
  protected:
   PatternAtom(int uid) : Tree(uid) {}
+  PatternAtom(int uid, Identifier *identifier)
+      : Tree(uid), identifier_(identifier) {}
 };
 
 class NodeAtom : public PatternAtom {
@@ -95,14 +99,11 @@ class NodeAtom : public PatternAtom {
     visitor.PostVisit(*this);
   }
 
-  Identifier* identifier_ = nullptr;
   std::vector<GraphDb::Label> labels_;
   std::map<GraphDb::Property, Expression*> properties_;
 
  protected:
-  NodeAtom(int uid) : PatternAtom(uid) {}
+  using PatternAtom::PatternAtom;
-  NodeAtom(int uid, Identifier *identifier) :
-      PatternAtom(uid), identifier_(identifier) {}
 };
 
 class EdgeAtom : public PatternAtom {
@@ -117,11 +118,10 @@ class EdgeAtom : public PatternAtom {
   }
 
   Direction direction_ = Direction::BOTH;
-  Identifier* identifier_ = nullptr;
   std::vector<GraphDb::EdgeType> types_;
 
  protected:
-  EdgeAtom(int uid) : PatternAtom(uid) {}
+  using PatternAtom::PatternAtom;
 };
 
 class Clause : public Tree {
@@ -166,7 +166,7 @@ class Query : public Tree {
 class Create : public Clause {
  public:
   Create(int uid) : Clause(uid) {}
-  std::vector<std::shared_ptr<Pattern>> patterns_;
+  std::vector<Pattern*> patterns_;
   void Accept(TreeVisitorBase &visitor) override {
     visitor.Visit(*this);
     for (auto &pattern : patterns_) {

src/query/frontend/semantic/symbol_generator.hpp

@@ -8,13 +8,20 @@ namespace query {
 
 class SymbolGenerator : public TreeVisitorBase {
  public:
-  SymbolGenerator(SymbolTable& symbol_table) : symbol_table_(symbol_table) {}
+  SymbolGenerator(SymbolTable &symbol_table) : symbol_table_(symbol_table) {}
 
   using TreeVisitorBase::Visit;
   using TreeVisitorBase::PostVisit;
 
   // Clauses
-  void PostVisit(Return& ret) override {
+  void Visit(Create &create) override {
+    scope_.in_create = true;
+  }
+  void PostVisit(Create &create) override {
+    scope_.in_create = false;
+  }
+
+  void PostVisit(Return &ret) override {
     for (auto &named_expr : ret.named_expressions_) {
       // Named expressions establish bindings for expressions which come after
       // return, but not for the expressions contained inside.
@@ -23,47 +30,92 @@ class SymbolGenerator : public TreeVisitorBase {
   }
 
   // Expressions
-  void Visit(Identifier& ident) override {
+  void Visit(Identifier &ident) override {
     Symbol symbol;
     if (scope_.in_pattern) {
+      // Patterns can bind new symbols or reference already bound. But there
+      // are the following special cases:
+      //  1) Expressions in property maps `{prop_name: expr}` can only reference
+      //     bound variables.
+      //  2) 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`.
+      if (scope_.in_property_map && !HasSymbol(ident.name_)) {
+        // Case 1)
+        throw UnboundVariableError(ident.name_);
+      } else if ((scope_.in_create_node || scope_.in_create_edge) &&
+                 HasSymbol(ident.name_)) {
+        // Case 2)
+        throw RedeclareVariableError(ident.name_);
+      }
       symbol = GetOrCreateSymbol(ident.name_);
     } else {
+      // Everything else references a bound symbol.
       if (!HasSymbol(ident.name_))
-        // TODO: Special exception for type check
+        throw UnboundVariableError(ident.name_);
-        throw SemanticException("Unbound identifier: " + ident.name_);
       symbol = scope_.variables[ident.name_];
     }
     symbol_table_[ident] = symbol;
   }
+
   // Pattern and its subparts.
-  void Visit(Pattern& pattern) override {
+  void Visit(Pattern &pattern) override {
     scope_.in_pattern = true;
+    if (scope_.in_create && pattern.atoms_.size() == 1) {
+      debug_assert(dynamic_cast<NodeAtom*>(pattern.atoms_[0]),
+                   "Expected a single NodeAtom in Pattern");
+      scope_.in_create_node = true;
+    }
   }
-  void PostVisit(Pattern& pattern) override {
+  void PostVisit(Pattern &pattern) override {
     scope_.in_pattern = false;
+    scope_.in_create_node = false;
+  }
+
+  void Visit(NodeAtom &node_atom) override {
+    scope_.in_property_map = true;
+    for (auto kv : node_atom.properties_) {
+      kv.second->Accept(*this);
+    }
+    scope_.in_property_map = false;
+  }
+
+  void Visit(EdgeAtom &edge_atom) override {
+    if (scope_.in_create) {
+      scope_.in_create_edge = true;
+    }
+  }
+  void PostVisit(EdgeAtom &edge_atom) override {
+    scope_.in_create_edge = false;
   }
 
  private:
   struct Scope {
-    Scope() : in_pattern(false) {}
+    bool in_pattern{false};
-    bool in_pattern;
+    bool in_create{false};
+    bool in_create_node{false};
+    bool in_create_edge{false};
+    bool in_property_map{false};
     std::map<std::string, Symbol> variables;
   };
 
-  bool HasSymbol(const std::string& name)
+  bool HasSymbol(const std::string &name) {
-  {
     return scope_.variables.find(name) != scope_.variables.end();
   }
 
-  Symbol CreateSymbol(const std::string &name)
+  Symbol CreateSymbol(const std::string &name) {
-  {
     auto symbol = symbol_table_.CreateSymbol(name);
     scope_.variables[name] = symbol;
     return symbol;
   }
 
-  Symbol GetOrCreateSymbol(const std::string& name)
+  Symbol GetOrCreateSymbol(const std::string &name) {
-  {
     auto search = scope_.variables.find(name);
     if (search != scope_.variables.end()) {
       return search->second;
@@ -71,7 +123,7 @@ class SymbolGenerator : public TreeVisitorBase {
     return CreateSymbol(name);
   }
 
-  SymbolTable& symbol_table_;
+  SymbolTable &symbol_table_;
   Scope scope_;
 };
 

tests/unit/query_semantic.cpp

@@ -11,15 +11,35 @@ using namespace query;
 
 namespace {
 
+// Returns a `(name1) -[name2]- (name3) ...` pattern.
+auto GetPattern(AstTreeStorage &storage, std::vector<std::string> names) {
+  bool is_node{true};
+  auto pattern = storage.Create<Pattern>();
+  for (auto &name : names) {
+    PatternAtom *atom;
+    auto identifier = storage.Create<Identifier>(name);
+    if (is_node) {
+      atom = storage.Create<NodeAtom>(identifier);
+    } else {
+      atom = storage.Create<EdgeAtom>(identifier);
+    }
+    pattern->atoms_.emplace_back(atom);
+    is_node = !is_node;
+  }
+  return pattern;
+}
+
+// Returns a `MATCH (node)` clause.
+auto GetMatchNode(AstTreeStorage &storage, const std::string &node_name) {
+  auto match = storage.Create<Match>();
+  match->patterns_.emplace_back(GetPattern(storage, {node_name}));
+  return match;
+}
+
 // Build a simple AST which describes:
 // MATCH (node_atom_1) RETURN node_atom_1 AS node_atom_1
 Query *MatchNodeReturn(AstTreeStorage &storage) {
-  auto node_atom = storage.Create<NodeAtom>();
+  auto match = GetMatchNode(storage, "node_atom_1");
-  node_atom->identifier_ = storage.Create<Identifier>("node_atom_1");
-  auto pattern = storage.Create<Pattern>();
-  pattern->atoms_.emplace_back(node_atom);
-  auto match = storage.Create<Match>();
-  match->patterns_.emplace_back(pattern);
   auto query = storage.query();
   query->clauses_.emplace_back(match);
 
@@ -36,12 +56,7 @@ Query *MatchNodeReturn(AstTreeStorage &storage) {
 // This is treated as an unbound variable.
 // MATCH (node_atom_1) RETURN node_atom_1 AS n, n AS n
 Query *MatchUnboundMultiReturn(AstTreeStorage &storage) {
-  auto node_atom = storage.Create<NodeAtom>();
+  auto match = GetMatchNode(storage, "node_atom_1");
-  node_atom->identifier_ = storage.Create<Identifier>("node_atom_1");
-  auto pattern = storage.Create<Pattern>();
-  pattern->atoms_.emplace_back(node_atom);
-  auto match = storage.Create<Match>();
-  match->patterns_.emplace_back(pattern);
   auto query = storage.query();
   query->clauses_.emplace_back(match);
 
@@ -60,12 +75,7 @@ Query *MatchUnboundMultiReturn(AstTreeStorage &storage) {
 
 // AST with unbound variable in return: MATCH (n) RETURN x AS x
 Query *MatchNodeUnboundReturn(AstTreeStorage &storage) {
-  auto node_atom = storage.Create<NodeAtom>();
+  auto match = GetMatchNode(storage, "n");
-  node_atom->identifier_ = storage.Create<Identifier>("n");
-  auto pattern = storage.Create<Pattern>();
-  pattern->atoms_.emplace_back(node_atom);
-  auto match = storage.Create<Match>();
-  match->patterns_.emplace_back(pattern);
   auto query = storage.query();
   query->clauses_.emplace_back(match);
 
@@ -78,6 +88,123 @@ Query *MatchNodeUnboundReturn(AstTreeStorage &storage) {
   return query;
 }
 
+// AST with match pattern referencing an edge multiple times:
+// MATCH (n) -[r]-> (n) -[r]-> (n) RETURN r AS r
+// This usually throws a redeclaration error, but we support it.
+Query *MatchSameEdge(AstTreeStorage &storage) {
+  auto match = storage.Create<Match>();
+  match->patterns_.emplace_back(GetPattern(storage, {"n", "r", "n", "r", "n"}));
+  auto query = storage.query();
+  query->clauses_.emplace_back(match);
+
+  auto named_expr = storage.Create<NamedExpression>();
+  named_expr->name_ = "r";
+  named_expr->expression_ = storage.Create<Identifier>("r");
+  auto ret = storage.Create<Return>();
+  ret->named_expressions_.emplace_back(named_expr);
+  query->clauses_.emplace_back(ret);
+  return query;
+}
+
+std::string prop_name = "prop";
+
+// AST with unbound variable in create: CREATE ({prop: x})
+Query *CreatePropertyUnbound(AstTreeStorage &storage) {
+  auto prop_expr = storage.Create<Identifier>("x");
+  auto node_atom = storage.Create<NodeAtom>();
+  node_atom->identifier_ = storage.Create<Identifier>("anon");
+  node_atom->properties_[&prop_name] = prop_expr;
+  auto pattern = storage.Create<Pattern>();
+  pattern->atoms_.emplace_back(node_atom);
+  auto create = storage.Create<Create>();
+  create->patterns_.emplace_back(pattern);
+  auto query = storage.query();
+  query->clauses_.emplace_back(create);
+  return query;
+}
+
+// Simple AST returning a created node: CREATE (n) RETURN n
+Query *CreateNodeReturn(AstTreeStorage &storage) {
+  auto node_atom = storage.Create<NodeAtom>();
+  node_atom->identifier_ = storage.Create<Identifier>("n");
+  auto pattern = storage.Create<Pattern>();
+  pattern->atoms_.emplace_back(node_atom);
+  auto create = storage.Create<Create>();
+  create->patterns_.emplace_back(pattern);
+  auto query = storage.query();
+  query->clauses_.emplace_back(create);
+
+  auto named_expr = storage.Create<NamedExpression>();
+  named_expr->name_ = "n";
+  named_expr->expression_ = storage.Create<Identifier>("n");
+  auto ret = storage.Create<Return>();
+  ret->named_expressions_.emplace_back(named_expr);
+  query->clauses_.emplace_back(ret);
+  return query;
+}
+
+// AST with redeclaring a variable when creating nodes: CREATE (n), (n)
+Query *CreateRedeclareNode(AstTreeStorage &storage) {
+  auto create = storage.Create<Create>();
+  for (int patterns = 0; patterns < 2; ++patterns) {
+    auto pattern = storage.Create<Pattern>();
+    auto node_atom = storage.Create<NodeAtom>();
+    node_atom->identifier_ = storage.Create<Identifier>("n");
+    pattern->atoms_.emplace_back(node_atom);
+    create->patterns_.emplace_back(pattern);
+  }
+  auto query = storage.query();
+  query->clauses_.emplace_back(create);
+  return query;
+}
+
+// AST with redeclaring a variable when creating nodes with multiple creates:
+// CREATE (n) CREATE (n)
+Query *MultiCreateRedeclareNode(AstTreeStorage &storage) {
+  auto query = storage.query();
+
+  for (int creates = 0; creates < 2; ++creates) {
+    auto pattern = storage.Create<Pattern>();
+    auto node_atom = storage.Create<NodeAtom>();
+    node_atom->identifier_ = storage.Create<Identifier>("n");
+    pattern->atoms_.emplace_back(node_atom);
+    auto create = storage.Create<Create>();
+    create->patterns_.emplace_back(pattern);
+    query->clauses_.emplace_back(create);
+  }
+  return query;
+}
+
+// AST with redeclaring a match node variable in create: MATCH (n) CREATE (n)
+Query *MatchCreateRedeclareNode(AstTreeStorage &storage) {
+  auto match = GetMatchNode(storage, "n");
+  auto query = storage.query();
+  query->clauses_.emplace_back(match);
+
+  auto node_atom_2 = storage.Create<NodeAtom>();
+  node_atom_2->identifier_ = storage.Create<Identifier>("n");
+  auto pattern_2 = storage.Create<Pattern>();
+  pattern_2->atoms_.emplace_back(node_atom_2);
+  auto create = storage.Create<Create>();
+  create->patterns_.emplace_back(pattern_2);
+  query->clauses_.emplace_back(create);
+  return query;
+}
+
+// AST with redeclaring a match edge variable in create:
+// MATCH (n) -[r]-> (m) CREATE (n) -[r] -> (l)
+Query *MatchCreateRedeclareEdge(AstTreeStorage &storage) {
+  auto match = storage.Create<Match>();
+  match->patterns_.emplace_back(GetPattern(storage, {"n", "r", "m"}));
+  auto query = storage.query();
+  query->clauses_.emplace_back(match);
+
+  auto create = storage.Create<Create>();
+  create->patterns_.emplace_back(GetPattern(storage, {"n", "r", "l"}));
+  query->clauses_.emplace_back(create);
+  return query;
+}
+
 TEST(TestSymbolGenerator, MatchNodeReturn) {
   SymbolTable symbol_table;
   AstTreeStorage storage;
@@ -104,7 +231,7 @@ TEST(TestSymbolGenerator, MatchUnboundMultiReturn) {
   AstTreeStorage storage;
   auto query_ast = MatchUnboundMultiReturn(storage);
   SymbolGenerator symbol_generator(symbol_table);
-  EXPECT_THROW(query_ast->Accept(symbol_generator), SemanticException);
+  EXPECT_THROW(query_ast->Accept(symbol_generator), UnboundVariableError);
 }
 
 TEST(TestSymbolGenerator, MatchNodeUnboundReturn) {
@@ -112,6 +239,103 @@ TEST(TestSymbolGenerator, MatchNodeUnboundReturn) {
   AstTreeStorage storage;
   auto query_ast = MatchNodeUnboundReturn(storage);
   SymbolGenerator symbol_generator(symbol_table);
-  EXPECT_THROW(query_ast->Accept(symbol_generator), SemanticException);
+  EXPECT_THROW(query_ast->Accept(symbol_generator), UnboundVariableError);
 }
+
+TEST(TestSymbolGenerator, MatchSameEdge) {
+  SymbolTable symbol_table;
+  AstTreeStorage storage;
+  auto query_ast = MatchSameEdge(storage);
+  SymbolGenerator symbol_generator(symbol_table);
+  query_ast->Accept(symbol_generator);
+  EXPECT_EQ(symbol_table.max_position(), 3);
+  auto match = dynamic_cast<Match*>(query_ast->clauses_[0]);
+  auto pattern = match->patterns_[0];
+  std::vector<Symbol> node_symbols;
+  std::vector<Symbol> 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();
+  for (auto &symbol : node_symbols) {
+    EXPECT_EQ(node_symbol, symbol);
+  }
+  auto &edge_symbol = edge_symbols.front();
+  for (auto &symbol : edge_symbols) {
+    EXPECT_EQ(edge_symbol, symbol);
+  }
+  auto ret = dynamic_cast<Return*>(query_ast->clauses_[1]);
+  auto named_expr = ret->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;
+  auto query_ast = CreatePropertyUnbound(storage);
+  SymbolGenerator symbol_generator(symbol_table);
+  EXPECT_THROW(query_ast->Accept(symbol_generator), UnboundVariableError);
+}
+
+TEST(TestSymbolGenerator, CreateNodeReturn) {
+  SymbolTable symbol_table;
+  AstTreeStorage storage;
+  auto query_ast = CreateNodeReturn(storage);
+  SymbolGenerator symbol_generator(symbol_table);
+  query_ast->Accept(symbol_generator);
+  EXPECT_EQ(symbol_table.max_position(), 2);
+  auto create = dynamic_cast<Create*>(query_ast->clauses_[0]);
+  auto pattern = create->patterns_[0];
+  auto node_atom = dynamic_cast<NodeAtom*>(pattern->atoms_[0]);
+  auto node_sym = symbol_table[*node_atom->identifier_];
+  EXPECT_EQ(node_sym.name_, "n");
+  auto ret = dynamic_cast<Return*>(query_ast->clauses_[1]);
+  auto named_expr = ret->named_expressions_[0];
+  auto column_sym = symbol_table[*named_expr];
+  EXPECT_EQ(node_sym.name_, column_sym.name_);
+  EXPECT_NE(node_sym, column_sym);
+  auto ret_sym = symbol_table[*named_expr->expression_];
+  EXPECT_EQ(node_sym, ret_sym);
+}
+
+TEST(TestSymbolGenerator, CreateRedeclareNode) {
+  SymbolTable symbol_table;
+  AstTreeStorage storage;
+  auto query_ast = CreateRedeclareNode(storage);
+  SymbolGenerator symbol_generator(symbol_table);
+  EXPECT_THROW(query_ast->Accept(symbol_generator), RedeclareVariableError);
+}
+
+TEST(TestSymbolGenerator, MultiCreateRedeclareNode) {
+  SymbolTable symbol_table;
+  AstTreeStorage storage;
+  auto query_ast = MultiCreateRedeclareNode(storage);
+  SymbolGenerator symbol_generator(symbol_table);
+  EXPECT_THROW(query_ast->Accept(symbol_generator), RedeclareVariableError);
+}
+
+TEST(TestSymbolGenerator, MatchCreateRedeclareNode) {
+  SymbolTable symbol_table;
+  AstTreeStorage storage;
+  auto query_ast = MatchCreateRedeclareNode(storage);
+  SymbolGenerator symbol_generator(symbol_table);
+  EXPECT_THROW(query_ast->Accept(symbol_generator), RedeclareVariableError);
+}
+
+TEST(TestSymbolGenerator, MatchCreateRedeclareEdge) {
+  SymbolTable symbol_table;
+  AstTreeStorage storage;
+  auto query_ast = MatchCreateRedeclareEdge(storage);
+  SymbolGenerator symbol_generator(symbol_table);
+  EXPECT_THROW(query_ast->Accept(symbol_generator), RedeclareVariableError);
+}
+
 }