Fix issues with lambda symbols and returning *

Summary:
Correctly propagate user declared symbols for lambda.
Unbind lambda symbols after inlining filters.
Also update unit tests.

Reviewers: florijan, mislav.bradac

Reviewed By: florijan

Subscribers: pullbot

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

src/query/frontend/semantic/symbol_generator.cpp

@@ -376,8 +376,14 @@ bool SymbolGenerator::PreVisit(EdgeAtom &edge_atom) {
       VisitWithIdentifiers(*edge_atom.filter_expression_,
                            {edge_atom.inner_edge_, edge_atom.inner_node_});
     } else {
-      for (auto i : {edge_atom.inner_edge_, edge_atom.inner_node_})
+      // Create inner symbols, but don't bind them in scope, since they are to
-        symbol_table_[*i] = CreateSymbol(i->name_, true);
+      // be used in the missing filter expression.
+      symbol_table_[*edge_atom.inner_edge_] = symbol_table_.CreateSymbol(
+          edge_atom.inner_edge_->name_, edge_atom.inner_edge_->user_declared_,
+          Symbol::Type::Edge);
+      symbol_table_[*edge_atom.inner_node_] = symbol_table_.CreateSymbol(
+          edge_atom.inner_node_->name_, edge_atom.inner_node_->user_declared_,
+          Symbol::Type::Vertex);
     }
     scope_.in_pattern = true;
   }
@@ -404,7 +410,8 @@ void SymbolGenerator::VisitWithIdentifiers(
     if (prev_symbol_it != scope_.symbols.end()) {
       prev_symbol = prev_symbol_it->second;
     }
-    symbol_table_[*identifier] = CreateSymbol(identifier->name_, true);
+    symbol_table_[*identifier] =
+        CreateSymbol(identifier->name_, identifier->user_declared_);
     prev_symbols.emplace_back(prev_symbol, identifier);
   }
   // Visit the tree with the new symbols bound.

src/query/plan/rule_based_planner.hpp

@@ -376,6 +376,9 @@ class RuleBasedPlanner {
                                       utils::Contains(fi.used_symbols, n);
                              }),
               filters.end());
+          // Unbind the temporarily bound inner symbols for filtering.
+          bound_symbols.erase(inner_edge_symbol);
+          bound_symbols.erase(inner_node_symbol);
 
           last_op = new ExpandVariable(
               node_symbol, edge_symbol, edge->type_, expansion.direction,

tests/unit/cypher_main_visitor.cpp

@@ -1395,9 +1395,27 @@ TYPED_TEST(CypherMainVisitorTest, MatchBfsReturn) {
                            ast_generator.db_accessor_.EdgeType("type2")));
   EXPECT_EQ(bfs->identifier_->name_, "r");
   EXPECT_EQ(bfs->inner_edge_->name_, "e");
+  EXPECT_TRUE(bfs->inner_edge_->user_declared_);
   EXPECT_EQ(bfs->inner_node_->name_, "n");
+  EXPECT_TRUE(bfs->inner_node_->user_declared_);
   CheckLiteral(ast_generator.context_, bfs->upper_bound_, 10);
   auto *eq = dynamic_cast<EqualOperator *>(bfs->filter_expression_);
   ASSERT_TRUE(eq);
 }
+
+TYPED_TEST(CypherMainVisitorTest, MatchVariableLambdaSymbols) {
+  TypeParam ast_generator("MATCH () -[*]- () RETURN *");
+  auto *query = ast_generator.query_;
+  ASSERT_EQ(query->clauses_.size(), 2U);
+  auto *match = dynamic_cast<Match *>(query->clauses_[0]);
+  ASSERT_TRUE(match);
+  ASSERT_EQ(match->patterns_.size(), 1U);
+  ASSERT_EQ(match->patterns_[0]->atoms_.size(), 3U);
+  auto *var_expand = dynamic_cast<EdgeAtom *>(match->patterns_[0]->atoms_[1]);
+  ASSERT_TRUE(var_expand);
+  ASSERT_TRUE(var_expand->IsVariable());
+  EXPECT_FALSE(var_expand->inner_edge_->user_declared_);
+  EXPECT_FALSE(var_expand->inner_node_->user_declared_);
+}
+
 }  // namespace

tests/unit/query_planner.cpp

@@ -1427,4 +1427,32 @@ TEST(TestLogicalPlanner, MatchWhereAndSplit) {
             ExpectFilter(), ExpectProduce());
 }
 
+TEST(TestLogicalPlanner, ReturnAsteriskOmitsLambdaSymbols) {
+  // Test MATCH (n) -[r* (ie, in | true)]- (m) RETURN *
+  GraphDb db;
+  GraphDbAccessor dba(db);
+  AstTreeStorage storage;
+  auto edge = EDGE_VARIABLE("r", Direction::BOTH);
+  edge->inner_edge_ = IDENT("ie");
+  edge->inner_node_ = IDENT("in");
+  edge->filter_expression_ = LITERAL(true);
+  auto ret = storage.Create<query::Return>();
+  ret->body_.all_identifiers = true;
+  QUERY(MATCH(PATTERN(NODE("n"), edge, NODE("m"))), ret);
+  auto symbol_table = MakeSymbolTable(*storage.query());
+  auto planning_context = MakePlanningContext(storage, symbol_table, dba);
+  auto plan = MakeLogicalPlan<RuleBasedPlanner>(planning_context);
+  auto *produce = dynamic_cast<Produce *>(plan.get());
+  ASSERT_TRUE(produce);
+  std::vector<std::string> outputs;
+  for (const auto &output_symbol : produce->OutputSymbols(symbol_table)) {
+    outputs.emplace_back(output_symbol.name());
+  }
+  // We expect `*` expanded to `n`, `r` and `m`.
+  EXPECT_EQ(outputs.size(), 3);
+  for (const auto &name : {"n", "r", "m"}) {
+    EXPECT_TRUE(utils::Contains(outputs, name));
+  }
+}
+
 }  // namespace

tests/unit/query_semantic.cpp

@@ -1020,10 +1020,12 @@ TEST(TestSymbolGenerator, MatchBfsReturn) {
   EXPECT_EQ(symbol_table.max_position(), 7);
   EXPECT_EQ(symbol_table.at(*ret_r), symbol_table.at(*bfs->identifier_));
   EXPECT_NE(symbol_table.at(*ret_r), symbol_table.at(*bfs->inner_edge_));
+  EXPECT_TRUE(symbol_table.at(*bfs->inner_edge_).user_declared());
   EXPECT_EQ(symbol_table.at(*bfs->inner_edge_),
             symbol_table.at(*r_prop->expression_));
   EXPECT_NE(symbol_table.at(*node_n->identifier_),
             symbol_table.at(*bfs->inner_node_));
+  EXPECT_TRUE(symbol_table.at(*bfs->inner_node_).user_declared());
   EXPECT_EQ(symbol_table.at(*node_n->identifier_),
             symbol_table.at(*n_prop->expression_));
 }
@@ -1076,4 +1078,35 @@ TEST(TestSymbolGenerator, MatchBfsUsesLaterSymbolError) {
   EXPECT_THROW(query->Accept(symbol_generator), UnboundVariableError);
 }
 
+TEST(TestSymbolGenerator, MatchVariableLambdaSymbols) {
+  // MATCH ()-[*]-() RETURN 42 AS res
+  AstTreeStorage storage;
+  auto ident_n = storage.Create<Identifier>("anon_n", false);
+  auto node = storage.Create<NodeAtom>(ident_n);
+  auto edge = storage.Create<EdgeAtom>(
+      storage.Create<Identifier>("anon_r", false), EdgeAtom::Type::DEPTH_FIRST,
+      EdgeAtom::Direction::BOTH);
+  edge->inner_edge_ = storage.Create<Identifier>("anon_inner_e", false);
+  edge->inner_node_ = storage.Create<Identifier>("anon_inner_n", false);
+  auto end_node =
+      storage.Create<NodeAtom>(storage.Create<Identifier>("anon_end", false));
+  auto query = QUERY(MATCH(PATTERN(node, edge, end_node)),
+                     RETURN(LITERAL(42), AS("res")));
+  SymbolTable symbol_table;
+  SymbolGenerator symbol_generator(symbol_table);
+  query->Accept(symbol_generator);
+  // Symbols for `anon_n`, `anon_r`, `anon_inner_e`, `anon_inner_n`, `anon_end`
+  // `AS res` and the auto-generated path name symbol.
+  EXPECT_EQ(symbol_table.max_position(), 7);
+  // All symbols except `AS res` are anonymously generated.
+  for (const auto &id_and_symbol : symbol_table.table()) {
+    const auto &symbol = id_and_symbol.second;
+    if (symbol.name() == "res") {
+      EXPECT_TRUE(symbol.user_declared());
+    } else {
+      EXPECT_FALSE(id_and_symbol.second.user_declared());
+    }
+  }
+}
+
 }  // namespace