Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error msg for validity check of geography #4601

Merged
merged 6 commits into from
Sep 7, 2022
Merged

Better error msg for validity check of geography #4601

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a4f8d0d
better error msg for isValid of geography
jievince Aug 31, 2022
4a07114
fix compile of ctest
jievince Sep 1, 2022
b0f415a
Merge branch 'master' into better-err-msg-for-geo
jievince Sep 7, 2022
82607dc
Merge branch 'master' into better-err-msg-for-geo
jievince Sep 7, 2022
c5dadad
Merge branch 'master' into better-err-msg-for-geo
Sophie-Xie Sep 7, 2022
454bc08
Merge branch 'master' into better-err-msg-for-geo
Sophie-Xie Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 71 additions & 29 deletions src/common/datatypes/Geography.cpp
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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