Split MATCH ... WHERE filtering on AND

Summary:
Test planner splits MATCH ... WHERE
Remove distinction between FilterAnd and AndOperator

Reviewers: florijan, mislav.bradac

Reviewed By: mislav.bradac

Subscribers: pullbot

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

src/query/frontend/ast/ast.hpp

@@ -192,27 +192,6 @@ class AndOperator : public BinaryOperator {
   using BinaryOperator::BinaryOperator;
 };
 
-// This is separate operator so that we can implement different short-circuiting
-// semantics than regular AndOperator. At this point CypherMainVisitor shouldn't
-// concern itself with this, and should constructor only AndOperator-s. This is
-// used in query planner at the moment.
-class FilterAndOperator : public BinaryOperator {
-  friend class AstTreeStorage;
-
- public:
-  DEFVISITABLE(TreeVisitor<TypedValue>);
-  bool Accept(HierarchicalTreeVisitor &visitor) override {
-    if (visitor.PreVisit(*this)) {
-      expression1_->Accept(visitor) && expression2_->Accept(visitor);
-    }
-    return visitor.PostVisit(*this);
-  }
-  CLONE_BINARY_EXPRESSION;
-
- protected:
-  using BinaryOperator::BinaryOperator;
-};
-
 class AdditionOperator : public BinaryOperator {
   friend class AstTreeStorage;
 

src/query/frontend/ast/ast_visitor.hpp

@@ -29,7 +29,6 @@ class MapLiteral;
 class OrOperator;
 class XorOperator;
 class AndOperator;
-class FilterAndOperator;
 class NotOperator;
 class AdditionOperator;
 class SubtractionOperator;
@@ -61,16 +60,16 @@ class Unwind;
 class CreateIndex;
 
 using TreeCompositeVisitor = ::utils::CompositeVisitor<
-    Query, NamedExpression, OrOperator, XorOperator, AndOperator,
-    FilterAndOperator, NotOperator, AdditionOperator, SubtractionOperator,
-    MultiplicationOperator, DivisionOperator, ModOperator, NotEqualOperator,
-    EqualOperator, LessOperator, GreaterOperator, LessEqualOperator,
-    GreaterEqualOperator, InListOperator, ListMapIndexingOperator,
-    ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator,
-    IsNullOperator, ListLiteral, MapLiteral, PropertyLookup, LabelsTest,
-    EdgeTypeTest, Aggregation, Function, All, Create, Match, Return, With,
-    Pattern, NodeAtom, EdgeAtom, BreadthFirstAtom, Delete, Where, SetProperty,
-    SetProperties, SetLabels, RemoveProperty, RemoveLabels, Merge, Unwind>;
+    Query, NamedExpression, OrOperator, XorOperator, AndOperator, NotOperator,
+    AdditionOperator, SubtractionOperator, MultiplicationOperator,
+    DivisionOperator, ModOperator, NotEqualOperator, EqualOperator,
+    LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator,
+    InListOperator, ListMapIndexingOperator, ListSlicingOperator, IfOperator,
+    UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral,
+    MapLiteral, PropertyLookup, LabelsTest, EdgeTypeTest, Aggregation, Function,
+    All, Create, Match, Return, With, Pattern, NodeAtom, EdgeAtom,
+    BreadthFirstAtom, Delete, Where, SetProperty, SetProperties, SetLabels,
+    RemoveProperty, RemoveLabels, Merge, Unwind>;
 
 using TreeLeafVisitor = ::utils::LeafVisitor<Identifier, PrimitiveLiteral,
                                              ParameterLookup, CreateIndex>;
@@ -78,24 +77,24 @@ using TreeLeafVisitor = ::utils::LeafVisitor<Identifier, PrimitiveLiteral,
 class HierarchicalTreeVisitor : public TreeCompositeVisitor,
                                 public TreeLeafVisitor {
  public:
-  using TreeCompositeVisitor::PreVisit;
   using TreeCompositeVisitor::PostVisit;
-  using typename TreeLeafVisitor::ReturnType;
+  using TreeCompositeVisitor::PreVisit;
   using TreeLeafVisitor::Visit;
+  using typename TreeLeafVisitor::ReturnType;
 };
 
 template <typename TResult>
 using TreeVisitor = ::utils::Visitor<
     TResult, Query, NamedExpression, OrOperator, XorOperator, AndOperator,
-    FilterAndOperator, NotOperator, AdditionOperator, SubtractionOperator,
-    MultiplicationOperator, DivisionOperator, ModOperator, NotEqualOperator,
-    EqualOperator, LessOperator, GreaterOperator, LessEqualOperator,
-    GreaterEqualOperator, InListOperator, ListMapIndexingOperator,
-    ListSlicingOperator, IfOperator, UnaryPlusOperator, UnaryMinusOperator,
-    IsNullOperator, ListLiteral, MapLiteral, PropertyLookup, LabelsTest,
-    EdgeTypeTest, Aggregation, Function, All, ParameterLookup, Create, Match,
-    Return, With, Pattern, NodeAtom, EdgeAtom, BreadthFirstAtom, Delete, Where,
-    SetProperty, SetProperties, SetLabels, RemoveProperty, RemoveLabels, Merge,
-    Unwind, Identifier, PrimitiveLiteral, CreateIndex>;
+    NotOperator, AdditionOperator, SubtractionOperator, MultiplicationOperator,
+    DivisionOperator, ModOperator, NotEqualOperator, EqualOperator,
+    LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator,
+    InListOperator, ListMapIndexingOperator, ListSlicingOperator, IfOperator,
+    UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, ListLiteral,
+    MapLiteral, PropertyLookup, LabelsTest, EdgeTypeTest, Aggregation, Function,
+    All, ParameterLookup, Create, Match, Return, With, Pattern, NodeAtom,
+    EdgeAtom, BreadthFirstAtom, Delete, Where, SetProperty, SetProperties,
+    SetLabels, RemoveProperty, RemoveLabels, Merge, Unwind, Identifier,
+    PrimitiveLiteral, CreateIndex>;
 
 }  // namespace query

src/query/interpret/eval.hpp

@@ -96,7 +96,6 @@ class ExpressionEvaluator : public TreeVisitor<TypedValue> {
 
   BINARY_OPERATOR_VISITOR(OrOperator, ||, OR);
   BINARY_OPERATOR_VISITOR(XorOperator, ^, XOR);
-  BINARY_OPERATOR_VISITOR(AndOperator, &&, AND);
   BINARY_OPERATOR_VISITOR(AdditionOperator, +, +);
   BINARY_OPERATOR_VISITOR(SubtractionOperator, -, -);
   BINARY_OPERATOR_VISITOR(MultiplicationOperator, *, *);
@@ -116,7 +115,7 @@ class ExpressionEvaluator : public TreeVisitor<TypedValue> {
 #undef BINARY_OPERATOR_VISITOR
 #undef UNARY_OPERATOR_VISITOR
 
-  TypedValue Visit(FilterAndOperator &op) override {
+  TypedValue Visit(AndOperator &op) override {
     auto value1 = op.expression1_->Accept(*this);
     if (value1.IsNull() || !value1.Value<bool>()) {
       // If first expression is null or false, don't execute the second one.

src/query/plan/rule_based_planner.cpp

@@ -3,6 +3,7 @@
 #include <algorithm>
 #include <functional>
 #include <limits>
+#include <stack>
 #include <unordered_set>
 
 #include "utils/algorithm.hpp"
@@ -331,7 +332,6 @@ class ReturnBodyContext : public HierarchicalTreeVisitor {
   VISIT_BINARY_OPERATOR(OrOperator)
   VISIT_BINARY_OPERATOR(XorOperator)
   VISIT_BINARY_OPERATOR(AndOperator)
-  VISIT_BINARY_OPERATOR(FilterAndOperator)
   VISIT_BINARY_OPERATOR(AdditionOperator)
   VISIT_BINARY_OPERATOR(SubtractionOperator)
   VISIT_BINARY_OPERATOR(MultiplicationOperator)
@@ -608,7 +608,7 @@ void AddMatching(const Match &match, SymbolTable &symbol_table,
 }
 
 // Iterates over `all_filters` joining them in one expression via
-// `FilterAndOperator`. Filters which use unbound symbols are skipped, as well
+// `AndOperator`. Filters which use unbound symbols are skipped, as well
 // as those that fail the `predicate` function. The function takes a single
 // argument, `FilterInfo`. All the joined filters are removed from
 // `all_filters`.
@@ -622,8 +622,8 @@ Expression *ExtractFilters(const std::unordered_set<Symbol> &bound_symbols,
        filters_it != all_filters.end();) {
     if (HasBoundFilterSymbols(bound_symbols, *filters_it) &&
         predicate(*filters_it)) {
-      filter_expr = impl::BoolJoin<FilterAndOperator>(storage, filter_expr,
-                                                      filters_it->expression);
+      filter_expr = impl::BoolJoin<AndOperator>(storage, filter_expr,
+                                                filters_it->expression);
       filters_it = all_filters.erase(filters_it);
     } else {
       filters_it++;
@@ -631,6 +631,24 @@ Expression *ExtractFilters(const std::unordered_set<Symbol> &bound_symbols,
   }
   return filter_expr;
 }
+
+auto SplitExpressionOnAnd(Expression *expression) {
+  std::vector<Expression *> expressions;
+  std::stack<Expression *> pending_expressions;
+  pending_expressions.push(expression);
+  while (!pending_expressions.empty()) {
+    auto *current_expression = pending_expressions.top();
+    pending_expressions.pop();
+    if (auto *and_op = dynamic_cast<AndOperator *>(current_expression)) {
+      pending_expressions.push(and_op->expression1_);
+      pending_expressions.push(and_op->expression2_);
+    } else {
+      expressions.push_back(current_expression);
+    }
+  }
+  return expressions;
+}
+
 }  // namespace
 
 namespace impl {
@@ -995,10 +1013,13 @@ void Filters::CollectPatternFilters(Pattern &pattern, SymbolTable &symbol_table,
 // information for potential property and label indexing.
 void Filters::CollectWhereFilter(Where &where,
                                  const SymbolTable &symbol_table) {
-  UsedSymbolsCollector collector(symbol_table);
-  where.expression_->Accept(collector);
-  all_filters_.emplace_back(FilterInfo{where.expression_, collector.symbols_});
-  AnalyzeFilter(where.expression_, symbol_table);
+  auto where_filters = SplitExpressionOnAnd(where.expression_);
+  for (const auto &filter : where_filters) {
+    UsedSymbolsCollector collector(symbol_table);
+    filter->Accept(collector);
+    all_filters_.emplace_back(FilterInfo{filter, collector.symbols_});
+    AnalyzeFilter(filter, symbol_table);
+  }
 }
 
 // Converts a Query to multiple QueryParts. In the process new Ast nodes may be

src/query/plan/rule_based_planner.hpp

@@ -548,9 +548,10 @@ class RuleBasedPlanner {
           const auto &next_node_symbol =
               symbol_table.at(*bf_atom->next_node_identifier_);
           // Inline BFS edge filtering together with its filter expression.
-          auto *filter_expr = impl::BoolJoin<FilterAndOperator>(
-              storage, impl::ExtractMultiExpandFilter(
-                           bound_symbols, node_symbol, all_filters, storage),
+          auto *filter_expr = impl::BoolJoin<AndOperator>(
+              storage,
+              impl::ExtractMultiExpandFilter(bound_symbols, node_symbol,
+                                             all_filters, storage),
               bf_atom->filter_expression_);
           last_op = new ExpandBreadthFirst(
               node_symbol, edge_symbol, expansion.direction, edge_type,

tests/unit/query_expression_evaluator.cpp

@@ -101,25 +101,18 @@ TEST(ExpressionEvaluator, AndOperator) {
   ASSERT_EQ(val2.Value<bool>(), false);
 }
 
-TEST(ExpressionEvaluator, FilterAndOperator) {
+TEST(ExpressionEvaluator, AndOperatorShortCircuit) {
   AstTreeStorage storage;
   NoContextExpressionEvaluator eval;
   {
-    auto *op = storage.Create<FilterAndOperator>(
-        storage.Create<PrimitiveLiteral>(true),
-        storage.Create<PrimitiveLiteral>(true));
-    auto value = op->Accept(eval.eval);
-    EXPECT_EQ(value.Value<bool>(), true);
-  }
-  {
-    auto *op = storage.Create<FilterAndOperator>(
-        storage.Create<PrimitiveLiteral>(false),
-        storage.Create<PrimitiveLiteral>(5));
+    auto *op =
+        storage.Create<AndOperator>(storage.Create<PrimitiveLiteral>(false),
+                                    storage.Create<PrimitiveLiteral>(5));
     auto value = op->Accept(eval.eval);
     EXPECT_EQ(value.Value<bool>(), false);
   }
   {
-    auto *op = storage.Create<FilterAndOperator>(
+    auto *op = storage.Create<AndOperator>(
         storage.Create<PrimitiveLiteral>(TypedValue::Null),
         storage.Create<PrimitiveLiteral>(5));
     auto value = op->Accept(eval.eval);

tests/unit/query_planner.cpp

@@ -1399,4 +1399,18 @@ TEST(TestLogicalPlanner, MatchScanToExpand) {
             ExpectFilter(), ExpectProduce());
 }
 
+TEST(TestLogicalPlanner, MatchWhereAndSplit) {
+  // Test MATCH (n) -[r]- (m) WHERE n.prop AND r.prop RETURN m
+  Dbms dbms;
+  auto dba = dbms.active();
+  auto prop = PROPERTY_PAIR("prop");
+  AstTreeStorage storage;
+  QUERY(MATCH(PATTERN(NODE("n"), EDGE("r"), NODE("m"))),
+        WHERE(AND(PROPERTY_LOOKUP("n", prop), PROPERTY_LOOKUP("r", prop))),
+        RETURN("m"));
+  // We expect `n.prop` filter right after scanning `n`.
+  CheckPlan(storage, ExpectScanAll(), ExpectFilter(), ExpectExpand(),
+            ExpectFilter(), ExpectProduce());
+}
+
 }  // namespace