fix wrong vid finder for logic or expr (vesoft-inc#1988)

* fix wrong vid finder * add tck cases Co-authored-by: jie.wang <[email protected]>
yixinglu · Dec 26, 2022 · 85cb6c8 · 85cb6c8
1 parent 6496480
commit 85cb6c8
Show file tree

Hide file tree

Showing 5 changed files with 192 additions and 18 deletions.
diff --git a/src/graph/util/ExpressionUtils.cpp b/src/graph/util/ExpressionUtils.cpp
@@ -536,7 +536,65 @@ StatusOr<Expression *> ExpressionUtils::foldConstantExpr(const Expression *expr)
     }
     return foldedExpr;
   }
-  return newExpr;
+
+  auto matcher = [](const Expression *e) {
+    return e->kind() == Expression::Kind::kLogicalAnd || e->kind() == Expression::Kind::kLogicalOr;
+  };
+  auto rewriter = [](const Expression *e) {
+    auto logicalExpr = static_cast<const LogicalExpression *>(e);
+    return simplifyLogicalExpr(logicalExpr);
+  };
+  return RewriteVisitor::transform(newExpr, matcher, rewriter);
+}
+
+Expression *ExpressionUtils::simplifyLogicalExpr(const LogicalExpression *logicalExpr) {
+  auto *expr = static_cast<LogicalExpression *>(logicalExpr->clone());
+  if (expr->kind() == Expression::Kind::kLogicalXor) return expr;
+
+  ObjectPool *objPool = logicalExpr->getObjPool();
+
+  // Simplify logical and/or
+  for (auto iter = expr->operands().begin(); iter != expr->operands().end();) {
+    auto *operand = *iter;
+    if (operand->kind() != Expression::Kind::kConstant) {
+      ++iter;
+      continue;
+    }
+    auto &val = static_cast<ConstantExpression *>(operand)->value();
+    if (!val.isBool()) {
+      ++iter;
+      continue;
+    }
+    if (expr->kind() == Expression::Kind::kLogicalAnd) {
+      if (val.getBool()) {
+        // Remove the true operand
+        iter = expr->operands().erase(iter);
+        continue;
+      }
+      // The whole expression is false
+      return ConstantExpression::make(objPool, false);
+    }
+    // expr->kind() == Expression::Kind::kLogicalOr
+    if (val.getBool()) {
+      // The whole expression is true
+      return ConstantExpression::make(objPool, true);
+    }
+    // Remove the false operand
+    iter = expr->operands().erase(iter);
+  }
+
+  if (expr->operands().empty()) {
+    // true and true and true => true
+    if (expr->kind() == Expression::Kind::kLogicalAnd) {
+      return ConstantExpression::make(objPool, true);
+    }
+    // false or false or false => false
+    return ConstantExpression::make(objPool, false);
+  } else if (expr->operands().size() == 1) {
+    return expr->operands()[0];
+  } else {
+    return expr;
+  }
 }
 
 Expression *ExpressionUtils::reduceUnaryNotExpr(const Expression *expr) {

diff --git a/src/graph/util/ExpressionUtils.h b/src/graph/util/ExpressionUtils.h
@@ -117,6 +117,13 @@ class ExpressionUtils {
   // v.age > 40 + 1  =>  v.age > 41
   static StatusOr<Expression*> foldConstantExpr(const Expression* expr);
 
+  // Simplify logical and/or expr.
+  // e.g. A and true => A
+  //      A or false => A
+  //      A and false => false
+  //      A or true => true
+  static Expression* simplifyLogicalExpr(const LogicalExpression* logicalExpr);
+
   // Clones and reduces unaryNot expression
   // Examples:
   // !!(A and B)  =>  (A and B)

diff --git a/src/graph/util/test/ExpressionUtilsTest.cpp b/src/graph/util/test/ExpressionUtilsTest.cpp
@@ -764,5 +764,68 @@ TEST_F(ExpressionUtilsTest, expandExpression) {
     ASSERT_EQ(expected, target->toString());
   }
 }
+
+TEST_F(ExpressionUtilsTest, simplifyLogicalExpr) {
+  {
+    auto filter = parse("A and true");
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(filter));
+    auto expected = "A";
+    ASSERT_EQ(expected, target->toString());
+  }
+  {
+    auto filter = parse("A and false");
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(filter));
+    auto expected = "false";
+    ASSERT_EQ(expected, target->toString());
+  }
+  {
+    auto filter = parse("A or true");
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(filter));
+    auto expected = "true";
+    ASSERT_EQ(expected, target->toString());
+  }
+  {
+    auto filter = parse("A or false");
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(filter));
+    auto expected = "A";
+    ASSERT_EQ(expected, target->toString());
+  }
+  {
+    auto filter = parse("A or 2 > 1");
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(filter));
+    auto expected = "(A OR (2>1))";
+    ASSERT_EQ(expected, target->toString());
+  }
+  {
+    auto filter = parse("true and true and true");
+    auto newFilter = ExpressionUtils::flattenInnerLogicalAndExpr(filter);
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(newFilter));
+    auto expected = "true";
+    ASSERT_EQ(expected, target->toString());
+  }
+  {
+    auto filter = parse("false or false or false");
+    auto newFilter = ExpressionUtils::flattenInnerLogicalOrExpr(filter);
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(newFilter));
+    auto expected = "false";
+    ASSERT_EQ(expected, target->toString());
+  }
+  {
+    auto filter = parse("(A and B) or (2 > 1) or (C and true) or (D or true)");
+    auto newFilter = ExpressionUtils::flattenInnerLogicalOrExpr(filter);
+    auto target =
+        ExpressionUtils::simplifyLogicalExpr(static_cast<const LogicalExpression *>(newFilter));
+    auto expected = "true";
+    ASSERT_EQ(expected, target->toString());
+  }
+}
+
 }  // namespace graph
 }  // namespace nebula
diff --git a/src/graph/visitor/VidExtractVisitor.cpp b/src/graph/visitor/VidExtractVisitor.cpp
@@ -309,28 +309,30 @@ void VidExtractVisitor::visit(LogicalExpression *expr) {
     operandsResult.reserve(expr->operands().size());
     for (const auto &operand : expr->operands()) {
       operand->accept(this);
+      if (vidPattern_.spec != VidPattern::Special::kInUsed) {
+        vidPattern_ = VidPattern{};
+        return;
+      }
       operandsResult.emplace_back(moveVidPattern());
     }
     VidPattern inResult{VidPattern::Special::kInUsed, {}};
     for (auto &result : operandsResult) {
-      if (result.spec == VidPattern::Special::kInUsed) {
-        for (auto &node : result.nodes) {
-          // Can't deduce with outher source (e.g. PropertiesIndex)
-          switch (node.second.kind) {
-            case VidPattern::Vids::Kind::kOtherSource:
-              vidPattern_ = VidPattern{};
-              return;
-            case VidPattern::Vids::Kind::kIn: {
-              inResult.nodes[node.first].kind = VidPattern::Vids::Kind::kIn;
-              inResult.nodes[node.first].vids.values.insert(
-                  inResult.nodes[node.first].vids.values.end(),
-                  std::make_move_iterator(node.second.vids.values.begin()),
-                  std::make_move_iterator(node.second.vids.values.end()));
-            }
-            case VidPattern::Vids::Kind::kNotIn:
-              // nothing
-              break;
+      for (auto &node : result.nodes) {
+        // Can't deduce with outher source (e.g. PropertiesIndex)
+        switch (node.second.kind) {
+          case VidPattern::Vids::Kind::kOtherSource:
+            vidPattern_ = VidPattern{};
+            return;
+          case VidPattern::Vids::Kind::kIn: {
+            inResult.nodes[node.first].kind = VidPattern::Vids::Kind::kIn;
+            inResult.nodes[node.first].vids.values.insert(
+                inResult.nodes[node.first].vids.values.end(),
+                std::make_move_iterator(node.second.vids.values.begin()),
+                std::make_move_iterator(node.second.vids.values.end()));
           }
+          case VidPattern::Vids::Kind::kNotIn:
+            // nothing
+            break;
         }
       }
     }

diff --git a/tests/tck/features/match/SeekById.feature b/tests/tck/features/match/SeekById.feature
@@ -25,6 +25,23 @@ Feature: Match seek by id
       | 'Jonathon Simmons' |
       | 'Klay Thompson'    |
       | 'Dejounte Murray'  |
+    # start vid finder don't support variable currently
+    When executing query:
+      """
+      WITH [1, 2, 3] AS coll
+      UNWIND coll AS vid
+      MATCH (v) WHERE id(v) == vid
+      RETURN v;
+      """
+    Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
+    When executing query:
+      """
+      WITH [1, 2, 3] AS coll
+      UNWIND coll AS vid
+      MATCH (v) WHERE id(v) == "Tony Parker" OR id(v) == vid
+      RETURN v;
+      """
+    Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
 
   Scenario: basic logical not
     When executing query:
@@ -128,6 +145,28 @@ Feature: Match seek by id
       | 'Jonathon Simmons' |
       | 'Klay Thompson'    |
       | 'Dejounte Murray'  |
+    When executing query:
+      """
+      MATCH (v)
+      WHERE id(v) IN ['James Harden', 'Jonathon Simmons', 'Klay Thompson', 'Dejounte Murray', 'Paul Gasol']
+            OR true
+      RETURN v.player.name AS Name
+      """
+    Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
+    When executing query:
+      """
+      MATCH (v)
+      WHERE id(v) IN ['James Harden', 'Jonathon Simmons', 'Klay Thompson', 'Dejounte Murray', 'Paul Gasol']
+            AND true
+      RETURN v.player.name AS Name
+      """
+    Then the result should be, in any order:
+      | Name               |
+      | 'Paul Gasol'       |
+      | 'James Harden'     |
+      | 'Jonathon Simmons' |
+      | 'Klay Thompson'    |
+      | 'Dejounte Murray'  |
     When executing query:
       """
       MATCH (v)
@@ -262,6 +301,11 @@ Feature: Match seek by id
       RETURN v.player.name AS Name
       """
     Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
+    When executing query:
+      """
+      MATCH (v) WHERE id(v) == "Tim Duncan" OR id(v) != "Tony Parker" RETURN COUNT(*) AS count
+      """
+    Then a ExecutionError should be raised at runtime: Scan vertices or edges need to specify a limit number, or limit number can not push down.
 
   Scenario: test OR logic
     When executing query: