Fix expression rewrite (vesoft-inc#498)

<!--
Thanks for your contribution!
In order to review PR more efficiently, please add information according to the template.
-->

#### What type of PR is this?
- [x] bug
- [ ] feature
- [x] enhancement

#### What problem does this PR solve?
Issue(s) number: 
Fix vesoft-inc#3248

Description:
This PR fixes `filterTransform()` and adds cases.


#### How do you slove it?
1. Do not transfer the filter expression if it contains 2 different tagProp expressions i.e. `v.player.name =="abc" && v.player.age == 30`
2. Fix expression overflow check. Do not rewrite the expression if it would cause an overflow.
3. Refactor `rewriteRelExpr()`
4. Return semantic error when the non-addition arithmetic expression contains strings.



#### Special notes for your reviewer, ex. impact of this fix, design document, etc:

```
(root@nebula) [nba]> match (v:player) where v.player.name+'n'=="Tim Duncann" return v
+-----------------------------------------------------+
| v                                                   |
+-----------------------------------------------------+
| ("Tim Duncan" :player{age: 42, name: "Tim Duncan"}) |
+-----------------------------------------------------+
Got 1 rows (time spent 5509/5861 us)

Fri, 31 Dec 2021 22:51:01 CST

(root@nebula) [NBA]> match (v:player) where v.player.name-'n'=="Tim Duncann" return v
`(v.player.name-"n")' is not a valid expression, can not apply `-' to `__EMPTY__' and `STRING'.

Fri, 31 Dec 2021 22:51:08 CST
```

#### Checklist:
Tests:
- [x] Unit test(positive and negative cases)
- [ ] Function test
- [ ] Perfomance test
- [ ] N/A

Affects:
- [ ] Documentation affected (Please add the label if documentation needs to be modified.)
- [ ] Incompatibility (If it breaks the compatibility, please describe it and add the label.）
- [ ] If it's needed to cherry-pick (If cherry-pick to some branches is required, please label the destination version(s).)
- [ ] Performance impacted: Consumes more CPU/Memory


#### Release notes:

Please confirm whether to be reflected in release notes and how to describe:
> ex. Fixed the bug .....


Migrated from vesoft-inc#3614

Co-authored-by: Yichen Wang <[email protected]>

conf/nebula-graphd.conf.default

@@ -129,8 +129,9 @@
 # There are eight categories for tracking. There are: [ login ｜ exit | ddl | dql | dml | dcl | util | unknown ].
 --audit_log_categories=login,exit
 
+########## metrics ##########
+--enable_space_level_metrics=false
+
 ########## experimental feature ##########
 # if use experimental features
 --enable_experimental_feature=false
-
---enable_space_level_metrics=false

conf/nebula-graphd.conf.production

@@ -128,8 +128,9 @@
 # There are eight categories for tracking. There are: [ login | exit | ddl | dql | dml | dcl | util | unknown ].
 --audit_log_categories=login,exit
 
+########## metrics ##########
+--enable_space_level_metrics=false
+
 ########## experimental feature ##########
 # if use experimental features
 --enable_experimental_feature=false
-
---enable_space_level_metrics=false

src/graph/util/ExpressionUtils.cpp

@@ -7,8 +7,10 @@
 
 #include <memory>
 #include <queue>
+#include <unordered_set>
 
 #include "common/base/ObjectPool.h"
+#include "common/expression/ArithmeticExpression.h"
 #include "common/expression/Expression.h"
 #include "common/expression/PropertyExpression.h"
 #include "common/function/AggFunctionManager.h"
@@ -415,30 +417,69 @@ Expression *ExpressionUtils::reduceUnaryNotExpr(const Expression *expr) {
 
 Expression *ExpressionUtils::rewriteRelExpr(const Expression *expr) {
   ObjectPool *pool = expr->getObjPool();
-  // Match relational expressions containing at least one arithmetic expr
-  auto matcher = [](const Expression *e) -> bool {
-    if (e->isRelExpr()) {
-      auto relExpr = static_cast<const RelationalExpression *>(e);
-      if (isEvaluableExpr(relExpr->right())) {
-        return true;
+  QueryExpressionContext ctx(nullptr);
+
+  auto checkArithmExpr = [&ctx](const ArithmeticExpression *arithmExpr) -> bool {
+    auto lExpr = const_cast<Expression *>(arithmExpr->left());
+    auto rExpr = const_cast<Expression *>(arithmExpr->right());
+
+    // If the arithExpr has constant expr as member that is a string, do not rewrite
+    if (lExpr->kind() == Expression::Kind::kConstant) {
+      if (lExpr->eval(ctx).isStr()) {
+        return false;
       }
-      // TODO: To match arithmetic expression on both side
-      auto lExpr = relExpr->left();
-      if (lExpr->isArithmeticExpr()) {
-        auto arithmExpr = static_cast<const ArithmeticExpression *>(lExpr);
-        return isEvaluableExpr(arithmExpr->left()) || isEvaluableExpr(arithmExpr->right());
+    }
+    if (rExpr->kind() == Expression::Kind::kConstant) {
+      if (rExpr->eval(ctx).isStr()) {
+        return false;
       }
     }
-    return false;
+    return isEvaluableExpr(arithmExpr->left()) || isEvaluableExpr(arithmExpr->right());
+  };
+
+  // Match relational expressions following these rules:
+  // 1. the right operand of rel expr should be evaluable
+  // 2. the left operand of rel expr should be:
+  // 2.a an arithmetic expr that does not contains string and has at least one operand that is
+  // evaluable
+  // OR
+  // 2.b an relational expr so that it might could be simplified:
+  // ((v.age > 40 == true)  => (v.age > 40))
+  auto matcher = [&checkArithmExpr](const Expression *e) -> bool {
+    if (!e->isRelExpr()) {
+      return false;
+    }
+
+    auto relExpr = static_cast<const RelationalExpression *>(e);
+    // Check right operand
+    bool checkRightOperand = isEvaluableExpr(relExpr->right());
+    if (!checkRightOperand) {
+      return false;
+    }
+
+    // Check left operand
+    bool checkLeftOperand = false;
+    auto lExpr = relExpr->left();
+    // Left operand is arithmetical expr
+    if (lExpr->isArithmeticExpr()) {
+      auto arithmExpr = static_cast<const ArithmeticExpression *>(lExpr);
+      checkLeftOperand = checkArithmExpr(arithmExpr);
+    } else if (lExpr->isRelExpr() ||
+               lExpr->kind() == Expression::Kind::kLabelAttribute) {  // for expressions that
+                                                                      // contain boolean literals
+                                                                      // such as (v.age <= null)
+      checkLeftOperand = true;
+    }
+    return checkLeftOperand;
   };
 
   // Simplify relational expressions involving boolean literals
-  auto simplifyBoolOperand =
-      [pool](RelationalExpression *relExpr, Expression *lExpr, Expression *rExpr) -> Expression * {
-    QueryExpressionContext ctx(nullptr);
+  auto simplifyBoolOperand = [pool, &ctx](RelationalExpression *relExpr,
+                                          Expression *lExpr,
+                                          Expression *rExpr) -> Expression * {
     if (rExpr->kind() == Expression::Kind::kConstant) {
       auto conExpr = static_cast<ConstantExpression *>(rExpr);
-      auto val = conExpr->eval(ctx(nullptr));
+      auto val = conExpr->eval(ctx);
       auto valType = val.type();
       // Rewrite to null if the expression contains any operand that is null
       if (valType == Value::Type::NULLVALUE) {
@@ -521,21 +562,43 @@ Expression *ExpressionUtils::rewriteRelExprHelper(const Expression *expr,
     // case Expression::Kind::kMultiply:
     // case Expression::Kind::kDivision:
     default:
-      LOG(FATAL) << "Unsupported expression kind: " << static_cast<uint8_t>(kind);
+      DLOG(ERROR) << "Unsupported expression kind: " << static_cast<uint8_t>(kind);
       break;
   }
 
   return rewriteRelExprHelper(root, relRightOperandExpr);
 }
 
 StatusOr<Expression *> ExpressionUtils::filterTransform(const Expression *filter) {
-  auto rewrittenExpr = const_cast<Expression *>(filter);
+  // If the filter contains more than one different LabelAttribute expr, this filter cannot be
+  // pushed down
+  auto propExprs = ExpressionUtils::collectAll(filter, {Expression::Kind::kLabelTagProperty});
+  // Deduplicate the list
+  std::unordered_set<std::string> dedupPropExprSet;
+  for (auto &iter : propExprs) {
+    dedupPropExprSet.emplace(iter->toString());
+  }
+  if (dedupPropExprSet.size() > 1) {
+    return const_cast<Expression *>(filter);
+  }
+
+  // Check if any overflow happen before filter tranform
+  auto initialConstFold = foldConstantExpr(filter);
+  NG_RETURN_IF_ERROR(initialConstFold);
+
   // Rewrite relational expression
+  auto rewrittenExpr = initialConstFold.value();
   rewrittenExpr = rewriteRelExpr(rewrittenExpr);
+
   // Fold constant expression
   auto constantFoldRes = foldConstantExpr(rewrittenExpr);
-  NG_RETURN_IF_ERROR(constantFoldRes);
+  // If errors like overflow happened during the constant fold, stop transferming and return the
+  // original expression
+  if (!constantFoldRes.ok()) {
+    return const_cast<Expression *>(filter);
+  }
   rewrittenExpr = constantFoldRes.value();
+
   // Reduce Unary expression
   rewrittenExpr = reduceUnaryNotExpr(rewrittenExpr);
   return rewrittenExpr;
@@ -880,8 +943,9 @@ Expression::Kind ExpressionUtils::getNegatedArithmeticType(const Expression::Kin
       return Expression::Kind::kDivision;
     case Expression::Kind::kDivision:
       return Expression::Kind::kMultiply;
+    // There is no oppsite operation to Mod, return itself
     case Expression::Kind::kMod:
-      LOG(FATAL) << "Unsupported expression kind: " << static_cast<uint8_t>(kind);
+      return Expression::Kind::kMod;
       break;
     default:
       LOG(FATAL) << "Invalid arithmetic expression kind: " << static_cast<uint8_t>(kind);

src/graph/visitor/DeduceTypeVisitor.cpp

@@ -44,33 +44,40 @@ static const std::unordered_map<Value::Type, Value> kConstantValues = {
     {Value::Type::DURATION, Value(Duration())},
 };
 
-#define DETECT_BIEXPR_TYPE(OP)                                                             \
-  expr->left()->accept(this);                                                              \
-  if (!ok()) return;                                                                       \
-  auto left = type_;                                                                       \
-  expr->right()->accept(this);                                                             \
-  if (!ok()) return;                                                                       \
-  auto right = type_;                                                                      \
-  auto lhs = kConstantValues.find(left);                                                   \
-  if (lhs == kConstantValues.end()) {                                                      \
-    status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.", \
-                                    Value::toString(left).c_str());                        \
-    return;                                                                                \
-  }                                                                                        \
-  auto rhs = kConstantValues.find(right);                                                  \
-  if (rhs == kConstantValues.end()) {                                                      \
-    status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.", \
-                                    Value::toString(right).c_str());                       \
-    return;                                                                                \
-  }                                                                                        \
-  auto detectVal = lhs->second OP rhs->second;                                             \
-  if (detectVal.isBadNull()) {                                                             \
-    std::stringstream ss;                                                                  \
-    ss << "`" << expr->toString() << "' is not a valid expression, "                       \
-       << "can not apply `" << #OP << "' to `" << left << "' and `" << right << "'.";      \
-    status_ = Status::SemanticError(ss.str());                                             \
-    return;                                                                                \
-  }                                                                                        \
+#define DETECT_BIEXPR_TYPE(OP)                                                                  \
+  expr->left()->accept(this);                                                                   \
+  if (!ok()) return;                                                                            \
+  auto left = type_;                                                                            \
+  expr->right()->accept(this);                                                                  \
+  if (!ok()) return;                                                                            \
+  auto right = type_;                                                                           \
+  if (strcmp(#OP, "-") == 0 && (left == Value::Type::STRING || right == Value::Type::STRING)) { \
+    std::stringstream ss;                                                                       \
+    ss << "`" << expr->toString() << "' is not a valid expression, "                            \
+       << "can not apply `" << #OP << "' to `" << left << "' and `" << right << "'.";           \
+    status_ = Status::SemanticError(ss.str());                                                  \
+    return;                                                                                     \
+  }                                                                                             \
+  auto lhs = kConstantValues.find(left);                                                        \
+  if (lhs == kConstantValues.end()) {                                                           \
+    status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.",      \
+                                    Value::toString(left).c_str());                             \
+    return;                                                                                     \
+  }                                                                                             \
+  auto rhs = kConstantValues.find(right);                                                       \
+  if (rhs == kConstantValues.end()) {                                                           \
+    status_ = Status::SemanticError("Can't find constant value of `%s' when deduce type.",      \
+                                    Value::toString(right).c_str());                            \
+    return;                                                                                     \
+  }                                                                                             \
+  auto detectVal = lhs->second OP rhs->second;                                                  \
+  if (detectVal.isBadNull()) {                                                                  \
+    std::stringstream ss;                                                                       \
+    ss << "`" << expr->toString() << "' is not a valid expression, "                            \
+       << "can not apply `" << #OP << "' to `" << left << "' and `" << right << "'.";           \
+    status_ = Status::SemanticError(ss.str());                                                  \
+    return;                                                                                     \
+  }                                                                                             \
   type_ = detectVal.type()
 
 #define DETECT_NARYEXPR_TYPE(OP)                                                               \

src/graph/visitor/test/FilterTransformTest.cpp

@@ -23,27 +23,24 @@ TEST_F(FilterTransformTest, TestComplexExprRewrite) {
   ASSERT_EQ(*res.value(), *expected) << res.value()->toString() << " vs. " << expected->toString();
 }
 
+// If the expression tranformation causes an overflow, it should not be done.
 TEST_F(FilterTransformTest, TestCalculationOverflow) {
-  // (v.age - 1 < 9223372036854775807)  =>  overflow
+  // (v.age - 1 < 9223372036854775807)  =>  unchanged
   {
     auto expr =
         ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(9223372036854775807));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::SemanticError(
-        "result of (9223372036854775807+1) cannot be represented as an "
-        "integer");
-    ASSERT(!res.status().ok());
-    ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
+    ASSERT(res.ok());
+    auto expected = expr;
+    ASSERT_EQ(res.value(), expected) << res.value() << " vs. " << expected->toString();
   }
-  // (v.age + 1 < -9223372036854775808)  =>  overflow
+  // (v.age + 1 < -9223372036854775808)  =>  unchanged
   {
     auto expr = ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(INT64_MIN));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::SemanticError(
-        "result of (-9223372036854775808-1) cannot be represented as an "
-        "integer");
-    ASSERT(!res.status().ok());
-    ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
+    ASSERT(res.ok());
+    auto expected = expr;
+    ASSERT_EQ(res.value(), expected) << res.value() << " vs. " << expected->toString();
   }
   // (v.age - 1 < 9223372036854775807 + 1)  =>  overflow
   {
@@ -53,7 +50,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
     auto expected = Status::SemanticError(
         "result of (9223372036854775807+1) cannot be represented as an "
         "integer");
-    ASSERT(!res.status().ok());
+    ASSERT(!res.ok());
     ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
   }
   // (v.age + 1 < -9223372036854775808 - 1)  =>  overflow
@@ -64,30 +61,50 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
     auto expected = Status::SemanticError(
         "result of (-9223372036854775808-1) cannot be represented as an "
         "integer");
-    ASSERT(!res.status().ok());
+    ASSERT(!res.ok());
     ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
   }
-  // !!!(v.age - 1 < 9223372036854775807)  =>  overflow
+  // !!!(v.age - 1 < 9223372036854775807)  =>  unchanged
   {
     auto expr = notExpr(notExpr(notExpr(ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)),
                                                constantExpr(9223372036854775807)))));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::SemanticError(
-        "result of (9223372036854775807+1) cannot be represented as an "
-        "integer");
-    ASSERT(!res.status().ok());
-    ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
+    ASSERT(res.ok());
+    auto expected = expr;
+    ASSERT_EQ(res.value(), expected) << res.value()->toString() << " vs. " << expected->toString();
   }
-  // !!!(v.age + 1 < -9223372036854775808)  =>  overflow
+  // !!!(v.age + 1 < -9223372036854775808)  =>  unchanged
   {
     auto expr = notExpr(notExpr(
         notExpr(ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(INT64_MIN)))));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::SemanticError(
-        "result of (-9223372036854775808-1) cannot be represented as an "
-        "integer");
-    ASSERT(!res.status().ok());
-    ASSERT_EQ(res.status(), expected) << res.status().toString() << " vs. " << expected.toString();
+    ASSERT(res.ok());
+    auto expected = expr;
+    ASSERT_EQ(res.value(), expected) << res.value()->toString() << " vs. " << expected->toString();
+  }
+}
+
+TEST_F(FilterTransformTest, TestNoRewrite) {
+  // Do not rewrite if the filter contains more than one different LabelAttribute expr
+  {
+    // (v.age - 1 < v2.age + 2)  =>  Unchanged
+    auto expr = ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)),
+                       addExpr(laExpr("v2", "age"), constantExpr(40)));
+    auto res = ExpressionUtils::filterTransform(expr);
+    ASSERT(res.ok());
+    auto expected = expr;
+    ASSERT_EQ(res.value()->toString(), expected->toString())
+        << res.value()->toString() << " vs. " << expected->toString();
+  }
+  // Do not rewrite if the arithmetic expression contains string/char constant
+  {
+    // (v.name - "ab" < "name")  =>  Unchanged
+    auto expr = ltExpr(minusExpr(laExpr("v", "name"), constantExpr("ab")), constantExpr("name"));
+    auto res = ExpressionUtils::filterTransform(expr);
+    ASSERT(res.ok());
+    auto expected = expr;
+    ASSERT_EQ(res.value()->toString(), expected->toString())
+        << res.value()->toString() << " vs. " << expected->toString();
   }
 }
 

src/graph/visitor/test/RewriteRelExprVisitorTest.cpp

@@ -234,5 +234,23 @@ TEST_F(RewriteRelExprVisitorTest, TestContainer) {
   }
 }
 
+TEST_F(RewriteRelExprVisitorTest, TestArithmeticalExprWithStr) {
+  // Do not rewrite if the arithmetic expression contains string/char constant
+  {
+    // (v.name - "ab" < "name")  =>  Unchanged
+    auto expr = ltExpr(minusExpr(laExpr("v", "name"), constantExpr("ab")), constantExpr("name"));
+    auto res = ExpressionUtils::rewriteRelExpr(expr);
+    auto expected = expr;
+    ASSERT_EQ(*res, *expected) << res->toString() << " vs. " << expected->toString();
+  }
+  {
+    // (v.name + "ab" < "name")  =>  Unchanged
+    auto expr = ltExpr(addExpr(laExpr("v", "name"), constantExpr("ab")), constantExpr("name"));
+    auto res = ExpressionUtils::rewriteRelExpr(expr);
+    auto expected = expr;
+    ASSERT_EQ(*res, *expected) << res->toString() << " vs. " << expected->toString();
+  }
+}
+
 }  // namespace graph
 }  // namespace nebula