Skip to content

Commit

Permalink
Better error msg for validity check of geography (#4601)
Browse files Browse the repository at this point in the history
* better error msg for isValid of geography

* fix compile of ctest

Co-authored-by: Sophie <[email protected]>
  • Loading branch information
jievince and Sophie-Xie authored Sep 7, 2022
1 parent 4b86948 commit 6898ebb
Show file tree
Hide file tree
Showing 5 changed files with 116 additions and 68 deletions.
100 changes: 71 additions & 29 deletions src/common/datatypes/Geography.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,31 +58,52 @@ void Coordinate::normalize() {
}
}

bool Coordinate::isValid() const {
return std::abs(x) <= kMaxLongitude && std::abs(y) <= kMaxLatitude;
Status Coordinate::isValid() const {
if (std::abs(x) > kMaxLongitude) {
return Status::Error("Invalid longitude: %f", x);
}
if (std::abs(y) > kMaxLatitude) {
return Status::Error("Invalid latitude: %f", y);
}
return Status::OK();
}

std::string Coordinate::toString() const {
return "(" + std::to_string(x) + " " + std::to_string(y) + ")";
}

void Point::normalize() {}

bool Point::isValid() const {
Status Point::isValid() const {
return coord.isValid();
}

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];
auto status = coord.isValid();
if (!status.ok()) {
return Status::Error("The %zu-th coordinate is invalid: %s", i, status.message().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() {
Expand All @@ -91,23 +112,39 @@ 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];
auto status = coord.isValid();
if (!status.ok()) {
return Status::Error("The %zu-th coordinate of %zu-th LinearRing is invalid: %s",
j,
i,
status.message().c_str());
}
}
}
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,
Expand All @@ -122,9 +159,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()) {
std::stringstream ss;
ss << "The geography " << geog.toString() << " built from wkt " << wkt
<< " is invalid, error: " << status.message();
return Status::Error(ss.str());
}
}

Expand All @@ -143,9 +183,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()) {
std::stringstream ss;
ss << "The geography " << geog.toString() << " built from wkb " << wkb
<< " is invalid, error: " << status.message();
return Status::Error(ss.str());
}
}

Expand Down Expand Up @@ -222,7 +265,7 @@ void Geography::normalize() {
}
}

bool Geography::isValid() const {
Status Geography::isValid() const {
switch (shape()) {
case GeoShape::POINT: {
const auto& point = this->point();
Expand All @@ -238,9 +281,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");
}
}
}
Expand All @@ -266,7 +308,7 @@ Point Geography::centroid() const {
default: {
LOG(ERROR)
<< "Geography shapes other than Point/LineString/Polygon are not currently supported";
return Point();
return {};
}
}
}
Expand Down
12 changes: 7 additions & 5 deletions src/common/datatypes/Geography.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ struct Coordinate {
Coordinate(double lng, double lat) : x(lng), y(lat) {}

void normalize();
bool isValid() const;
Status isValid() const;

std::string toString() const;

void clear() {
x = 0.0;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -116,7 +118,7 @@ struct LineString {
}

void normalize();
bool isValid() const;
Status isValid() const;

void clear() {
coordList.clear();
Expand Down Expand Up @@ -145,7 +147,7 @@ struct Polygon {
}

void normalize();
bool isValid() const;
Status isValid() const;

void clear() {
coordListList.clear();
Expand Down Expand Up @@ -193,7 +195,7 @@ struct Geography {
Polygon& mutablePolygon();

void normalize();
bool isValid() const;
Status isValid() const;

Point centroid() const;

Expand Down
6 changes: 5 additions & 1 deletion src/common/function/FunctionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2511,7 +2511,11 @@ FunctionManager::FunctionManager() {
if (!args[0].get().isGeography()) {
return Value::kNullBadType;
}
return args[0].get().getGeography().isValid();
auto status = args[0].get().getGeography().isValid();
if (!status.ok()) {
LOG(ERROR) << "ST_IsValid error: " << status;
}
return status.ok();
};
}
// geo predicates
Expand Down
26 changes: 13 additions & 13 deletions src/common/geo/io/wkb/test/WKBTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -41,25 +41,25 @@ TEST_F(WKBTest, TestWKB) {
Point v(Coordinate(24.7, 36.842));
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(true, v.isValid());
EXPECT_EQ(true, v.isValid().ok());
}
{
Point v(Coordinate(-179, 36.842));
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(true, v.isValid());
EXPECT_EQ(true, v.isValid().ok());
}
{
Point v(Coordinate(24.7, 36.842));
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(true, v.isValid());
EXPECT_EQ(true, v.isValid().ok());
}
{
Point v(Coordinate(298.4, 499.99));
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(false, v.isValid());
EXPECT_EQ(false, v.isValid().ok());
}
{
Point v(Coordinate(24.7, 36.842));
Expand All @@ -75,32 +75,32 @@ TEST_F(WKBTest, TestWKB) {
Coordinate(0, 1), Coordinate(1, 2), Coordinate(2, 3), Coordinate(3, 4)});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(true, v.isValid());
EXPECT_EQ(true, v.isValid().ok());
}
{
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(false, v.isValid());
EXPECT_EQ(false, v.isValid().ok());
}
{
LineString v(std::vector<Coordinate>{Coordinate(0, 1), Coordinate(2, 3), Coordinate(0, 1)});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(true, v.isValid());
EXPECT_EQ(true, v.isValid().ok());
}
{
LineString v(std::vector<Coordinate>{Coordinate(0, 1), Coordinate(0, 1)});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(false, v.isValid());
EXPECT_EQ(false, v.isValid().ok());
}
// LineString must have at least 2 points
{
LineString v(std::vector<Coordinate>{Coordinate(0, 1)});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(false, v.isValid());
EXPECT_EQ(false, v.isValid().ok());
}
{
LineString v(std::vector<Coordinate>{Coordinate(26.4, 78.9), Coordinate(138.725, 91.0)});
Expand All @@ -124,7 +124,7 @@ TEST_F(WKBTest, TestWKB) {
Coordinate(0, 1), Coordinate(1, 2), Coordinate(2, 3), Coordinate(0, 1)}});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(true, v.isValid());
EXPECT_EQ(true, v.isValid().ok());
}
{
Polygon v(std::vector<std::vector<Coordinate>>{
Expand All @@ -137,23 +137,23 @@ TEST_F(WKBTest, TestWKB) {
Coordinate(4, 5)}});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(true, v.isValid());
EXPECT_EQ(true, v.isValid().ok());
}
// The loop is not closed
{
Polygon v(std::vector<std::vector<Coordinate>>{std::vector<Coordinate>{
Coordinate(0, 1), Coordinate(1, 2), Coordinate(2, 3), Coordinate(3, 4)}});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(false, v.isValid());
EXPECT_EQ(false, v.isValid().ok());
}
// Loop must have at least 4 points
{
Polygon v(std::vector<std::vector<Coordinate>>{
std::vector<Coordinate>{Coordinate(0, 1), Coordinate(1, 2), Coordinate(0, 1)}});
auto result = read(v);
ASSERT_TRUE(result.ok()) << result.status();
EXPECT_EQ(false, v.isValid());
EXPECT_EQ(false, v.isValid().ok());
}
{
Polygon v(std::vector<std::vector<Coordinate>>{std::vector<Coordinate>{
Expand Down
Loading

0 comments on commit 6898ebb

Please sign in to comment.