Merge branch 'master' into check-jobs-before-br

src/graph/executor/query/FilterExecutor.cpp

@@ -65,7 +65,10 @@ StatusOr<DataSet> FilterExecutor::handleJob(size_t begin, size_t end, Iterator *
   for (; iter->valid() && begin++ < end; iter->next()) {
     auto val = condition->eval(ctx(iter));
     if (val.isBadNull() || (!val.empty() && !val.isImplicitBool() && !val.isNull())) {
-      return Status::Error("Wrong type result, the type should be NULL, EMPTY, BOOL");
+      return Status::Error("Failed to evaluate condition: %s. %s%s",
+                           condition->toString().c_str(),
+                           "For boolean conditions, please write in their full forms like",
+                           " <condition> == <true/false> or <condition> IS [NOT] NULL.");
     }
     if (!(val.empty() || val.isNull() || (val.isImplicitBool() && !val.implicitBool()))) {
       // TODO: Maybe we can move.
@@ -96,7 +99,10 @@ Status FilterExecutor::handleSingleJobFilter() {
     while (iter->valid()) {
       auto val = condition->eval(ctx(iter));
       if (val.isBadNull() || (!val.empty() && !val.isImplicitBool() && !val.isNull())) {
-        return Status::Error("Wrong type result, the type should be NULL, EMPTY, BOOL");
+        return Status::Error("Failed to evaluate condition: %s. %s%s",
+                             condition->toString().c_str(),
+                             "For boolean conditions, please write in their full forms like",
+                             " <condition> == <true/false> or <condition> IS [NOT] NULL.");
       }
       if (val.empty() || val.isNull() || (val.isImplicitBool() && !val.implicitBool())) {
         if (UNLIKELY(filter->needStableFilter())) {
@@ -119,7 +125,10 @@ Status FilterExecutor::handleSingleJobFilter() {
     for (; iter->valid(); iter->next()) {
       auto val = condition->eval(ctx(iter));
       if (val.isBadNull() || (!val.empty() && !val.isImplicitBool() && !val.isNull())) {
-        return Status::Error("Wrong type result, the type should be NULL, EMPTY, BOOL");
+        return Status::Error("Failed to evaluate condition: %s. %s%s",
+                             condition->toString().c_str(),
+                             "For boolean conditions, please write in their full forms like",
+                             " <condition> == <true/false> or <condition> IS [NOT] NULL.");
       }
       if (val.isImplicitBool() && val.implicitBool()) {
         Row row;

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) {

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)

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

src/graph/validator/MaintainValidator.cpp

@@ -28,7 +28,8 @@ namespace graph {
 // Validate columns of schema.
 // Check validity of columns and fill to thrift structure.
 static Status validateColumns(const std::vector<ColumnSpecification *> &columnSpecs,
-                              meta::cpp2::Schema &schema) {
+                              meta::cpp2::Schema &schema,
+                              bool isAlter = false) {
   for (auto &spec : columnSpecs) {
     meta::cpp2::ColumnDef column;
     auto type = spec->type();
@@ -59,6 +60,12 @@ static Status validateColumns(const std::vector<ColumnSpecification *> &columnSp
     if (!column.nullable_ref().has_value()) {
       column.nullable_ref() = true;
     }
+    // Should report an error when altering the column
+    // which doesn't have default value to not nullable
+    if (isAlter && !column.nullable_ref().value() && !column.default_value_ref().has_value()) {
+      return Status::SemanticError("Column `%s' must have a default value if it's not nullable",
+                                   spec->name()->c_str());
+    }
     schema.columns_ref().value().emplace_back(std::move(column));
   }
   return Status::OK();
@@ -92,7 +99,7 @@ static StatusOr<std::vector<meta::cpp2::AlterSchemaItem>> validateSchemaOpts(
           return Status::SemanticError("Duplicate column name `%s'", spec->name()->c_str());
         }
       }
-      NG_LOG_AND_RETURN_IF_ERROR(validateColumns(specs, schema));
+      NG_LOG_AND_RETURN_IF_ERROR(validateColumns(specs, schema, true));
     }
 
     schemaItem.schema_ref() = std::move(schema);

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;
         }
       }
     }

src/meta/ActiveHostsMan.cpp

@@ -13,6 +13,7 @@
 DECLARE_int32(heartbeat_interval_secs);
 DEFINE_int32(agent_heartbeat_interval_secs, 60, "Agent heartbeat interval in seconds");
 DECLARE_uint32(expired_time_factor);
+DEFINE_bool(check_term_for_leader_info, false, "if check term when update leader info");
 
 namespace nebula {
 namespace meta {
@@ -45,14 +46,16 @@ nebula::cpp2::ErrorCode ActiveHostsMan::updateHostInfo(kvstore::KVStore* kv,
     TermID term = -1;
     nebula::cpp2::ErrorCode code;
     for (auto i = 0U; i != leaderKeys.size(); ++i) {
-      if (statusVec[i].ok()) {
-        std::tie(std::ignore, term, code) = MetaKeyUtils::parseLeaderValV3(vals[i]);
-        if (code != nebula::cpp2::ErrorCode::SUCCEEDED) {
-          LOG(INFO) << apache::thrift::util::enumNameSafe(code);
-          continue;
-        }
-        if (terms[i] <= term) {
-          continue;
+      if (FLAGS_check_term_for_leader_info) {
+        if (statusVec[i].ok()) {
+          std::tie(std::ignore, term, code) = MetaKeyUtils::parseLeaderValV3(vals[i]);
+          if (code != nebula::cpp2::ErrorCode::SUCCEEDED) {
+            LOG(INFO) << apache::thrift::util::enumNameSafe(code);
+            continue;
+          }
+          if (terms[i] <= term) {
+            continue;
+          }
         }
       }
       // write directly if not exist, or update if has greater term

tests/tck/features/match/Base.feature

@@ -632,7 +632,7 @@ Feature: Basic match
       """
       MATCH (v:player) where v.player.name return v
       """
-    Then a ExecutionError should be raised at runtime: Wrong type result, the type should be NULL, EMPTY, BOOL
+    Then a ExecutionError should be raised at runtime: Failed to evaluate condition: v.player.name. For boolean conditions, please write in their full forms like <condition> == <true/false> or <condition> IS [NOT] NULL.
 
   Scenario: Unimplemented features
     When executing query:

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: