Fix ExtractFilterExprVisitor. (#5422)

* Fix ExtractFilterExprVisitor.

* add tck.

* remove an DCHECK. it is not needed. although the binary operations that it is guarding are suppoed to work only on more than 1 operand, they can and they are, de facto, used to process expressions with only one operand.

src/common/expression/LogicalExpression.cpp

@@ -10,7 +10,6 @@
 namespace nebula {
 
 const Value &LogicalExpression::eval(ExpressionContext &ctx) {
-  DCHECK_GE(operands_.size(), 2UL);
   switch (kind()) {
     case Kind::kLogicalAnd:
       return evalAnd(ctx);

src/graph/visitor/ExtractFilterExprVisitor.cpp

@@ -240,12 +240,26 @@ void ExtractFilterExprVisitor::ExtractRemainExpr(LogicalExpression *expr,
     extractedExpr_ = operands[0];
   }
 
+  LogicalExpression *remainedExpr = nullptr;
   if (remainedOperands.size() > 1) {
-    auto remainedExpr = LogicalExpression::makeAnd(pool_);
+    remainedExpr = LogicalExpression::makeAnd(pool_);
     remainedExpr->setOperands(std::move(remainedOperands));
-    remainedExpr_ = std::move(remainedExpr);
   } else {
-    remainedExpr_ = std::move(remainedOperands[0]);
+    remainedExpr = static_cast<LogicalExpression *>(std::move(remainedOperands[0]));
+  }
+  if (remainedExpr_ != nullptr) {
+    if (remainedExprFromAnd_) {
+      auto mergeExpr = LogicalExpression::makeAnd(pool_, remainedExpr_, std::move(remainedExpr));
+      remainedExpr_ = std::move(mergeExpr);
+      remainedExprFromAnd_ = true;
+    } else {
+      auto mergeExpr = LogicalExpression::makeOr(pool_, remainedExpr_, std::move(remainedExpr));
+      remainedExpr_ = std::move(mergeExpr);
+      remainedExprFromAnd_ = true;
+    }
+  } else {
+    remainedExpr_ = std::move(remainedExpr);
+    remainedExprFromAnd_ = true;
   }
 }
 
@@ -375,6 +389,7 @@ void ExtractFilterExprVisitor::visit(LogicalExpression *expr) {
       DCHECK_EQ(expr->kind(), Expression::Kind::kLogicalAnd);
       DCHECK_EQ(expr->operands().size(), 2);
       remainedExpr_ = expr->operands()[1]->clone();
+      remainedExprFromAnd_ = false;
       expr->operands().pop_back();
     }
   } else {

src/graph/visitor/ExtractFilterExprVisitor.h

@@ -94,6 +94,7 @@ class ExtractFilterExprVisitor final : public ExprVisitorImpl {
   bool hasSplit{false};
   bool splitForbidden{false};
   Expression *remainedExpr_{nullptr};
+  bool remainedExprFromAnd_{false};
   Expression *extractedExpr_{nullptr};
   PushType pushType_{PushType::kGetNeighbors};
   std::vector<std::string> colNames_;

tests/tck/features/optimizer/PushFilterDownBugFixes.feature

@@ -0,0 +1,52 @@
+# Copyright (c) 2023 vesoft inc. All rights reserved.
+#
+# This source code is licensed under Apache 2.0 License.
+Feature: Bug fixes on filter push-down
+
+  Background:
+    Given a graph with space named "ngdata"
+
+  Scenario: missing predicate bug
+    When profiling query:
+      """
+      MATCH (v0)<-[e0]-()<-[e1]-(v1)
+      WHERE (id(v0) == 6)
+      AND ((NOT (NOT ((v1.Label_6.Label_6_5_Int > (e1.Rel_0_5_Int - 0.300177))
+      AND v1.Label_6.Label_6_1_Bool))))
+      RETURN count(*)
+      """
+    Then the result should be, in any order:
+      | count(*) |
+      | 0        |
+    And the execution plan should be:
+      | id | name           | dependencies | operator info |
+      | 9  | Aggregate      | 11           |               |
+      | 11 | Project        | 10           |               |
+      | 10 | Filter         | 6            |               |
+      | 6  | AppendVertices | 5            |               |
+      | 5  | Traverse       | 4            |               |
+      | 4  | Traverse       | 2            |               |
+      | 2  | Dedup          | 1            |               |
+      | 1  | PassThrough    | 3            |               |
+      | 3  | Start          |              |               |
+    When profiling query:
+      """
+      MATCH (v0)<-[e0]-()<-[e1]-(v1)
+      WHERE id(v0) == 6
+      AND v1.Label_6.Label_6_1_Bool
+      RETURN count(*)
+      """
+    Then the result should be, in any order:
+      | count(*) |
+      | 126      |
+    And the execution plan should be:
+      | id | name           | dependencies | operator info |
+      | 9  | Aggregate      | 11           |               |
+      | 11 | Project        | 10           |               |
+      | 10 | Filter         | 6            |               |
+      | 6  | AppendVertices | 5            |               |
+      | 5  | Traverse       | 4            |               |
+      | 4  | Traverse       | 2            |               |
+      | 2  | Dedup          | 1            |               |
+      | 1  | PassThrough    | 3            |               |
+      | 3  | Start          |              |               |