From db6da983909e5ffb807f12afa41093205931f5d5 Mon Sep 17 00:00:00 2001 From: "jie.wang" <38901892+jievince@users.noreply.github.com> Date: Fri, 22 Oct 2021 10:40:15 +0800 Subject: [PATCH] fix geo predicate rule and geography isvalid (#3180) --- src/common/datatypes/Geography.cpp | 6 ++++++ src/common/function/FunctionManager.cpp | 16 +++++++++----- src/common/geo/GeoFunction.cpp | 4 ++++ src/common/geo/io/wkb/test/WKBTest.cpp | 2 +- src/common/geo/test/GeoFunctionTest.cpp | 21 +++++++++++++++++++ src/common/utils/IndexKeyUtils.h | 2 +- .../rule/GeoPredicateIndexScanBaseRule.cpp | 6 +++--- src/graph/validator/LookupValidator.cpp | 2 ++ tests/tck/features/geo/GeoBase.feature | 18 ++++++++++++++++ 9 files changed, 67 insertions(+), 10 deletions(-) diff --git a/src/common/datatypes/Geography.cpp b/src/common/datatypes/Geography.cpp index aef9a457599..d26d43e6a66 100644 --- a/src/common/datatypes/Geography.cpp +++ b/src/common/datatypes/Geography.cpp @@ -50,6 +50,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(s2Region.get())->IsValid(); @@ -71,6 +74,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); diff --git a/src/common/function/FunctionManager.cpp b/src/common/function/FunctionManager.cpp index 07b402e74bb..68fc09dfd68 100644 --- a/src/common/function/FunctionManager.cpp +++ b/src/common/function/FunctionManager.cpp @@ -368,11 +368,16 @@ std::unordered_map> 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", @@ -2497,7 +2502,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; @@ -2507,10 +2512,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 diff --git a/src/common/geo/GeoFunction.cpp b/src/common/geo/GeoFunction.cpp index 5ceedb47b54..cbf8311721f 100644 --- a/src/common/geo/GeoFunction.cpp +++ b/src/common/geo/GeoFunction.cpp @@ -436,6 +436,10 @@ std::vector GeoFunction::s2CoveringCellIds( opts.set_max_cells(maxCells); if (bufferInMeters == 0.0) { + if (a.shape() == GeoShape::POINT) { + const S2Point& gPoint = static_cast(aRegion.get())->point(); + return {S2CellId(gPoint).id()}; + } return coveringCellIds(*aRegion, opts); } diff --git a/src/common/geo/io/wkb/test/WKBTest.cpp b/src/common/geo/io/wkb/test/WKBTest.cpp index e93c3ef1e2b..1d232a88e68 100644 --- a/src/common/geo/io/wkb/test/WKBTest.cpp +++ b/src/common/geo/io/wkb/test/WKBTest.cpp @@ -82,7 +82,7 @@ TEST_F(WKBTest, TestWKB) { LineString v(std::vector{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(0, 1), Coordinate(2, 3), Coordinate(0, 1)}); diff --git a/src/common/geo/test/GeoFunctionTest.cpp b/src/common/geo/test/GeoFunctionTest.cpp index 5bfc5ba489d..9cddf7105fb 100644 --- a/src/common/geo/test/GeoFunctionTest.cpp +++ b/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 diff --git a/src/common/utils/IndexKeyUtils.h b/src/common/utils/IndexKeyUtils.h index 049fabebebb..90522db8757 100644 --- a/src/common/utils/IndexKeyUtils.h +++ b/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; } diff --git a/src/graph/optimizer/rule/GeoPredicateIndexScanBaseRule.cpp b/src/graph/optimizer/rule/GeoPredicateIndexScanBaseRule.cpp index 5f7eacd82d9..2b10d1fb602 100644 --- a/src/graph/optimizer/rule/GeoPredicateIndexScanBaseRule.cpp +++ b/src/graph/optimizer/rule/GeoPredicateIndexScanBaseRule.cpp @@ -81,7 +81,7 @@ StatusOr GeoPredicateIndexScanBaseRule::transform( first->kind() == Expression::Kind::kEdgeProperty); DCHECK(second->kind() == Expression::Kind::kConstant); const auto& secondVal = static_cast(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 GeoPredicateIndexScanBaseRule::transform( auto* third = geoPredicate->args()->args()[2]; DCHECK_EQ(third->kind(), Expression::Kind::kConstant); const auto& thirdVal = static_cast(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 idxCtxs; diff --git a/src/graph/validator/LookupValidator.cpp b/src/graph/validator/LookupValidator.cpp index a17f83a9807..615354ac0b3 100644 --- a/src/graph/validator/LookupValidator.cpp +++ b/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(); diff --git a/tests/tck/features/geo/GeoBase.feature b/tests/tck/features/geo/GeoBase.feature index c390f20a6ed..1110c17c9f1 100644 --- a/tests/tck/features/geo/GeoBase.feature +++ b/tests/tck/features/geo/GeoBase.feature @@ -420,6 +420,16 @@ Feature: Geo base LOOKUP ON any_shape WHERE ST_Intersects(any_shape.geo, any_shape.geo) YIELD ST_ASText(any_shape.geo); """ Then a SemanticError should be raised at runtime: Expression ST_Intersects(any_shape.geo,any_shape.geo) not supported yet + When executing query: + """ + LOOKUP ON any_shape WHERE ST_Distance(any_shape.geo, ST_Point(3, 8.4)) < true YIELD ST_ASText(any_shape.geo); + """ + Then a SemanticError should be raised at runtime: + When executing query: + """ + LOOKUP ON any_shape WHERE ST_DWithin(any_shape.geo, ST_Point(3, 8.4), true) YIELD ST_ASText(any_shape.geo); + """ + Then a SemanticError should be raised at runtime: # Match with geo predicate When executing query: """ @@ -523,6 +533,14 @@ Feature: Geo base | VertexID | ST_ASText(any_shape.geo) | | "101" | "POINT(3 8)" | | "102" | "LINESTRING(3 8, 4.7 73.23)" | + When executing query: + """ + LOOKUP ON any_shape WHERE ST_DWithin(any_shape.geo, ST_Point(3, 8), 100) YIELD ST_ASText(any_shape.geo); + """ + Then the result should be, in any order: + | VertexID | ST_ASText(any_shape.geo) | + | "101" | "POINT(3 8)" | + | "102" | "LINESTRING(3 8, 4.7 73.23)" | # ST_Covers When executing query: """