Join filters with FilterAnd (short-circuit)

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

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

src/query/frontend/ast/ast.hpp

@@ -67,7 +67,6 @@ class OrOperator : public BinaryOperator {
   void Accept(TreeVisitorBase &visitor) override {
     if (visitor.PreVisit(*this)) {
       visitor.Visit(*this);
-      // TODO: Should we short-circuit?
       expression1_->Accept(visitor);
       expression2_->Accept(visitor);
       visitor.PostVisit(*this);
@@ -102,7 +101,27 @@ class AndOperator : public BinaryOperator {
   void Accept(TreeVisitorBase &visitor) override {
     if (visitor.PreVisit(*this)) {
       visitor.Visit(*this);
-      // TODO: Should we short-circuit?
+      expression1_->Accept(visitor);
+      expression2_->Accept(visitor);
+      visitor.PostVisit(*this);
+    }
+  }
+
+ protected:
+  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:
+  void Accept(TreeVisitorBase &visitor) override {
+    if (visitor.PreVisit(*this)) {
+      visitor.Visit(*this);
       expression1_->Accept(visitor);
       expression2_->Accept(visitor);
       visitor.PostVisit(*this);

src/query/frontend/ast/ast_visitor.hpp

@@ -25,6 +25,7 @@ class ListLiteral;
 class OrOperator;
 class XorOperator;
 class AndOperator;
+class FilterAndOperator;
 class NotOperator;
 class AdditionOperator;
 class SubtractionOperator;
@@ -54,15 +55,15 @@ class Merge;
 class Unwind;
 
 using TreeVisitorBase = ::utils::Visitor<
-    Query, NamedExpression, OrOperator, XorOperator, AndOperator, NotOperator,
-    AdditionOperator, SubtractionOperator, MultiplicationOperator,
-    DivisionOperator, ModOperator, NotEqualOperator, EqualOperator,
-    LessOperator, GreaterOperator, LessEqualOperator, GreaterEqualOperator,
-    InListOperator, ListIndexingOperator, ListSlicingOperator,
-    UnaryPlusOperator, UnaryMinusOperator, IsNullOperator, Identifier,
-    PrimitiveLiteral, ListLiteral, PropertyLookup, LabelsTest, EdgeTypeTest,
-    Aggregation, Function, Create, Match, Return, With, Pattern, NodeAtom,
-    EdgeAtom, Delete, Where, SetProperty, SetProperties, SetLabels,
+    Query, NamedExpression, OrOperator, XorOperator, AndOperator,
+    FilterAndOperator, NotOperator, AdditionOperator, SubtractionOperator,
+    MultiplicationOperator, DivisionOperator, ModOperator, NotEqualOperator,
+    EqualOperator, LessOperator, GreaterOperator, LessEqualOperator,
+    GreaterEqualOperator, InListOperator, ListIndexingOperator,
+    ListSlicingOperator, UnaryPlusOperator, UnaryMinusOperator, IsNullOperator,
+    Identifier, PrimitiveLiteral, ListLiteral, PropertyLookup, LabelsTest,
+    EdgeTypeTest, Aggregation, Function, Create, Match, Return, With, Pattern,
+    NodeAtom, EdgeAtom, Delete, Where, SetProperty, SetProperties, SetLabels,
     RemoveProperty, RemoveLabels, Merge, Unwind>;
 
 }  // namespace query

src/query/interpret/eval.hpp

@@ -89,6 +89,20 @@ class ExpressionEvaluator : public TreeVisitorBase {
 #undef BINARY_OPERATOR_VISITOR
 #undef UNARY_OPERATOR_VISITOR
 
+  bool PreVisit(FilterAndOperator &op) override {
+    op.expression1_->Accept(*this);
+    auto expression1 = PopBack();
+    if (expression1.IsNull() || !expression1.Value<bool>()) {
+      // If first expression is null or false, don't execute the second one.
+      result_stack_.emplace_back(expression1);
+      return false;
+    }
+    op.expression2_->Accept(*this);
+    auto expression2 = PopBack();
+    result_stack_.emplace_back(expression2);
+    return false;
+  }
+
   void PostVisit(InListOperator &) override {
     auto _list = PopBack();
     auto literal = PopBack();

src/query/plan/planner.cpp

@@ -147,7 +147,7 @@ Expression *PropertiesEqual(AstTreeStorage &storage,
         storage.Create<PropertyLookup>(atom->identifier_, prop_pair.first);
     auto *prop_equal =
         storage.Create<EqualOperator>(property_lookup, prop_pair.second);
-    filter_expr = BoolJoin<AndOperator>(storage, filter_expr, prop_equal);
+    filter_expr = BoolJoin<FilterAndOperator>(storage, filter_expr, prop_equal);
   }
   return filter_expr;
 }
@@ -166,7 +166,7 @@ auto &CollectPatternFilters(
     if (labels_filter || props_filter) {
       collector.symbols_.insert(symbol_table.at(*node->identifier_).position_);
       filters.emplace_back(
-          BoolJoin<AndOperator>(storage, labels_filter, props_filter),
+          BoolJoin<FilterAndOperator>(storage, labels_filter, props_filter),
           collector.symbols_);
       collector.symbols_.clear();
     }
@@ -183,7 +183,7 @@ auto &CollectPatternFilters(
       const auto &edge_symbol = symbol_table.at(*edge->identifier_);
       collector.symbols_.insert(edge_symbol.position_);
       filters->emplace_back(
-          BoolJoin<AndOperator>(storage, types_filter, props_filter),
+          BoolJoin<FilterAndOperator>(storage, types_filter, props_filter),
           collector.symbols_);
       collector.symbols_.clear();
     }
@@ -235,7 +235,7 @@ auto GenFilters(
   for (auto filters_it = filters.begin(); filters_it != filters.end();) {
     if (HasBoundFilterSymbols(bound_symbols, *filters_it)) {
       filter_expr =
-          BoolJoin<AndOperator>(storage, filter_expr, filters_it->first);
+          BoolJoin<FilterAndOperator>(storage, filter_expr, filters_it->first);
       filters_it = filters.erase(filters_it);
     } else {
       filters_it++;
@@ -456,7 +456,8 @@ class ReturnBodyContext : public TreeVisitorBase {
     // Aggregation contains a virtual symbol, where the result will be stored.
     const auto &symbol = symbol_table_.at(aggr);
     aggregations_.emplace_back(aggr.expression_, aggr.op_, symbol);
-    // aggregation expression_ is opional in COUNT(*) so it's possible the has_aggregation_ stack is empty
+    // aggregation expression_ is opional in COUNT(*) so it's possible the
+    // has_aggregation_ stack is empty
     if (aggr.expression_)
       has_aggregation_.back() = true;
     else

tests/unit/query_expression_evaluator.cpp

@@ -90,6 +90,32 @@ TEST(ExpressionEvaluator, AndOperator) {
   ASSERT_EQ(eval.eval.PopBack().Value<bool>(), false);
 }
 
+TEST(ExpressionEvaluator, FilterAndOperator) {
+  AstTreeStorage storage;
+  NoContextExpressionEvaluator eval;
+  {
+    auto *op = storage.Create<FilterAndOperator>(
+        storage.Create<PrimitiveLiteral>(true),
+        storage.Create<PrimitiveLiteral>(true));
+    op->Accept(eval.eval);
+    EXPECT_EQ(eval.eval.PopBack().Value<bool>(), true);
+  }
+  {
+    auto *op = storage.Create<FilterAndOperator>(
+        storage.Create<PrimitiveLiteral>(false),
+        storage.Create<PrimitiveLiteral>(5));
+    op->Accept(eval.eval);
+    EXPECT_EQ(eval.eval.PopBack().Value<bool>(), false);
+  }
+  {
+    auto *op = storage.Create<FilterAndOperator>(
+        storage.Create<PrimitiveLiteral>(TypedValue::Null),
+        storage.Create<PrimitiveLiteral>(5));
+    op->Accept(eval.eval);
+    EXPECT_TRUE(eval.eval.PopBack().IsNull());
+  }
+}
+
 TEST(ExpressionEvaluator, AdditionOperator) {
   AstTreeStorage storage;
   NoContextExpressionEvaluator eval;