From 97fbc0769ba38613d9c90b439ba7b7b46e2ad11e Mon Sep 17 00:00:00 2001 From: jievince <38901892+jievince@users.noreply.github.com> Date: Wed, 31 Aug 2022 16:04:16 +0800 Subject: [PATCH] better error msg for isValid of geography --- src/common/datatypes/Geography.cpp | 94 +++++++++++++++++-------- src/common/datatypes/Geography.h | 10 +-- src/common/function/FunctionManager.cpp | 3 +- 3 files changed, 74 insertions(+), 33 deletions(-) diff --git a/src/common/datatypes/Geography.cpp b/src/common/datatypes/Geography.cpp index 6336e978863..d596fd0523c 100644 --- a/src/common/datatypes/Geography.cpp +++ b/src/common/datatypes/Geography.cpp @@ -9,6 +9,7 @@ #include #include +#include #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(s2Region.get())->IsValid(); + + S2Error error; + if (static_cast(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(s2Region.get())->IsValid(); + + S2Error error; + if (static_cast(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::fromWKT(const std::string& wkt, @@ -122,9 +155,12 @@ StatusOr 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()) { + std::stringstream ss; + ss << "The geography " << geog.toString() << " built from wkt " << wkt + << " is invalid, error: " << status.message(); + return Status::Error(ss.str()); } } @@ -143,9 +179,12 @@ StatusOr 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()) { + std::stringstream ss; + ss << "The geography " << geog.toString() << " built from wkb " << wkb + << " is invalid, error: " << status.message(); + return Status::Error(ss.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 index 259a0508482..10c0a692282 100644 --- 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 index ef5af9050f3..8f5094e56d7 100644 --- 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