Allow planning Cartesian after Produce

Summary:
Hopefully, the mechanism of generating Cartesian is general enough, so
this simple change should work correctly in all cases.

Planner tests have been modified to use a FakeDbAccessor in order to
speed them up and potentially allow extracting planning into a library.

Reviewers: msantl, mtomic, buda

Reviewed By: mtomic

Subscribers: pullbot

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

src/query/plan/distributed.cpp

@@ -219,6 +219,163 @@ class IndependentSubtreeFinder : public HierarchicalLogicalOperatorVisitor {
     return true;
   }
 
+  bool PostVisit(Optional &optional) override {
+    throw utils::NotYetImplemented("distributed Cartesian planning");
+  }
+
+  bool PostVisit(Unwind &unwind) override {
+    if (subtree_) return true;
+    UsedSymbolsCollector collector(*symbol_table_);
+    unwind.input_expression()->Accept(collector);
+    if (auto found = ContainsForbidden(collector.symbols_)) {
+      subtree_ = unwind.input();
+      parent_ = &unwind;
+      depends_on_ = found;
+    }
+    return true;
+  }
+
+  // Remaining operators can only appear if they don't contain any forbidden
+  // symbols. This is the case when we are planning *another* Cartesian after
+  // Produce.
+
+  bool PostVisit(CreateNode &op) override {
+    CHECK(!FindForbidden(symbol_table_->at(*(op.node_atom()->identifier_))));
+    for (auto &kv : op.node_atom()->properties_) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      kv.second->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    return true;
+  }
+
+  bool PostVisit(CreateExpand &op) override {
+    CHECK(!FindForbidden(op.input_symbol()));
+    CHECK(!FindForbidden(symbol_table_->at(*(op.node_atom()->identifier_))));
+    CHECK(!FindForbidden(symbol_table_->at(*(op.edge_atom()->identifier_))));
+    for (auto &kv : op.node_atom()->properties_) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      kv.second->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    for (auto &kv : op.edge_atom()->properties_) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      kv.second->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    return true;
+  }
+
+  bool PostVisit(Delete &op) override {
+    for (auto *expr : op.expressions()) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      expr->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    return true;
+  }
+
+  bool PostVisit(SetProperty &op) override {
+    UsedSymbolsCollector collector(*symbol_table_);
+    op.lhs()->Accept(collector);
+    op.rhs()->Accept(collector);
+    CHECK(!ContainsForbidden(collector.symbols_));
+    return true;
+  }
+
+  bool PostVisit(SetProperties &op) override {
+    CHECK(!FindForbidden(op.input_symbol()));
+    UsedSymbolsCollector collector(*symbol_table_);
+    op.rhs()->Accept(collector);
+    CHECK(!ContainsForbidden(collector.symbols_));
+    return true;
+  }
+
+  bool PostVisit(SetLabels &op) override {
+    CHECK(!FindForbidden(op.input_symbol()));
+    return true;
+  }
+
+  bool PostVisit(RemoveProperty &op) override {
+    UsedSymbolsCollector collector(*symbol_table_);
+    op.lhs()->Accept(collector);
+    CHECK(!ContainsForbidden(collector.symbols_));
+    return true;
+  }
+
+  bool PostVisit(RemoveLabels &op) override {
+    CHECK(!FindForbidden(op.input_symbol()));
+    return true;
+  }
+
+  bool PostVisit(Aggregate &aggr) override {
+    CHECK(!ContainsForbidden(aggr.remember()));
+    for (auto &elem : aggr.aggregations()) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      if (elem.value) elem.value->Accept(collector);
+      if (elem.key) elem.key->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    for (auto *expr : aggr.group_by()) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      expr->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    return true;
+  }
+
+  bool PostVisit(Skip &skip) override {
+    UsedSymbolsCollector collector(*symbol_table_);
+    skip.expression()->Accept(collector);
+    CHECK(!ContainsForbidden(collector.symbols_));
+    return true;
+  }
+
+  bool PostVisit(Limit &limit) override {
+    UsedSymbolsCollector collector(*symbol_table_);
+    limit.expression()->Accept(collector);
+    CHECK(!ContainsForbidden(collector.symbols_));
+    return true;
+  }
+
+  bool PostVisit(OrderBy &order_by) override {
+    CHECK(!ContainsForbidden(order_by.output_symbols()));
+    for (auto *expr : order_by.order_by()) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      expr->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    return true;
+  }
+
+  bool PostVisit(Distinct &distinct) override {
+    CHECK(!ContainsForbidden(distinct.value_symbols()));
+    return true;
+  }
+
+  bool PostVisit(Cartesian &cart) override {
+    CHECK(!ContainsForbidden(cart.left_symbols()) &&
+          !ContainsForbidden(cart.right_symbols()));
+    return true;
+  }
+
+  bool PostVisit(Synchronize &op) override { return true; }
+
+  bool PostVisit(PullRemote &pull) override {
+    CHECK(!ContainsForbidden(pull.symbols()));
+    return true;
+  }
+
+  bool PostVisit(PullRemoteOrderBy &pull) override {
+    CHECK(!ContainsForbidden(pull.symbols()));
+    for (auto *expr : pull.order_by()) {
+      UsedSymbolsCollector collector(*symbol_table_);
+      expr->Accept(collector);
+      CHECK(!ContainsForbidden(collector.symbols_));
+    }
+    return true;
+  }
+
   // Independent subtree
   std::shared_ptr<LogicalOperator> subtree_;
   // Immediate parent of `subtree_`.
@@ -231,6 +388,10 @@ class IndependentSubtreeFinder : public HierarchicalLogicalOperatorVisitor {
     throw utils::NotYetImplemented("distributed Cartesian planning");
   }
 
+ private:
+  std::vector<std::vector<Symbol>> forbidden_symbols_;
+  const SymbolTable *symbol_table_;
+
   template <class TCollection>
   std::experimental::optional<int64_t> ContainsForbidden(
       const TCollection &symbols) {
@@ -252,10 +413,6 @@ class IndependentSubtreeFinder : public HierarchicalLogicalOperatorVisitor {
     }
     return std::experimental::nullopt;
   }
-
- private:
-  std::vector<std::vector<Symbol>> forbidden_symbols_;
-  const SymbolTable *symbol_table_;
 };
 
 // Find the subtree parent, below which no operator uses symbols found in the
@@ -788,13 +945,7 @@ class DistributedPlanner : public HierarchicalLogicalOperatorVisitor {
       // TODO: It might be better to plan Cartesians later if this Produce isn't
       // the last one and is not followed by an operator which requires a merge
       // point (Skip, OrderBy, etc.).
-      if (!on_master_) {
+      Split(produce, PlanCartesian(produce.input()));
-        Split(produce, PlanCartesian(produce.input()));
-      } else {
-        // We are on master, so our produce input must come on the left hand
-        // side.
-        throw utils::NotYetImplemented("distributed planning");
-      }
     }
     return true;
   }
@@ -992,6 +1143,11 @@ class DistributedPlanner : public HierarchicalLogicalOperatorVisitor {
       // which contains that dependency.
       branch.parent_start = input;
     }
+    if (on_master_ && cartesian_branches_.empty()) {
+      // Since we are planning a new Cartesian, the first CartesianBranch must
+      // not be sent to workers if we are executing on master.
+      return branch;
+    }
     // Send the independent subtree to workers and wire it in PullRemote
     auto id = AddWorkerPlan(branch.subtree);
     branch.subtree = std::make_shared<PullRemote>(
@@ -1018,7 +1174,6 @@ class DistributedPlanner : public HierarchicalLogicalOperatorVisitor {
   // Sets the master_op input to be merge_op. Afterwards, on_master_ is true.
   template <class TOp>
   void Split(TOp &master_op, std::shared_ptr<LogicalOperator> merge_op) {
-    if (on_master_) throw utils::NotYetImplemented("distributed planning");
     on_master_ = true;
     master_op.set_input(merge_op);
   }

src/query/plan/operator.cpp

@@ -3142,7 +3142,9 @@ std::vector<Symbol> Synchronize::ModifiedSymbols(
 
 bool Synchronize::Accept(HierarchicalLogicalOperatorVisitor &visitor) {
   if (visitor.PreVisit(*this)) {
-    input_->Accept(visitor) && pull_remote_->Accept(visitor);
+    // pull_remote_ is optional here, so visit it only if we continue visiting
+    // and pull_remote_ does exist.
+    input_->Accept(visitor) && pull_remote_ && pull_remote_->Accept(visitor);
   }
   return visitor.PostVisit(*this);
 }

src/query/plan/operator.lcp

@@ -365,7 +365,7 @@ and false on every following Pull.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (node-atom "NodeAtom *" :initval "nullptr"
+   (node-atom "NodeAtom *" :initval "nullptr" :reader t
               :capnp-type "Ast.Tree" :capnp-init nil
               :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "NodeAtom *")
               :save-fun #'save-pointer :load-fun #'load-pointer)
@@ -426,10 +426,12 @@ a preceeding `MATCH`), or multiple nodes (`MATCH ... CREATE` or
 (lcp:define-class create-expand (logical-operator)
   (
    ;; info on what's getting expanded
-   (node-atom "NodeAtom *" :save-fun #'save-pointer :load-fun #'load-pointer
+   (node-atom "NodeAtom *" :reader t
+              :save-fun #'save-pointer :load-fun #'load-pointer
               :capnp-type "Ast.Tree" :capnp-init nil
               :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "NodeAtom *"))
-   (edge-atom "EdgeAtom *" :save-fun #'save-pointer :load-fun #'load-pointer
+   (edge-atom "EdgeAtom *" :reader t
+              :save-fun #'save-pointer :load-fun #'load-pointer
               :capnp-type "Ast.Tree" :capnp-init nil
               :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "EdgeAtom *"))
    ;; the input op and the symbol under which the op's result
@@ -437,7 +439,7 @@ a preceeding `MATCH`), or multiple nodes (`MATCH ... CREATE` or
    (input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (input-symbol "Symbol")
+   (input-symbol "Symbol" :reader t)
    (existing-node :bool :documentation
                   "if the given node atom refers to an existing node (either matched or created)"))
   (:documentation
@@ -1157,7 +1159,7 @@ RETURN clause) the Produce's pull succeeds exactly once.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (expressions "std::vector<Expression *>"
+   (expressions "std::vector<Expression *>" :reader t
                 :capnp-type "List(Ast.Tree)"
                 :capnp-save (save-ast-vector "Expression *")
                 :capnp-load (load-ast-vector "Expression *")
@@ -1206,11 +1208,11 @@ Has a flag for using DETACH DELETE when deleting vertices.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (lhs "PropertyLookup *"
+   (lhs "PropertyLookup *" :reader t
         :capnp-type "Ast.Tree" :capnp-init nil
         :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "PropertyLookup *")
         :save-fun #'save-pointer :load-fun #'load-pointer)
-   (rhs "Expression *"
+   (rhs "Expression *" :reader t
         :capnp-type "Ast.Tree" :capnp-init nil
         :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "Expression *")
         :save-fun #'save-pointer :load-fun #'load-pointer))
@@ -1256,8 +1258,8 @@ can be stored (a TypedValue that can be converted to PropertyValue).")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (input-symbol "Symbol")
+   (input-symbol "Symbol" :reader t)
-   (rhs "Expression *"
+   (rhs "Expression *" :reader t
         :capnp-type "Ast.Tree" :capnp-init nil
         :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "Expression *")
         :save-fun #'save-pointer :load-fun #'load-pointer)
@@ -1327,7 +1329,7 @@ that the old props are discarded and replaced with new ones.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (input-symbol "Symbol")
+   (input-symbol "Symbol" :reader t)
    (labels "std::vector<storage::Label>"
      :capnp-save (lcp:capnp-save-vector "::storage::capnp::Common" "storage::Label")
      :capnp-load (lcp:capnp-load-vector "::storage::capnp::Common" "storage::Label")))
@@ -1371,7 +1373,7 @@ It does NOT remove labels that are already set on that Vertex.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (lhs "PropertyLookup *"
+   (lhs "PropertyLookup *" :reader t
         :capnp-type "Ast.Tree" :capnp-init nil
         :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "PropertyLookup *")
         :save-fun #'save-pointer :load-fun #'load-pointer))
@@ -1415,7 +1417,7 @@ It does NOT remove labels that are already set on that Vertex.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (input-symbol "Symbol")
+   (input-symbol "Symbol" :reader t)
    (labels "std::vector<storage::Label>"
      :capnp-save (lcp:capnp-save-vector "::storage::capnp::Common" "storage::Label")
      :capnp-load (lcp:capnp-load-vector "::storage::capnp::Common" "storage::Label")))
@@ -1763,7 +1765,7 @@ elements are in an undefined state after aggregation.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (expression "Expression *"
+   (expression "Expression *" :reader t
                :capnp-type "Ast.Tree" :capnp-init nil
                :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "Expression *")
                :save-fun #'save-pointer :load-fun #'load-pointer))
@@ -1820,7 +1822,7 @@ operator's implementation does not expect this.")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (expression "Expression *"
+   (expression "Expression *" :reader t
                :capnp-type "Ast.Tree" :capnp-init nil
                :capnp-save #'save-ast-pointer :capnp-load (load-ast-pointer "Expression *")
                :save-fun #'save-pointer :load-fun #'load-pointer))
@@ -2124,7 +2126,7 @@ Input is optional (unwind can be the first clause in a query).")
   ((input "std::shared_ptr<LogicalOperator>"
           :capnp-save #'save-operator-pointer
           :capnp-load #'load-operator-pointer)
-   (value-symbols "std::vector<Symbol>"
+   (value-symbols "std::vector<Symbol>" :reader t
                   :capnp-save (lcp:capnp-save-vector "::query::capnp::Symbol" "Symbol")
                   :capnp-load (lcp:capnp-load-vector "::query::capnp::Symbol" "Symbol")))
   (:documentation

tests/manual/query_planner.cpp

@@ -550,7 +550,17 @@ class PlanPrinter : public query::plan::HierarchicalLogicalOperatorVisitor {
   }
 
   bool PreVisit(query::plan::Cartesian &op) override {
-    WithPrintLn([](auto &out) { out << "* Cartesian"; });
+    WithPrintLn([&op](auto &out) {
+      out << "* Cartesian {";
+      utils::PrintIterable(
+          out, op.left_symbols(), ", ",
+          [](auto &out, const auto &sym) { out << sym.name(); });
+      out << " : ";
+      utils::PrintIterable(
+          out, op.right_symbols(), ", ",
+          [](auto &out, const auto &sym) { out << sym.name(); });
+      out << "}";
+    });
     Branch(*op.right_op());
     op.left_op()->Accept(*this);
     return false;

tests/unit/query_common.hpp

@@ -1,12 +1,13 @@
-///
 /// @file
 /// This file provides macros for easier construction of openCypher query AST.
 /// The usage of macros is very similar to how one would write openCypher. For
 /// example:
 ///
 ///     AstStorage storage;  // Macros rely on storage being in scope.
-///     // PROPERTY_LOOKUP and PROPERTY_PAIR macros rely on database::SingleNode
+///     // PROPERTY_LOOKUP and PROPERTY_PAIR macros
+///     // rely on a DbAccessor named dba.
 ///     database::SingleNode db;
+///     database::GraphDbAccessor dba(db);
 ///
 ///     QUERY(MATCH(PATTERN(NODE("n"), EDGE("e"), NODE("m"))),
 ///           WHERE(LESS(PROPERTY_LOOKUP("e", edge_prop), LITERAL(3))),
@@ -18,7 +19,6 @@
 /// resolution and template magic to provide a type safe way of constructing
 /// queries. Although the functions can be used by themselves, it is more
 /// convenient to use the macros.
-///
 
 #pragma once
 
@@ -28,10 +28,7 @@
 #include <utility>
 #include <vector>
 
-#include "database/graph_db.hpp"
-#include "database/graph_db_accessor.hpp"
 #include "query/frontend/ast/ast.hpp"
-#include "query/interpret/awesome_memgraph_functions.hpp"
 #include "storage/types.hpp"
 #include "utils/string.hpp"
 
@@ -91,12 +88,10 @@ auto FillOrderBy(OrderBy &order_by, Expression *expression, T... rest) {
   FillOrderBy(order_by, rest...);
 }
 
-///
 /// Create OrderBy expressions.
 ///
 /// The supported combination of arguments is: (Expression, [Ordering])+
 /// Since the Ordering is optional, by default it is ascending.
-///
 template <class... T>
 auto GetOrderBy(T... exprs) {
   OrderBy order_by;
@@ -104,41 +99,42 @@ auto GetOrderBy(T... exprs) {
   return order_by;
 }
 
-///
 /// Create PropertyLookup with given name and property.
 ///
 /// Name is used to create the Identifier which is used for property lookup.
-///
+template <class TDbAccessor>
-auto GetPropertyLookup(AstStorage &storage, database::GraphDb &db,
+auto GetPropertyLookup(AstStorage &storage, TDbAccessor &dba,
                        const std::string &name, storage::Property property) {
-  database::GraphDbAccessor dba(db);
   return storage.Create<PropertyLookup>(storage.Create<Identifier>(name),
                                         dba.PropertyName(property), property);
 }
-auto GetPropertyLookup(AstStorage &storage, database::GraphDb &db,
+
-                       Expression *expr, storage::Property property) {
+template <class TDbAccessor>
-  database::GraphDbAccessor dba(db);
+auto GetPropertyLookup(AstStorage &storage, TDbAccessor &dba, Expression *expr,
+                       storage::Property property) {
   return storage.Create<PropertyLookup>(expr, dba.PropertyName(property),
                                         property);
 }
+
+template <class TDbAccessor>
 auto GetPropertyLookup(
-    AstStorage &storage, database::GraphDb &, const std::string &name,
+    AstStorage &storage, TDbAccessor &, const std::string &name,
     const std::pair<std::string, storage::Property> &prop_pair) {
   return storage.Create<PropertyLookup>(storage.Create<Identifier>(name),
                                         prop_pair.first, prop_pair.second);
 }
+
+template <class TDbAccessor>
 auto GetPropertyLookup(
-    AstStorage &storage, database::GraphDb &, Expression *expr,
+    AstStorage &storage, TDbAccessor &, Expression *expr,
     const std::pair<std::string, storage::Property> &prop_pair) {
   return storage.Create<PropertyLookup>(expr, prop_pair.first,
                                         prop_pair.second);
 }
 
-///
 /// Create an EdgeAtom with given name, direction and edge_type.
 ///
 /// Name is used to create the Identifier which is assigned to the edge.
-///
 auto GetEdge(AstStorage &storage, const std::string &name,
              EdgeAtom::Direction dir = EdgeAtom::Direction::BOTH,
              const std::vector<storage::EdgeType> &edge_types = {}) {
@@ -146,12 +142,10 @@ auto GetEdge(AstStorage &storage, const std::string &name,
                                   EdgeAtom::Type::SINGLE, dir, edge_types);
 }
 
-///
 /// Create a variable length expansion EdgeAtom with given name, direction and
 /// edge_type.
 ///
 /// Name is used to create the Identifier which is assigned to the edge.
-///
 auto GetEdgeVariable(AstStorage &storage, const std::string &name,
                      EdgeAtom::Direction dir = EdgeAtom::Direction::BOTH,
                      const std::vector<storage::EdgeType> &edge_types = {},
@@ -169,11 +163,9 @@ auto GetEdgeVariable(AstStorage &storage, const std::string &name,
   return r_val;
 }
 
-///
 /// Create a NodeAtom with given name and label.
 ///
 /// Name is used to create the Identifier which is assigned to the node.
-///
 auto GetNode(AstStorage &storage, const std::string &name,
              std::experimental::optional<storage::Label> label =
                  std::experimental::nullopt) {
@@ -182,9 +174,7 @@ auto GetNode(AstStorage &storage, const std::string &name,
   return node;
 }
 
-///
 /// Create a Pattern with given atoms.
-///
 auto GetPattern(AstStorage &storage, std::vector<PatternAtom *> atoms) {
   auto pattern = storage.Create<Pattern>();
   pattern->identifier_ =
@@ -193,9 +183,7 @@ auto GetPattern(AstStorage &storage, std::vector<PatternAtom *> atoms) {
   return pattern;
 }
 
-///
 /// Create a Pattern with given name and atoms.
-///
 auto GetPattern(AstStorage &storage, const std::string &name,
                 std::vector<PatternAtom *> atoms) {
   auto pattern = storage.Create<Pattern>();
@@ -204,11 +192,9 @@ auto GetPattern(AstStorage &storage, const std::string &name,
   return pattern;
 }
 
-///
 /// This function fills an AST node which with given patterns.
 ///
 /// The function is most commonly used to create Match and Create clauses.
-///
 template <class TWithPatterns>
 auto GetWithPatterns(TWithPatterns *with_patterns,
                      std::vector<Pattern *> patterns) {
@@ -217,9 +203,7 @@ auto GetWithPatterns(TWithPatterns *with_patterns,
   return with_patterns;
 }
 
-///
 /// Create a query with given clauses.
-///
 
 auto GetSingleQuery(SingleQuery *single_query, Clause *clause) {
   single_query->clauses_.emplace_back(clause);
@@ -351,7 +335,6 @@ void FillReturnBody(AstStorage &storage, ReturnBody &body,
   FillReturnBody(storage, body, rest...);
 }
 
-///
 /// Create the return clause with given expressions.
 ///
 /// The supported expression combination of arguments is:
@@ -373,7 +356,6 @@ auto GetReturn(AstStorage &storage, bool distinct, T... exprs) {
   return ret;
 }
 
-///
 /// Create the with clause with given expressions.
 ///
 /// The supported expression combination is the same as for @c GetReturn.
@@ -387,9 +369,7 @@ auto GetWith(AstStorage &storage, bool distinct, T... exprs) {
   return with;
 }
 
-///
 /// Create the UNWIND clause with given named expression.
-///
 auto GetUnwind(AstStorage &storage, NamedExpression *named_expr) {
   return storage.Create<query::Unwind>(named_expr);
 }
@@ -398,9 +378,7 @@ auto GetUnwind(AstStorage &storage, Expression *expr, NamedExpression *as) {
   return GetUnwind(storage, as);
 }
 
-///
 /// Create the delete clause with given named expressions.
-///
 auto GetDelete(AstStorage &storage, std::vector<Expression *> exprs,
                bool detach = false) {
   auto del = storage.Create<Delete>();
@@ -410,52 +388,40 @@ auto GetDelete(AstStorage &storage, std::vector<Expression *> exprs,
   return del;
 }
 
-///
 /// Create a set property clause for given property lookup and the right hand
 /// side expression.
-///
 auto GetSet(AstStorage &storage, PropertyLookup *prop_lookup,
             Expression *expr) {
   return storage.Create<SetProperty>(prop_lookup, expr);
 }
 
-///
 /// Create a set properties clause for given identifier name and the right hand
 /// side expression.
-///
 auto GetSet(AstStorage &storage, const std::string &name, Expression *expr,
             bool update = false) {
   return storage.Create<SetProperties>(storage.Create<Identifier>(name), expr,
                                        update);
 }
 
-///
 /// Create a set labels clause for given identifier name and labels.
-///
 auto GetSet(AstStorage &storage, const std::string &name,
             std::vector<storage::Label> labels) {
   return storage.Create<SetLabels>(storage.Create<Identifier>(name), labels);
 }
 
-///
 /// Create a remove property clause for given property lookup
-///
 auto GetRemove(AstStorage &storage, PropertyLookup *prop_lookup) {
   return storage.Create<RemoveProperty>(prop_lookup);
 }
 
-///
 /// Create a remove labels clause for given identifier name and labels.
-///
 auto GetRemove(AstStorage &storage, const std::string &name,
                std::vector<storage::Label> labels) {
   return storage.Create<RemoveLabels>(storage.Create<Identifier>(name), labels);
 }
 
-///
 /// Create a Merge clause for given Pattern with optional OnMatch and OnCreate
 /// parts.
-///
 auto GetMerge(AstStorage &storage, Pattern *pattern,
               OnCreate on_create = OnCreate{}) {
   auto *merge = storage.Create<query::Merge>();
@@ -476,7 +442,6 @@ auto GetMerge(AstStorage &storage, Pattern *pattern, OnMatch on_match,
 
 }  // namespace query
 
-///
 /// All the following macros implicitly pass `storage` variable to functions.
 /// You need to have `AstStorage storage;` somewhere in scope to use them.
 /// Refer to function documentation to see what the macro does.
@@ -487,7 +452,6 @@ auto GetMerge(AstStorage &storage, Pattern *pattern, OnMatch on_match,
 ///   AstStorage storage;
 ///   auto query = QUERY(MATCH(PATTERN(NODE("n"), EDGE("r"), NODE("m"))),
 ///                      RETURN(NEXPR("new_name"), IDENT("m")));
-///
 #define NODE(...) query::test_common::GetNode(storage, __VA_ARGS__)
 #define EDGE(...) query::test_common::GetEdge(storage, __VA_ARGS__)
 #define EDGE_VARIABLE(...) \
@@ -515,10 +479,9 @@ auto GetMerge(AstStorage &storage, Pattern *pattern, OnMatch on_match,
       std::unordered_map<std::pair<std::string, storage::Property>, \
                          query::Expression *>{__VA_ARGS__})
 #define PROPERTY_PAIR(property_name) \
-  std::make_pair(property_name,      \
+  std::make_pair(property_name, dba.Property(property_name))
-                 database::GraphDbAccessor(db).Property(property_name))
 #define PROPERTY_LOOKUP(...) \
-  query::test_common::GetPropertyLookup(storage, db, __VA_ARGS__)
+  query::test_common::GetPropertyLookup(storage, dba, __VA_ARGS__)
 #define NEXPR(name, expr) storage.Create<query::NamedExpression>((name), (expr))
 // AS is alternative to NEXPR which does not initialize NamedExpression with
 // Expression. It should be used with RETURN or WITH. For example: