check vidType when execute, not validate

vesoft-inc · Jul 23, 2022 · e2618be · e2618be
1 parent 08ce4dc
commit e2618be
Show file tree

Hide file tree

Showing 12 changed files with 56 additions and 36 deletions.
diff --git a/src/graph/executor/StorageAccessExecutor.cpp b/src/graph/executor/StorageAccessExecutor.cpp
@@ -37,12 +37,12 @@ struct Vid<std::string> {
 };
 
 template <typename VidType>
-DataSet buildRequestDataSet(const SpaceInfo &space,
-                            QueryExpressionContext &exprCtx,
-                            Iterator *iter,
-                            Expression *expr,
-                            bool dedup,
-                            bool isCypher) {
+StatusOr<DataSet> buildRequestDataSet(const SpaceInfo &space,
+                                      QueryExpressionContext &exprCtx,
+                                      Iterator *iter,
+                                      Expression *expr,
+                                      bool dedup,
+                                      bool isCypher) {
   DCHECK(iter && expr) << "iter=" << iter << ", expr=" << expr;
   nebula::DataSet vertices({kVid});
   auto s = iter->size();
@@ -62,9 +62,11 @@ DataSet buildRequestDataSet(const SpaceInfo &space,
       if (isCypher) {
         continue;
       }
-      LOG(WARNING) << "Mismatched vid type: " << vid.type()
-                   << ", space vid type: " << SchemaUtil::typeToString(vidType);
-      continue;
+      std::stringstream ss;
+      ss << "`" << vid.toString() << "', the srcs should be type of "
+         << apache::thrift::util::enumNameSafe(vidType.get_type()) << ", but was`" << vid.type()
+         << "'";
+      return Status::Error(ss.str());
     }
     if (dedup && !uniqueSet.emplace(Vid<VidType>::value(vid)).second) {
       continue;
@@ -80,10 +82,10 @@ bool StorageAccessExecutor::isIntVidType(const SpaceInfo &space) const {
   return (*space.spaceDesc.vid_type_ref()).type == nebula::cpp2::PropertyType::INT64;
 }
 
-DataSet StorageAccessExecutor::buildRequestDataSetByVidType(Iterator *iter,
-                                                            Expression *expr,
-                                                            bool dedup,
-                                                            bool isCypher) {
+StatusOr<DataSet> StorageAccessExecutor::buildRequestDataSetByVidType(Iterator *iter,
+                                                                      Expression *expr,
+                                                                      bool dedup,
+                                                                      bool isCypher) {
   const auto &space = qctx()->rctx()->session()->space();
   QueryExpressionContext exprCtx(qctx()->ectx());
 

diff --git a/src/graph/executor/StorageAccessExecutor.h b/src/graph/executor/StorageAccessExecutor.h
@@ -158,10 +158,10 @@ class StorageAccessExecutor : public Executor {
 
   bool isIntVidType(const SpaceInfo &space) const;
 
-  DataSet buildRequestDataSetByVidType(Iterator *iter,
-                                       Expression *expr,
-                                       bool dedup,
-                                       bool isCypher = false);
+  StatusOr<DataSet> buildRequestDataSetByVidType(Iterator *iter,
+                                                 Expression *expr,
+                                                 bool dedup,
+                                                 bool isCypher = false);
 };
 
 }  // namespace graph

diff --git a/src/graph/executor/query/AppendVerticesExecutor.cpp b/src/graph/executor/query/AppendVerticesExecutor.cpp
@@ -16,7 +16,7 @@ folly::Future<Status> AppendVerticesExecutor::execute() {
   return appendVertices();
 }
 
-DataSet AppendVerticesExecutor::buildRequestDataSet(const AppendVertices *av) {
+StatusOr<DataSet> AppendVerticesExecutor::buildRequestDataSet(const AppendVertices *av) {
   if (av == nullptr) {
     return nebula::DataSet({kVid});
   }
@@ -30,7 +30,9 @@ folly::Future<Status> AppendVerticesExecutor::appendVertices() {
   auto *av = asNode<AppendVertices>(node());
   StorageClient *storageClient = qctx()->getStorageClient();
 
-  DataSet vertices = buildRequestDataSet(av);
+  auto res = buildRequestDataSet(av);
+  NG_RETURN_IF_ERROR(res);
+  auto vertices = std::move(res).value();
   if (vertices.rows.empty()) {
     return finish(ResultBuilder().value(Value(DataSet(av->colNames()))).build());
   }

diff --git a/src/graph/executor/query/AppendVerticesExecutor.h b/src/graph/executor/query/AppendVerticesExecutor.h
@@ -22,7 +22,7 @@ class AppendVerticesExecutor final : public GetPropExecutor {
   folly::Future<Status> execute() override;
 
  private:
-  DataSet buildRequestDataSet(const AppendVertices *gv);
+  StatusOr<DataSet> buildRequestDataSet(const AppendVertices *gv);
 
   folly::Future<Status> appendVertices();
 

diff --git a/src/graph/executor/query/GetEdgesExecutor.cpp b/src/graph/executor/query/GetEdgesExecutor.cpp
@@ -18,20 +18,36 @@ folly::Future<Status> GetEdgesExecutor::execute() {
   return getEdges();
 }
 
-DataSet GetEdgesExecutor::buildRequestDataSet(const GetEdges *ge) {
+StatusOr<DataSet> GetEdgesExecutor::buildRequestDataSet(const GetEdges *ge) {
   auto valueIter = ectx_->getResult(ge->inputVar()).iter();
   QueryExpressionContext exprCtx(qctx()->ectx());
 
   nebula::DataSet edges({kSrc, kType, kRank, kDst});
   edges.rows.reserve(valueIter->size());
   std::unordered_set<std::tuple<Value, Value, Value, Value>> uniqueEdges;
   uniqueEdges.reserve(valueIter->size());
+
   const auto &space = qctx()->rctx()->session()->space();
+  const auto &vidType = *(space.spaceDesc.vid_type_ref());
   for (; valueIter->valid(); valueIter->next()) {
     auto type = ge->type()->eval(exprCtx(valueIter.get()));
     auto src = ge->src()->eval(exprCtx(valueIter.get()));
     auto dst = ge->dst()->eval(exprCtx(valueIter.get()));
     auto rank = ge->ranking()->eval(exprCtx(valueIter.get()));
+    if (!SchemaUtil::isValidVid(src, vidType)) {
+      std::stringstream ss;
+      ss << "`" << src.toString() << "', the src should be type of "
+         << apache::thrift::util::enumNameSafe(vidType.get_type()) << ", but was`" << src.type()
+         << "'";
+      return Status::Error(ss.str());
+    }
+    if (!SchemaUtil::isValidVid(dst, vidType)) {
+      std::stringstream ss;
+      ss << "`" << dst.toString() << "', the dst should be type of "
+         << apache::thrift::util::enumNameSafe(vidType.get_type()) << ", but was`" << dst.type()
+         << "'";
+      return Status::Error(ss.str());
+    }
     type = type < 0 ? -type : type;
     auto edgeKey = std::make_tuple(src, type, rank, dst);
     if (ge->dedup() && !uniqueEdges.emplace(std::move(edgeKey)).second) {
@@ -54,8 +70,9 @@ folly::Future<Status> GetEdgesExecutor::getEdges() {
     return Status::Error("ptr is nullptr");
   }
 
-  auto edges = buildRequestDataSet(ge);
+  auto res = buildRequestDataSet(ge);
   NG_RETURN_IF_ERROR(res);
+  auto edges = std::move(res).value();
 
   if (edges.rows.empty()) {
     // TODO: add test for empty input.

diff --git a/src/graph/executor/query/GetEdgesExecutor.h b/src/graph/executor/query/GetEdgesExecutor.h
@@ -19,7 +19,7 @@ class GetEdgesExecutor final : public GetPropExecutor {
   folly::Future<Status> execute() override;
 
  private:
-  DataSet buildRequestDataSet(const GetEdges *ge);
+  StatusOr<DataSet> buildRequestDataSet(const GetEdges *ge);
 
   folly::Future<Status> getEdges();
 };

diff --git a/src/graph/executor/query/GetNeighborsExecutor.cpp b/src/graph/executor/query/GetNeighborsExecutor.cpp
@@ -13,15 +13,17 @@ using nebula::storage::cpp2::GetNeighborsResponse;
 namespace nebula {
 namespace graph {
 
-DataSet GetNeighborsExecutor::buildRequestDataSet() {
+StatusOr<DataSet> GetNeighborsExecutor::buildRequestDataSet() {
   SCOPED_TIMER(&execTime_);
   auto inputVar = gn_->inputVar();
   auto iter = ectx_->getResult(inputVar).iter();
   return buildRequestDataSetByVidType(iter.get(), gn_->src(), gn_->dedup());
 }
 
 folly::Future<Status> GetNeighborsExecutor::execute() {
-  DataSet reqDs = buildRequestDataSet();
+  auto res = buildRequestDataSet();
+  NG_RETURN_IF_ERROR(res);
+  auto reqDs = std::move(res).value();
   if (reqDs.rows.empty()) {
     List emptyResult;
     return finish(ResultBuilder()

diff --git a/src/graph/executor/query/GetNeighborsExecutor.h b/src/graph/executor/query/GetNeighborsExecutor.h
@@ -21,7 +21,7 @@ class GetNeighborsExecutor final : public StorageAccessExecutor {
 
   folly::Future<Status> execute() override;
 
-  DataSet buildRequestDataSet();
+  StatusOr<DataSet> buildRequestDataSet();
 
  private:
   using RpcResponse = storage::StorageRpcResponse<storage::cpp2::GetNeighborsResponse>;

diff --git a/src/graph/executor/query/GetVerticesExecutor.cpp b/src/graph/executor/query/GetVerticesExecutor.cpp
@@ -21,7 +21,9 @@ folly::Future<Status> GetVerticesExecutor::getVertices() {
   auto *gv = asNode<GetVertices>(node());
   StorageClient *storageClient = qctx()->getStorageClient();
 
-  DataSet vertices = buildRequestDataSet(gv);
+  auto res = buildRequestDataSet(gv);
+  NG_RETURN_IF_ERROR(res);
+  auto vertices = std::move(res).value();
   if (vertices.rows.empty()) {
     // TODO: add test for empty input.
     return finish(
@@ -55,7 +57,7 @@ folly::Future<Status> GetVerticesExecutor::getVertices() {
       });
 }
 
-DataSet GetVerticesExecutor::buildRequestDataSet(const GetVertices *gv) {
+StatusOr<DataSet> GetVerticesExecutor::buildRequestDataSet(const GetVertices *gv) {
   if (gv == nullptr) {
     return nebula::DataSet({kVid});
   }

diff --git a/src/graph/executor/query/GetVerticesExecutor.h b/src/graph/executor/query/GetVerticesExecutor.h
@@ -19,7 +19,7 @@ class GetVerticesExecutor final : public GetPropExecutor {
   folly::Future<Status> execute() override;
 
  private:
-  DataSet buildRequestDataSet(const GetVertices *gv);
+  StatusOr<DataSet> buildRequestDataSet(const GetVertices *gv);
 
   folly::Future<Status> getVertices();
 };

diff --git a/src/graph/executor/test/GetNeighborsTest.cpp b/src/graph/executor/test/GetNeighborsTest.cpp
@@ -69,7 +69,8 @@ TEST_F(GetNeighborsTest, BuildRequestDataSet) {
   gn->setInputVar("input_gn");
 
   auto gnExe = std::make_unique<GetNeighborsExecutor>(gn, qctx_.get());
-  auto reqDs = gnExe->buildRequestDataSet();
+  auto res = gnExe->buildRequestDataSet();
+  auto reqDs = std::move(res).value();
 
   DataSet expected;
   expected.colNames = {kVid};

diff --git a/src/graph/validator/FetchEdgesValidator.cpp b/src/graph/validator/FetchEdgesValidator.cpp
@@ -50,12 +50,6 @@ StatusOr<std::string> FetchEdgesValidator::validateEdgeRef(const Expression *exp
   }
   auto exprType = deduceExprType(expr);
   NG_RETURN_IF_ERROR(exprType);
-  if (exprType.value() != type) {
-    std::stringstream ss;
-    ss << "`" << expr->toString() << "' should be type of " << type << ", but was "
-       << exprType.value();
-    return Status::SemanticError(ss.str());
-  }
   if (kind == Expression::Kind::kInputProperty) {
     return inputVarName_;
   }