better error msg for isValid of geography

vesoft-inc · Aug 31, 2022 · d505057 · d505057
1 parent db8b909
commit d505057
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 33 deletions.
diff --git a/src/common/datatypes/Geography.cpp b/src/common/datatypes/Geography.cpp
@@ -9,6 +9,7 @@
 #include <folly/hash/Hash.h>
 
 #include <cstdint>
+#include <string>
 
 #include "common/geo/GeoUtils.h"
 #include "common/geo/io/wkb/WKBReader.h"
@@ -62,27 +63,44 @@ bool Coordinate::isValid() const {
   return std::abs(x) <= kMaxLongitude && std::abs(y) <= kMaxLatitude;
 }
 
+std::string Coordinate::toString() const {
+  return "(" + std::to_string(x) + " " + std::to_string(y) + ")";
+}
+
 void Point::normalize() {}
 
-bool Point::isValid() const {
-  return coord.isValid();
+Status Point::isValid() const {
+  if (coord.isValid()) {
+    return Status::OK();
+  }
+  return Status::Error("The coordinate is invalid");
 }
 
 void LineString::normalize() {
   geo::GeoUtils::removeAdjacentDuplicateCoordinates(coordList);
 }
 
-bool LineString::isValid() const {
-  // LineString must have at least 2 coordinates;
+Status LineString::isValid() const {
   if (coordList.size() < 2) {
-    return false;
+    return Status::Error("LineString contains %zu coordinates, requires at least 2 coordinates",
+                         coordList.size());
   }
-  for (const auto& coord : coordList) {
-    if (!coord.isValid()) return false;
+  for (size_t i = 0; i < coordList.size(); ++i) {
+    auto& coord = coordList[i];
+    if (!coord.isValid()) {
+      return Status::Error("The %zu-th coordinate %s is invalid", i, coord.toString().c_str());
+    }
   }
   auto s2Region = geo::GeoUtils::s2RegionFromGeography(*this);
   CHECK_NOTNULL(s2Region);
-  return static_cast<S2Polyline*>(s2Region.get())->IsValid();
+
+  S2Error error;
+  if (static_cast<S2Polyline*>(s2Region.get())->FindValidationError(&error)) {
+    std::stringstream ss;
+    ss << "FindValidationError, S2 Errorcode: " << error.code() << ", message: " << error.text();
+    return Status::Error(ss.str());
+  }
+  return Status::OK();
 }
 
 void Polygon::normalize() {
@@ -91,23 +109,38 @@ void Polygon::normalize() {
   }
 }
 
-bool Polygon::isValid() const {
-  for (const auto& coordList : coordListList) {
-    // Polygon's LinearRing must have at least 4 coordinates
+Status Polygon::isValid() const {
+  for (size_t i = 0; i < coordListList.size(); ++i) {
+    auto& coordList = coordListList[i];
     if (coordList.size() < 4) {
-      return false;
+      return Status::Error(
+          "Polygon's %zu-th LinearRing contains %zu coordinates, requires at least 4",
+          i,
+          coordList.size());
     }
-    // Polygon's LinearRing must be closed
     if (coordList.front() != coordList.back()) {
-      return false;
+      return Status::Error("Polygon's %zu-th LinearRing is not closed", i);
     }
-    for (const auto& coord : coordList) {
-      if (!coord.isValid()) return false;
+    for (size_t j = 0; j < coordList.size(); ++j) {
+      auto& coord = coordList[j];
+      if (!coord.isValid()) {
+        return Status::Error("The %zu-th coordinate %s of %zu-th LinearRing is invalid",
+                             j,
+                             coord.toString().c_str(),
+                             i);
+      }
     }
   }
   auto s2Region = geo::GeoUtils::s2RegionFromGeography(*this);
   CHECK_NOTNULL(s2Region);
-  return static_cast<S2Polygon*>(s2Region.get())->IsValid();
+
+  S2Error error;
+  if (static_cast<S2Polygon*>(s2Region.get())->FindValidationError(&error)) {
+    std::stringstream ss;
+    ss << "FindValidationError, S2 Errorcode: " << error.code() << ", message: " << error.text();
+    return Status::Error(ss.str());
+  }
+  return Status::OK();
 }
 
 StatusOr<Geography> Geography::fromWKT(const std::string& wkt,
@@ -122,9 +155,12 @@ StatusOr<Geography> Geography::fromWKT(const std::string& wkt,
     geog.normalize();
   }
   if (verifyValidity) {
-    if (!geog.isValid()) {
-      return Status::Error("Failed to parse an valid Geography instance from the wkt `%s'",
-                           wkt.c_str());
+    auto status = geog.isValid();
+    if (!status.ok()) {
+      return Status::Error(
+          "Failed to parse an valid Geography instance from the wkt `%s', error: %s",
+          wkt.c_str(),
+          status.message().c_str());
     }
   }
 
@@ -143,9 +179,12 @@ StatusOr<Geography> Geography::fromWKB(const std::string& wkb,
     geog.normalize();
   }
   if (verifyValidity) {
-    if (!geog.isValid()) {
-      return Status::Error("Failed to parse an valid Geography instance from the wkb `%s'",
-                           wkb.c_str());
+    auto status = geog.isValid();
+    if (!status.ok()) {
+      return Status::Error(
+          "Failed to parse an valid Geography instance from the wkb `%s', error: %s",
+          wkb.c_str(),
+          status.message().c_str());
     }
   }
 
@@ -222,7 +261,7 @@ void Geography::normalize() {
   }
 }
 
-bool Geography::isValid() const {
+Status Geography::isValid() const {
   switch (shape()) {
     case GeoShape::POINT: {
       const auto& point = this->point();
@@ -238,9 +277,8 @@ bool Geography::isValid() const {
     }
     case GeoShape::UNKNOWN:
     default: {
-      LOG(ERROR)
-          << "Geography shapes other than Point/LineString/Polygon are not currently supported";
-      return false;
+      return Status::Error(
+          "Geography shapes other than Point/LineString/Polygon are not currently supported");
     }
   }
 }
@@ -266,7 +304,7 @@ Point Geography::centroid() const {
     default: {
       LOG(ERROR)
           << "Geography shapes other than Point/LineString/Polygon are not currently supported";
-      return Point();
+      return {};
     }
   }
 }

diff --git a/src/common/datatypes/Geography.h b/src/common/datatypes/Geography.h
@@ -54,6 +54,8 @@ struct Coordinate {
   void normalize();
   bool isValid() const;
 
+  std::string toString() const;
+
   void clear() {
     x = 0.0;
     y = 0.0;
@@ -87,7 +89,7 @@ struct Point {
   explicit Point(Coordinate&& v) : coord(std::move(v)) {}
 
   void normalize();
-  bool isValid() const;
+  Status isValid() const;
 
   void clear() {
     coord.clear();
@@ -116,7 +118,7 @@ struct LineString {
   }
 
   void normalize();
-  bool isValid() const;
+  Status isValid() const;
 
   void clear() {
     coordList.clear();
@@ -145,7 +147,7 @@ struct Polygon {
   }
 
   void normalize();
-  bool isValid() const;
+  Status isValid() const;
 
   void clear() {
     coordListList.clear();
@@ -193,7 +195,7 @@ struct Geography {
   Polygon& mutablePolygon();
 
   void normalize();
-  bool isValid() const;
+  Status isValid() const;
 
   Point centroid() const;
 

diff --git a/src/common/function/FunctionManager.cpp b/src/common/function/FunctionManager.cpp
@@ -2511,7 +2511,8 @@ FunctionManager::FunctionManager() {
       if (!args[0].get().isGeography()) {
         return Value::kNullBadType;
       }
-      return args[0].get().getGeography().isValid();
+      auto status = args[0].get().getGeography().isValid();
+      return status.ok() ? true : false;
     };
   }
   // geo predicates