Merge branch 'round' of github.com:jackwener/nebula into round

.github/pull_request_template.md

@@ -0,0 +1,29 @@
+#### What type of PR is this?
+- [ ] bug
+- [ ] feature
+
+#### Which issue(s) this PR fixes:
+close #xxx   
+（If it is requirement, issue(s) number must be listed.）
+
+#### What this PR does / why we need it?
+
+
+#### Special notes for your reviewer, ex. impact of this fix, etc:
+
+
+#### Additional context:
+
+
+#### Checklist：
+- [ ] Documentation affected （If need to modify document, please label it.)
+- [ ] Incompatible （If it is incompatile, please describle it and label it.）
+- [ ] Need to cherry pick （If need to cherry pick to some branchs, please label the destination version(s).)
+- [ ] Performance regression: Consumes more CPU
+- [ ] Performance regression: Consumes more Memory
+
+
+
+#### Release notes：
+Please confirm whether to reflect in release notes and how to describe:
+>                                                                 `

CMakeLists.txt

@@ -48,7 +48,7 @@ include(NebulaCustomTargets)
 
 add_custom_target(
     install-all
-    COMMAND $(MAKE) install
+    COMMAND $MAKE install
     WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
 )
 

src/common/datatypes/Geography.cpp

@@ -19,6 +19,30 @@
 
 namespace nebula {
 
+std::ostream& operator<<(std::ostream& os, const GeoShape& shape) {
+  switch (shape) {
+    case GeoShape::POINT: {
+      os << "POINT";
+      break;
+    }
+    case GeoShape::LINESTRING: {
+      os << "LINESTRING";
+      break;
+    }
+    case GeoShape::POLYGON: {
+      os << "POLYGON";
+      break;
+    }
+    case GeoShape::UNKNOWN:
+    default: {
+      os << "__UNKNOWN__";
+      break;
+    }
+  }
+
+  return os;
+}
+
 constexpr double kMaxLongitude = 180.0;
 constexpr double kMaxLatitude = 90.0;
 
@@ -50,6 +74,9 @@ bool LineString::isValid() const {
   if (coordList.size() < 2) {
     return false;
   }
+  for (const auto& coord : coordList) {
+    if (!coord.isValid()) return false;
+  }
   auto s2Region = geo::GeoUtils::s2RegionFromGeography(*this);
   CHECK_NOTNULL(s2Region);
   return static_cast<S2Polyline*>(s2Region.get())->IsValid();
@@ -71,6 +98,9 @@ bool Polygon::isValid() const {
     if (coordList.front() != coordList.back()) {
       return false;
     }
+    for (const auto& coord : coordList) {
+      if (!coord.isValid()) return false;
+    }
   }
   auto s2Region = geo::GeoUtils::s2RegionFromGeography(*this);
   CHECK_NOTNULL(s2Region);

src/common/datatypes/Geography.h

@@ -32,6 +32,8 @@ enum class GeoShape : uint32_t {
   POLYGON = 3,
 };
 
+std::ostream& operator<<(std::ostream& os, const GeoShape& shape);
+
 // clang-format off
 /*
 static const std::unordered_map<GeoShape, S2Region> kShapeTypeToS2Region = {

src/common/function/FunctionManager.cpp

@@ -370,11 +370,16 @@ std::unordered_map<std::string, std::vector<TypeSignature>> FunctionManager::typ
      {
          TypeSignature({Value::Type::GEOGRAPHY, Value::Type::GEOGRAPHY, Value::Type::FLOAT},
                        Value::Type::BOOL),
+         TypeSignature({Value::Type::GEOGRAPHY, Value::Type::GEOGRAPHY, Value::Type::INT},
+                       Value::Type::BOOL),
          TypeSignature({Value::Type::GEOGRAPHY,
                         Value::Type::GEOGRAPHY,
                         Value::Type::FLOAT,
                         Value::Type::BOOL},
                        Value::Type::BOOL),
+         TypeSignature(
+             {Value::Type::GEOGRAPHY, Value::Type::GEOGRAPHY, Value::Type::INT, Value::Type::BOOL},
+             Value::Type::BOOL),
      }},
     // geo measures
     {"st_distance",
@@ -2508,7 +2513,7 @@ FunctionManager::FunctionManager() {
     attr.isPure_ = true;
     attr.body_ = [](const auto &args) -> Value {
       if (!args[0].get().isGeography() || !args[1].get().isGeography() ||
-          !args[2].get().isFloat()) {
+          !args[2].get().isNumeric()) {
         return Value::kNullBadType;
       }
       bool exclusive = false;
@@ -2518,10 +2523,11 @@ FunctionManager::FunctionManager() {
         }
         exclusive = args[3].get().getBool();
       }
-      return geo::GeoFunction::dWithin(args[0].get().getGeography(),
-                                       args[1].get().getGeography(),
-                                       args[2].get().getFloat(),
-                                       exclusive);
+      return geo::GeoFunction::dWithin(
+          args[0].get().getGeography(),
+          args[1].get().getGeography(),
+          args[2].get().isFloat() ? args[2].get().getFloat() : args[2].get().getInt(),
+          exclusive);
     };
   }
   // geo measures

src/common/geo/GeoFunction.cpp

@@ -436,6 +436,10 @@ std::vector<uint64_t> GeoFunction::s2CoveringCellIds(
   opts.set_max_cells(maxCells);
 
   if (bufferInMeters == 0.0) {
+    if (a.shape() == GeoShape::POINT) {
+      const S2Point& gPoint = static_cast<const S2PointRegion*>(aRegion.get())->point();
+      return {S2CellId(gPoint).id()};
+    }
     return coveringCellIds(*aRegion, opts);
   }
 

src/common/geo/io/wkb/test/WKBTest.cpp

@@ -82,7 +82,7 @@ TEST_F(WKBTest, TestWKB) {
     LineString v(std::vector<Coordinate>{Coordinate(26.4, 78.9), Coordinate(138.725, 91.0)});
     auto result = read(v);
     ASSERT_TRUE(result.ok()) << result.status();
-    EXPECT_EQ(true, v.isValid());
+    EXPECT_EQ(false, v.isValid());
   }
   {
     LineString v(std::vector<Coordinate>{Coordinate(0, 1), Coordinate(2, 3), Coordinate(0, 1)});

src/common/geo/test/GeoFunctionTest.cpp

@@ -1096,6 +1096,16 @@ TEST(isValid, lineString) {
     bool b = line.isValid();
     EXPECT_EQ(false, b);
   }
+  {
+    auto line = Geography::fromWKT("LINESTRING(1.0 1.0, 181.0 2.0)").value();
+    bool b = line.isValid();
+    EXPECT_EQ(false, b);
+  }
+  {
+    auto line = Geography::fromWKT("LINESTRING(1.0 1.0, 1.0 90.001)").value();
+    bool b = line.isValid();
+    EXPECT_EQ(false, b);
+  }
 }
 
 TEST(isValid, polygon) {
@@ -1153,6 +1163,17 @@ TEST(isValid, polygon) {
   //   bool b = polygon.isValid();
   //   EXPECT_EQ(false, b);  // Expect false, got true
   // }
+  {
+    auto polygon =
+        Geography::fromWKT("POLYGON((1.0 1.0, -180.0001 2.0, 0.0 2.0, 1.0 1.0))").value();
+    bool b = polygon.isValid();
+    EXPECT_EQ(false, b);
+  }
+  {
+    auto polygon = Geography::fromWKT("POLYGON((1.0 1.0, 2.0 2.0, 0.0 -90.001, 1.0 1.0))").value();
+    bool b = polygon.isValid();
+    EXPECT_EQ(false, b);
+  }
 }
 
 }  // namespace geo

src/common/utils/IndexKeyUtils.h

@@ -437,7 +437,7 @@ class IndexKeyUtils final {
           break;
         }
         case Value::Type::GEOGRAPHY: {
-          // LOG(FATAL) << "unable to get geography value from index key"
+          // NOTE: The data read from index key is S2CellId which type is uint64, not wkb
           len = sizeof(uint64_t);
           break;
         }

src/graph/optimizer/rule/GeoPredicateIndexScanBaseRule.cpp

@@ -81,7 +81,7 @@ StatusOr<TransformResult> GeoPredicateIndexScanBaseRule::transform(
          first->kind() == Expression::Kind::kEdgeProperty);
   DCHECK(second->kind() == Expression::Kind::kConstant);
   const auto& secondVal = static_cast<const ConstantExpression*>(second)->value();
-  DCHECK(secondVal.type() == Value::Type::GEOGRAPHY);
+  DCHECK(secondVal.isGeography());
   const auto& geog = secondVal.getGeography();
 
   // TODO(jie): Get index params from meta to construct RegionCoverParams
@@ -100,8 +100,8 @@ StatusOr<TransformResult> GeoPredicateIndexScanBaseRule::transform(
     auto* third = geoPredicate->args()->args()[2];
     DCHECK_EQ(third->kind(), Expression::Kind::kConstant);
     const auto& thirdVal = static_cast<const ConstantExpression*>(third)->value();
-    DCHECK_EQ(thirdVal.type(), Value::Type::FLOAT);
-    double distanceInMeters = thirdVal.getFloat();
+    DCHECK(thirdVal.isNumeric());
+    double distanceInMeters = thirdVal.isFloat() ? thirdVal.getFloat() : thirdVal.getInt();
     scanRanges = geoIndex.dWithin(geog, distanceInMeters);
   }
   std::vector<IndexQueryContext> idxCtxs;

src/graph/validator/FetchEdgesValidator.cpp

@@ -150,34 +150,14 @@ void FetchEdgesValidator::extractEdgeProp(ExpressionProps &exprProps) {
 }
 
 Status FetchEdgesValidator::validateYield(const YieldClause *yield) {
-  auto pool = qctx_->objPool();
-  bool noYield = false;
   if (yield == nullptr) {
-    // TODO: compatible with previous version, this will be deprecated in version 3.0.
-    auto *yieldColumns = new YieldColumns();
-    auto *edge = new YieldColumn(EdgeExpression::make(pool), "edges_");
-    yieldColumns->addColumn(edge);
-    yield = pool->add(new YieldClause(yieldColumns));
-    noYield = true;
+    return Status::SemanticError("Missing yield clause.");
   }
   fetchCtx_->distinct = yield->isDistinct();
-
   auto &exprProps = fetchCtx_->exprProps;
-  auto *newCols = pool->add(new YieldColumns());
-  if (!noYield) {
-    auto *src = new YieldColumn(EdgeSrcIdExpression::make(pool, edgeName_));
-    auto *dst = new YieldColumn(EdgeDstIdExpression::make(pool, edgeName_));
-    auto *rank = new YieldColumn(EdgeRankExpression::make(pool, edgeName_));
-    outputs_.emplace_back(src->name(), vidType_);
-    outputs_.emplace_back(dst->name(), vidType_);
-    outputs_.emplace_back(rank->name(), Value::Type::INT);
-    newCols->addColumn(src);
-    newCols->addColumn(dst);
-    newCols->addColumn(rank);
-    exprProps.insertEdgeProp(edgeType_, kSrc);
-    exprProps.insertEdgeProp(edgeType_, kDst);
-    exprProps.insertEdgeProp(edgeType_, kRank);
-  }
+  exprProps.insertEdgeProp(edgeType_, nebula::kSrc);
+  exprProps.insertEdgeProp(edgeType_, nebula::kDst);
+  exprProps.insertEdgeProp(edgeType_, nebula::kRank);
 
   for (const auto &col : yield->columns()) {
     if (ExpressionUtils::hasAny(col->expr(), {Expression::Kind::kEdge})) {
@@ -186,8 +166,10 @@ Status FetchEdgesValidator::validateYield(const YieldClause *yield) {
     }
   }
   auto size = yield->columns().size();
-  outputs_.reserve(size + 3);
+  outputs_.reserve(size);
 
+  auto pool = qctx_->objPool();
+  auto *newCols = pool->add(new YieldColumns());
   for (auto col : yield->columns()) {
     if (ExpressionUtils::hasAny(col->expr(),
                                 {Expression::Kind::kVertex, Expression::Kind::kPathBuild})) {

src/graph/validator/FetchVerticesValidator.cpp

@@ -13,8 +13,6 @@
 namespace nebula {
 namespace graph {
 
-static constexpr char VertexID[] = "VertexID";
-
 Status FetchVerticesValidator::validateImpl() {
   auto *fSentence = static_cast<FetchVerticesSentence *>(sentence_);
   fetchCtx_ = getContext<FetchVerticesContext>();
@@ -52,26 +50,15 @@ Status FetchVerticesValidator::validateTag(const NameLabelList *nameLabels) {
 }
 
 Status FetchVerticesValidator::validateYield(YieldClause *yield) {
-  auto pool = qctx_->objPool();
-  bool noYield = false;
   if (yield == nullptr) {
-    // TODO: compatible with previous version, this will be deprecated in version 3.0.
-    auto *yieldColumns = new YieldColumns();
-    auto *vertex = new YieldColumn(VertexExpression::make(pool), "vertices_");
-    yieldColumns->addColumn(vertex);
-    yield = pool->add(new YieldClause(yieldColumns));
-    noYield = true;
+    return Status::SemanticError("Missing yield clause.");
   }
   fetchCtx_->distinct = yield->isDistinct();
   auto size = yield->columns().size();
-  outputs_.reserve(size + 1);
+  outputs_.reserve(size);
 
+  auto pool = qctx_->objPool();
   auto *newCols = pool->add(new YieldColumns());
-  if (!noYield) {
-    outputs_.emplace_back(VertexID, vidType_);
-    auto *vidCol = new YieldColumn(InputPropertyExpression::make(pool, nebula::kVid), VertexID);
-    newCols->addColumn(vidCol);
-  }
 
   auto &exprProps = fetchCtx_->exprProps;
   for (auto col : yield->columns()) {

src/graph/validator/GoValidator.cpp

@@ -115,27 +115,13 @@ Status GoValidator::validateTruncate(TruncateClause* truncate) {
 }
 
 Status GoValidator::validateYield(YieldClause* yield) {
+  if (yield == nullptr) {
+    return Status::SemanticError("Missing yield clause.");
+  }
   goCtx_->distinct = yield->isDistinct();
-  const auto& over = goCtx_->over;
-  auto* pool = qctx_->objPool();
   auto& exprProps = goCtx_->exprProps;
 
-  auto cols = yield->columns();
-  if (cols.empty() && over.isOverAll) {
-    DCHECK(!over.allEdges.empty());
-    auto* newCols = pool->add(new YieldColumns());
-    for (const auto& e : over.allEdges) {
-      auto* col = new YieldColumn(EdgeDstIdExpression::make(pool, e));
-      newCols->addColumn(col);
-      outputs_.emplace_back(col->name(), vidType_);
-      NG_RETURN_IF_ERROR(deduceProps(col->expr(), exprProps));
-    }
-    goCtx_->yieldExpr = newCols;
-    goCtx_->colNames = getOutColNames();
-    return Status::OK();
-  }
-
-  for (auto col : cols) {
+  for (auto col : yield->columns()) {
     if (ExpressionUtils::hasAny(col->expr(),
                                 {Expression::Kind::kAggregate, Expression::Kind::kPathBuild})) {
       return Status::SemanticError("`%s' is not support in go sentence.", col->toString().c_str());
@@ -169,6 +155,7 @@ Status GoValidator::validateYield(YieldClause* yield) {
     NG_RETURN_IF_ERROR(deduceProps(colExpr, exprProps));
   }
 
+  const auto& over = goCtx_->over;
   for (const auto& e : exprProps.edgeProps()) {
     auto found = std::find(over.edgeTypes.begin(), over.edgeTypes.end(), e.first);
     if (found == over.edgeTypes.end()) {

src/graph/validator/LookupValidator.cpp

@@ -224,6 +224,8 @@ Status LookupValidator::validateFilter() {
     auto ret = checkFilter(filter);
     NG_RETURN_IF_ERROR(ret);
     lookupCtx_->filter = std::move(ret).value();
+    // Make sure the type of the rewritted filter expr is right
+    NG_RETURN_IF_ERROR(deduceExprType(lookupCtx_->filter));
   }
   NG_RETURN_IF_ERROR(deduceProps(lookupCtx_->filter, exprProps_));
   return Status::OK();