github-actions[bot] commented on code in PR #65790:
URL: https://github.com/apache/doris/pull/65790#discussion_r3953916238
##########
be/src/exprs/function/geo/geo_types.h:
##########
@@ -70,6 +70,8 @@ class GeoShape {
virtual bool touches(const GeoShape* rhs) const { return false; }
+ virtual bool within(const GeoShape* rhs) const { return
rhs->contains(this); }
Review Comment:
[P1] Complete the relation before exposing it as `ST_Within`
This entry point accepts every decoded `GeoShape`, but it delegates entirely
to `rhs->contains(this)`. `GeoPoint` and `GeoLine` never override `contains`
(so point-on-line, sub-line/equal-line, and equal-point cases are always
false), while `GeoCircle::contains` handles only points. Even the implemented
paths are not a whole-region predicate: `GeoPolygon::contains(GeoLine)` rejects
a line with one boundary-coincident segment plus an interior segment, and
`GeoMultiPolygon::contains(GeoLine)` requires one component to contain the
entire line rather than testing the union of point-touching components. All of
those inputs can satisfy the Within definition (no point in the exterior and an
interior-interior intersection). Please implement the complete supported
geometry-pair/whole-region relation and add these concrete matrix cases.
##########
regression-test/suites/query_p0/sql_functions/spatial_functions/test_gis_function.groovy:
##########
@@ -86,6 +86,37 @@ suite("test_gis_function", "arrow_flight_sql") {
qt_sql "SELECT ST_Contains(ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10
10, 10 0, 0 0)), ((15 5, 15 10, 20 10, 20 5, 15 5)))'),
ST_GeomFromText('MULTIPOLYGON(((-0.000001 -0.000001, -0.000001 10.000001,
10.000001 10.000001, 10.000001 -0.000001, -0.000001 -0.000001)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
qt_sql "SELECT ST_Contains(ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10
10, 10 0, 0 0), (3 3, 3 7, 7 7, 7 3, 3 3)), ((15 5, 15 10, 20 10, 20 5, 15
5)))'), ST_GeomFromText('MULTIPOLYGON(((1 1, 1 9, 9 9, 9 1, 1 1), (4 4, 4 6, 6
6, 6 4, 4 4)), ((16 6, 16 9, 19 9, 19 6, 16 6)))'));"
+ qt_sql "SELECT ST_Within(ST_Point(5, 5), ST_Polygon(\"POLYGON ((0 0, 10 0,
10 10, 0 10, 0 0))\"));"
Review Comment:
[P2] Exercise the registered vectorized function boundary
All added SQL cases build valid literal geometries, and the C++ test calls
`GeoShape::within` directly, so none proves the new
`GeoFunction<StRelationFunction<StWithinFunc>>` path for ordinary, Const,
Nullable, and invalid encoded inputs. The new-function guide linked from #48203
explicitly requires a BE UT using `check_function_all_arg_comb` for Const
combinations. Please add that function-level test (including NULL/invalid data)
and at least one non-constant SQL case so the nullable decode/output path is
covered.
##########
regression-test/suites/query_p0/sql_functions/spatial_functions/test_gis_function.groovy:
##########
@@ -86,6 +86,37 @@ suite("test_gis_function", "arrow_flight_sql") {
qt_sql "SELECT ST_Contains(ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10
10, 10 0, 0 0)), ((15 5, 15 10, 20 10, 20 5, 15 5)))'),
ST_GeomFromText('MULTIPOLYGON(((-0.000001 -0.000001, -0.000001 10.000001,
10.000001 10.000001, 10.000001 -0.000001, -0.000001 -0.000001)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
qt_sql "SELECT ST_Contains(ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10
10, 10 0, 0 0), (3 3, 3 7, 7 7, 7 3, 3 3)), ((15 5, 15 10, 20 10, 20 5, 15
5)))'), ST_GeomFromText('MULTIPOLYGON(((1 1, 1 9, 9 9, 9 1, 1 1), (4 4, 4 6, 6
6, 6 4, 4 4)), ((16 6, 16 9, 19 9, 19 6, 16 6)))'));"
+ qt_sql "SELECT ST_Within(ST_Point(5, 5), ST_Polygon(\"POLYGON ((0 0, 10 0,
10 10, 0 10, 0 0))\"));"
+ qt_sql "SELECT ST_Within(ST_Point(50, 50), ST_Polygon(\"POLYGON ((0 0, 10
0, 10 10, 0 10, 0 0))\"));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POINT(2 10)'),
ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+
+ qt_sql "SELECT ST_Within(ST_GeomFromText('LINESTRING(2 5, 8 5)'),
ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('LINESTRING(2 0, 8 0)'),
ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('LINESTRING(0 0, 10 0)'),
ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('LINESTRING(0 0, 10 10)'),
ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0,
0 0))'), ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((3 3, 3 7, 7 7, 7 3, 3
3))'), ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((10 10, 20 10, 20 20, 10
20, 10 10))'), ST_GeomFromText('POLYGON((0 0, 0 10, 10 10, 10 0, 0 0))'));"
+
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((4 4, 7 4, 7 7, 4 7, 4
4))'), ST_GeomFromText('POLYGON((0 0, 10 0, 10 10, 0 10, 0 0), (3 3, 8 3, 8 8,
3 8, 3 3))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((1 1, 9 1, 9 9, 1 9, 1
1))'), ST_GeomFromText('POLYGON((0 0, 10 0, 10 10, 0 10, 0 0), (3 3, 8 3, 8 8,
3 8, 3 3))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('MULTIPOLYGON(((2 2, 4 2, 4 4, 2
4, 2 2)), ((6 6, 8 6, 8 8, 6 8, 6 6)))'), ST_GeomFromText('POLYGON((0 0, 0 10,
10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('MULTIPOLYGON(((2 2, 2 8, 8 8, 8
2, 2 2)), ((10 10, 10 15, 15 15, 15 10, 10 10)))'), ST_GeomFromText('POLYGON((0
0, 0 10, 10 10, 10 0, 0 0))'));"
+ qt_sql "SELECT ST_Within(ST_Point(5, 5), ST_GeomFromText('MULTIPOLYGON(((0
0, 0 10, 10 10, 10 0, 0 0)), ((15 5, 15 10, 20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_Point(17, 7),
ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_Point(12, 7),
ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('LINESTRING(2 2, 8 8)'),
ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('LINESTRING(5 5, 16 6)'),
ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((1 1, 1 9, 9 9, 9 1, 1
1))'), ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5,
15 10, 20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((16 6, 16 9, 19 9, 19 6,
16 6))'), ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15
5, 15 10, 20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('POLYGON((5 5, 5 15, 15 15, 15 5,
5 5))'), ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5,
15 10, 20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('MULTIPOLYGON(((1 1, 1 5, 5 5, 5
1, 1 1)), ((16 6, 16 9, 19 9, 19 6, 16 6)))'),
ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_GeomFromText('MULTIPOLYGON(((1 1, 1 5, 5 5, 5
1, 1 1)), ((12 6, 12 9, 14 9, 14 6, 12 6)))'),
ST_GeomFromText('MULTIPOLYGON(((0 0, 0 10, 10 10, 10 0, 0 0)), ((15 5, 15 10,
20 10, 20 5, 15 5)))'));"
+ qt_sql "SELECT ST_Within(ST_Point(0.5, 0.5), ST_Circle(0, 0, 1000000));"
Review Comment:
[P2] Exclude the Circle boundary from `ST_Within`
These tests make Circle an intentional RHS, but `within` reaches
`GeoCircle::contains(Point)`, which calls `S2Cap::Contains`. S2 caps are closed
sets, so that method includes the boundary; for example
`ST_Within(ST_Point(1,0), ST_Circle(0,0,ST_Distance_Sphere(0,0,1,0)))` returns
true. Within requires the interiors to intersect, so a point on the Circle
boundary must be false, consistent with the added Polygon boundary cases.
Please use strict interior containment (S2 provides `InteriorContains`) and add
this exact boundary oracle.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
