enhance match attribute filter (#3272)

* enhance match attribute filter * add test case * fix test error * delete some null test * fix test error * fix storage crash
vesoft-inc · Nov 22, 2021 · b575189 · b575189
1 parent f449c17
commit b575189
Show file tree

Hide file tree

Showing 19 changed files with 281 additions and 128 deletions.
diff --git a/src/graph/validator/MatchValidator.cpp b/src/graph/validator/MatchValidator.cpp
@@ -112,8 +112,7 @@ Status MatchValidator::validateImpl() {
   return Status::OK();
 }
 
-Status MatchValidator::validatePath(const MatchPath *path,
-                                    MatchClauseContext &matchClauseCtx) const {
+Status MatchValidator::validatePath(const MatchPath *path, MatchClauseContext &matchClauseCtx) {
   NG_RETURN_IF_ERROR(
       buildNodeInfo(path, matchClauseCtx.nodeInfos, matchClauseCtx.aliasesGenerated));
   NG_RETURN_IF_ERROR(
@@ -122,8 +121,7 @@ Status MatchValidator::validatePath(const MatchPath *path,
   return Status::OK();
 }
 
-Status MatchValidator::buildPathExpr(const MatchPath *path,
-                                     MatchClauseContext &matchClauseCtx) const {
+Status MatchValidator::buildPathExpr(const MatchPath *path, MatchClauseContext &matchClauseCtx) {
   auto *pathAlias = path->alias();
   if (pathAlias == nullptr) {
     return Status::OK();
@@ -148,7 +146,7 @@ Status MatchValidator::buildPathExpr(const MatchPath *path,
 
 Status MatchValidator::buildNodeInfo(const MatchPath *path,
                                      std::vector<NodeInfo> &nodeInfos,
-                                     std::unordered_map<std::string, AliasType> &aliases) const {
+                                     std::unordered_map<std::string, AliasType> &aliases) {
   auto *sm = qctx_->schemaMng();
   auto steps = path->steps();
   auto *pool = qctx_->objPool();
@@ -182,7 +180,7 @@ Status MatchValidator::buildNodeInfo(const MatchPath *path,
     }
     Expression *filter = nullptr;
     if (props != nullptr) {
-      auto result = makeNodeSubFilter(props, "*");
+      auto result = makeNodeSubFilter(const_cast<MapExpression *>(props), "*");
       NG_RETURN_IF_ERROR(result);
       filter = result.value();
     } else if (node->labels() != nullptr && !node->labels()->labels().empty()) {
@@ -204,7 +202,7 @@ Status MatchValidator::buildNodeInfo(const MatchPath *path,
 
 Status MatchValidator::buildEdgeInfo(const MatchPath *path,
                                      std::vector<EdgeInfo> &edgeInfos,
-                                     std::unordered_map<std::string, AliasType> &aliases) const {
+                                     std::unordered_map<std::string, AliasType> &aliases) {
   auto *sm = qctx_->schemaMng();
   auto steps = path->steps();
   edgeInfos.resize(steps);
@@ -250,7 +248,7 @@ Status MatchValidator::buildEdgeInfo(const MatchPath *path,
     }
     Expression *filter = nullptr;
     if (props != nullptr) {
-      auto result = makeEdgeSubFilter(props);
+      auto result = makeEdgeSubFilter(const_cast<MapExpression *>(props));
       NG_RETURN_IF_ERROR(result);
       filter = result.value();
     }
@@ -521,32 +519,40 @@ Status MatchValidator::validateUnwind(const UnwindClause *unwindClause,
   return Status::OK();
 }
 
-StatusOr<Expression *> MatchValidator::makeEdgeSubFilter(const MapExpression *map) const {
+StatusOr<Expression *> MatchValidator::makeEdgeSubFilter(MapExpression *map) const {
   auto *pool = qctx_->objPool();
   DCHECK(map != nullptr);
   auto &items = map->items();
   DCHECK(!items.empty());
 
-  if (!ExpressionUtils::isEvaluableExpr(items[0].second)) {
-    return Status::SemanticError("Props must be constant: `%s'",
+  auto foldStatus = ExpressionUtils::foldConstantExpr(items[0].second);
+  NG_RETURN_IF_ERROR(foldStatus);
+  auto foldExpr = foldStatus.value();
+  if (!ExpressionUtils::isEvaluableExpr(foldExpr)) {
+    return Status::SemanticError("Props must be evaluable: `%s'",
                                  items[0].second->toString().c_str());
   }
+  map->setItem(0, std::make_pair(items[0].first, foldExpr));
   Expression *root = RelationalExpression::makeEQ(
-      pool, EdgePropertyExpression::make(pool, "*", items[0].first), items[0].second->clone());
+      pool, EdgePropertyExpression::make(pool, "*", items[0].first), foldExpr);
   for (auto i = 1u; i < items.size(); i++) {
-    if (!ExpressionUtils::isEvaluableExpr(items[i].second)) {
-      return Status::SemanticError("Props must be constant: `%s'",
+    foldStatus = ExpressionUtils::foldConstantExpr(items[i].second);
+    NG_RETURN_IF_ERROR(foldStatus);
+    foldExpr = foldStatus.value();
+    if (!ExpressionUtils::isEvaluableExpr(foldExpr)) {
+      return Status::SemanticError("Props must be evaluable: `%s'",
                                    items[i].second->toString().c_str());
     }
+    map->setItem(0, std::make_pair(items[i].first, foldExpr));
     auto *left = root;
     auto *right = RelationalExpression::makeEQ(
-        pool, EdgePropertyExpression::make(pool, "*", items[i].first), items[i].second->clone());
+        pool, EdgePropertyExpression::make(pool, "*", items[i].first), foldExpr);
     root = LogicalExpression::makeAnd(pool, left, right);
   }
   return root;
 }
 
-StatusOr<Expression *> MatchValidator::makeNodeSubFilter(const MapExpression *map,
+StatusOr<Expression *> MatchValidator::makeNodeSubFilter(MapExpression *map,
                                                          const std::string &label) const {
   auto *pool = qctx_->objPool();
   // Node has tag without property
@@ -562,20 +568,28 @@ StatusOr<Expression *> MatchValidator::makeNodeSubFilter(const MapExpression *ma
   auto &items = map->items();
   DCHECK(!items.empty());
 
-  if (!ExpressionUtils::isEvaluableExpr(items[0].second)) {
-    return Status::SemanticError("Props must be constant: `%s'",
+  auto foldStatus = ExpressionUtils::foldConstantExpr(items[0].second);
+  NG_RETURN_IF_ERROR(foldStatus);
+  auto foldExpr = foldStatus.value();
+  if (!ExpressionUtils::isEvaluableExpr(foldExpr)) {
+    return Status::SemanticError("Props must be evaluable: `%s'",
                                  items[0].second->toString().c_str());
   }
+  map->setItem(0, std::make_pair(items[0].first, foldExpr));
   Expression *root = RelationalExpression::makeEQ(
-      pool, TagPropertyExpression::make(pool, label, items[0].first), items[0].second->clone());
+      pool, TagPropertyExpression::make(pool, label, items[0].first), foldExpr);
   for (auto i = 1u; i < items.size(); i++) {
-    if (!ExpressionUtils::isEvaluableExpr(items[i].second)) {
-      return Status::SemanticError("Props must be constant: `%s'",
+    foldStatus = ExpressionUtils::foldConstantExpr(items[i].second);
+    NG_RETURN_IF_ERROR(foldStatus);
+    foldExpr = foldStatus.value();
+    if (!ExpressionUtils::isEvaluableExpr(foldExpr)) {
+      return Status::SemanticError("Props must be evaluable: `%s'",
                                    items[i].second->toString().c_str());
     }
+    map->setItem(i, std::make_pair(items[i].first, foldExpr));
     auto *left = root;
     auto *right = RelationalExpression::makeEQ(
-        pool, TagPropertyExpression::make(pool, label, items[i].first), items[i].second->clone());
+        pool, TagPropertyExpression::make(pool, label, items[i].first), foldExpr);
     root = LogicalExpression::makeAnd(pool, left, right);
   }
   return root;

diff --git a/src/graph/validator/MatchValidator.h b/src/graph/validator/MatchValidator.h
@@ -25,7 +25,7 @@ class MatchValidator final : public Validator {
 
   AstContext *getAstContext() override;
 
-  Status validatePath(const MatchPath *path, MatchClauseContext &matchClauseCtx) const;
+  Status validatePath(const MatchPath *path, MatchClauseContext &matchClauseCtx);
 
   Status validateFilter(const Expression *filter, WhereClauseContext &whereClauseCtx) const;
 
@@ -68,13 +68,13 @@ class MatchValidator final : public Validator {
 
   Status buildNodeInfo(const MatchPath *path,
                        std::vector<NodeInfo> &edgeInfos,
-                       std::unordered_map<std::string, AliasType> &aliases) const;
+                       std::unordered_map<std::string, AliasType> &aliases);
 
   Status buildEdgeInfo(const MatchPath *path,
                        std::vector<EdgeInfo> &nodeInfos,
-                       std::unordered_map<std::string, AliasType> &aliases) const;
+                       std::unordered_map<std::string, AliasType> &aliases);
 
-  Status buildPathExpr(const MatchPath *path, MatchClauseContext &matchClauseCtx) const;
+  Status buildPathExpr(const MatchPath *path, MatchClauseContext &matchClauseCtx);
 
   Status combineAliases(std::unordered_map<std::string, AliasType> &curAliases,
                         const std::unordered_map<std::string, AliasType> &lastAliases) const;
@@ -89,10 +89,9 @@ class MatchValidator final : public Validator {
 
   Status buildOutputs(const YieldColumns *yields);
 
-  StatusOr<Expression *> makeEdgeSubFilter(const MapExpression *map) const;
+  StatusOr<Expression *> makeEdgeSubFilter(MapExpression *map) const;
 
-  StatusOr<Expression *> makeNodeSubFilter(const MapExpression *map,
-                                           const std::string &label) const;
+  StatusOr<Expression *> makeNodeSubFilter(MapExpression *map, const std::string &label) const;
 
  private:
   std::unique_ptr<MatchAstContext> matchCtx_;

diff --git a/src/graph/visitor/FoldConstantExprVisitor.cpp b/src/graph/visitor/FoldConstantExprVisitor.cpp
@@ -346,17 +346,25 @@ Expression *FoldConstantExprVisitor::fold(Expression *expr) {
   auto value = expr->eval(ctx(nullptr));
   if (value.type() == Value::Type::NULLVALUE) {
     switch (value.getNull()) {
-      case NullType::DIV_BY_ZERO:
+      case NullType::DIV_BY_ZERO: {
         canBeFolded_ = false;
-        status_ = Status::Error("/ by zero");
+        status_ = Status::SemanticError("Divide by 0");
         break;
-      case NullType::ERR_OVERFLOW:
+      }
+      case NullType::ERR_OVERFLOW: {
+        canBeFolded_ = false;
+        status_ = Status::SemanticError("result of %s cannot be represented as an integer",
+                                        expr->toString().c_str());
+        break;
+      }
+      case NullType::BAD_TYPE: {
         canBeFolded_ = false;
-        status_ = Status::Error("result of %s cannot be represented as an integer",
-                                expr->toString().c_str());
+        status_ = Status::SemanticError("Type error `%s'", expr->toString().c_str());
         break;
-      default:
+      }
+      default: {
         break;
+      }
     }
   } else {
     status_ = Status::OK();

diff --git a/src/graph/visitor/test/FilterTransformTest.cpp b/src/graph/visitor/test/FilterTransformTest.cpp
@@ -29,7 +29,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
     auto expr =
         ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(9223372036854775807));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::Error(
+    auto expected = Status::SemanticError(
         "result of (9223372036854775807+1) cannot be represented as an "
         "integer");
     ASSERT(!res.status().ok());
@@ -39,7 +39,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
   {
     auto expr = ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(INT64_MIN));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::Error(
+    auto expected = Status::SemanticError(
         "result of (-9223372036854775808-1) cannot be represented as an "
         "integer");
     ASSERT(!res.status().ok());
@@ -50,7 +50,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
     auto expr = ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)),
                        addExpr(constantExpr(9223372036854775807), constantExpr(1)));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::Error(
+    auto expected = Status::SemanticError(
         "result of (9223372036854775807+1) cannot be represented as an "
         "integer");
     ASSERT(!res.status().ok());
@@ -61,7 +61,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
     auto expr = ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)),
                        minusExpr(constantExpr(INT64_MIN), constantExpr(1)));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::Error(
+    auto expected = Status::SemanticError(
         "result of (-9223372036854775808-1) cannot be represented as an "
         "integer");
     ASSERT(!res.status().ok());
@@ -72,7 +72,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
     auto expr = notExpr(notExpr(notExpr(ltExpr(minusExpr(laExpr("v", "age"), constantExpr(1)),
                                                constantExpr(9223372036854775807)))));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::Error(
+    auto expected = Status::SemanticError(
         "result of (9223372036854775807+1) cannot be represented as an "
         "integer");
     ASSERT(!res.status().ok());
@@ -83,7 +83,7 @@ TEST_F(FilterTransformTest, TestCalculationOverflow) {
     auto expr = notExpr(notExpr(
         notExpr(ltExpr(addExpr(laExpr("v", "age"), constantExpr(1)), constantExpr(INT64_MIN)))));
     auto res = ExpressionUtils::filterTransform(expr);
-    auto expected = Status::Error(
+    auto expected = Status::SemanticError(
         "result of (-9223372036854775808-1) cannot be represented as an "
         "integer");
     ASSERT(!res.status().ok());

diff --git a/src/parser/MatchSentence.h b/src/parser/MatchSentence.h
@@ -165,6 +165,8 @@ class MatchNode final {
 
   const MapExpression* props() const { return props_; }
 
+  MapExpression* props() { return props_; }
+
   std::string toString() const;
 
  private:

diff --git a/src/storage/exec/IndexScanNode.cpp b/src/storage/exec/IndexScanNode.cpp
@@ -61,20 +61,30 @@ std::string Path::encodeValue(const Value& value,
                               std::string& key) {
   std::string val;
   bool isNull = false;
-  if (colDef.get_type() == ::nebula::cpp2::PropertyType::GEOGRAPHY) {
-    CHECK_EQ(value.type(), Value::Type::STRING);
-    val = value.getStr();
-  } else if (value.type() == Value::Type::STRING) {
-    val = IndexKeyUtils::encodeValue(value, *colDef.get_type_length());
-    if (val.back() != '\0') {
-      strategySet_.insert(QualifiedStrategy::constant<QualifiedStrategy::UNCERTAIN>());
+  switch (colDef.get_type()) {
+    case ::nebula::cpp2::PropertyType::STRING:
+    case ::nebula::cpp2::PropertyType::FIXED_STRING: {
+      if (value.type() == Value::Type::NULLVALUE) {
+        val = IndexKeyUtils::encodeNullValue(Value::Type::STRING, colDef.get_type_length());
+        isNull = true;
+      } else {
+        val = IndexKeyUtils::encodeValue(value, *colDef.get_type_length());
+        if (val.back() != '\0') {
+          strategySet_.insert(QualifiedStrategy::constant<QualifiedStrategy::UNCERTAIN>());
+        }
+      }
+      break;
+    }
+    default: {
+      if (value.type() == Value::Type::NULLVALUE) {
+        auto vType = IndexKeyUtils::toValueType(colDef.get_type());
+        val = IndexKeyUtils::encodeNullValue(vType, colDef.get_type_length());
+        isNull = true;
+      } else {
+        val = IndexKeyUtils::encodeValue(value);
+      }
+      break;
     }
-  } else if (value.type() == Value::Type::NULLVALUE) {
-    auto vtype = IndexKeyUtils::toValueType(colDef.get_type());
-    val = IndexKeyUtils::encodeNullValue(vtype, colDef.get_type_length());
-    isNull = true;
-  } else {
-    val = IndexKeyUtils::encodeValue(value);
   }
   // If the current colDef can be null, then it is necessary to additionally determine whether the
   // corresponding value under a nullable is null when parsing the key (the encoding of the maximum

diff --git a/tests/tck/features/expression/EndsWith.feature b/tests/tck/features/expression/EndsWith.feature
@@ -60,9 +60,7 @@ Feature: Ends With Expression
       """
       YIELD 123 ENDS WITH 3
       """
-    Then the result should be, in any order:
-      | (123 ENDS WITH 3) |
-      | BAD_TYPE          |
+    Then a SemanticError should be raised at runtime: Type error `(123 ENDS WITH 3)'
 
   Scenario: yield not ends with
     When executing query:
@@ -118,9 +116,7 @@ Feature: Ends With Expression
       """
       YIELD 123 NOT ENDS WITH 3
       """
-    Then the result should be, in any order:
-      | (123 NOT ENDS WITH 3) |
-      | BAD_TYPE              |
+    Then a SemanticError should be raised at runtime: Type error `(123 NOT ENDS WITH 3)'
 
   Scenario: ends with go
     When executing query:

diff --git a/tests/tck/features/expression/Null.feature b/tests/tck/features/expression/Null.feature
@@ -24,11 +24,11 @@ Feature: NULL related operations
       | NULL   | NULL   | NULL   | NULL   | NULL   |
     When executing query:
       """
-      RETURN cbrt(NULL) AS value1, hypot(NULL, NULL) AS value2, pow(NULL, NULL) AS value3, exp(NULL) AS value4, exp2(NULL) AS value5
+      RETURN cbrt(NULL) AS value1, exp(NULL) AS value4, exp2(NULL) AS value5
       """
     Then the result should be, in any order:
-      | value1 | value2   | value3   | value4 | value5 |
-      | NULL   | BAD_TYPE | BAD_TYPE | NULL   | NULL   |
+      | value1 | value4 | value5 |
+      | NULL   | NULL   | NULL   |
     When executing query:
       """
       RETURN log(NULL) AS value1, log2(NULL) AS value2, log10(NULL) AS value3, sin(NULL) AS value4, asin(NULL) AS value5
@@ -38,18 +38,18 @@ Feature: NULL related operations
       | NULL   | NULL   | NULL   | NULL   | NULL   |
     When executing query:
       """
-      RETURN cos(NULL) AS value1, acos(NULL) AS value2, tan(NULL) AS value3, atan(NULL) AS value4, rand32(NULL) AS value5
+      RETURN cos(NULL) AS value1, acos(NULL) AS value2, tan(NULL) AS value3, atan(NULL) AS value4
       """
     Then the result should be, in any order:
-      | value1 | value2 | value3 | value4 | value5   |
-      | NULL   | NULL   | NULL   | NULL   | BAD_TYPE |
+      | value1 | value2 | value3 | value4 |
+      | NULL   | NULL   | NULL   | NULL   |
     When executing query:
       """
-      RETURN collect(NULL) AS value1, avg(NULL) AS value2, count(NULL) AS value3, max(NULL) AS value4, rand64(NULL,NULL) AS value5
+      RETURN collect(NULL) AS value1, avg(NULL) AS value2, count(NULL) AS value3, max(NULL) AS value4
       """
     Then the result should be, in any order:
-      | value1 | value2 | value3 | value4 | value5   |
-      | []     | NULL   | 0      | NULL   | BAD_TYPE |
+      | value1 | value2 | value3 | value4 |
+      | []     | NULL   | 0      | NULL   |
     When executing query:
       """
       RETURN min(NULL) AS value1, std(NULL) AS value2, sum(NULL) AS value3, bit_and(NULL) AS value4, bit_or(NULL,NULL) AS value5
@@ -59,8 +59,8 @@ Feature: NULL related operations
       | NULL   | NULL   | 0      | NULL   | NULL   |
     When executing query:
       """
-      RETURN bit_xor(NULL) AS value1, size(NULL) AS value2, range(NULL,NULL) AS value3, sign(NULL) AS value4, radians(NULL) AS value5
+      RETURN bit_xor(NULL) AS value1, size(NULL) AS value2, sign(NULL) AS value4, radians(NULL) AS value5
       """
     Then the result should be, in any order:
-      | value1 | value2 | value3   | value4 | value5 |
-      | NULL   | NULL   | BAD_TYPE | NULL   | NULL   |
+      | value1 | value2 | value4 | value5 |
+      | NULL   | NULL   | NULL   | NULL   |