Copilot commented on code in PR #2858: URL: https://github.com/apache/sedona/pull/2858#discussion_r3148652121
########## docs/api/sql/geography/Geography-Functions/ST_Within.md: ########## @@ -0,0 +1,48 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + --> + +# ST_Within + +Introduction: Tests whether geography A is fully within geography B using S2 spherical boolean operations. Returns true if every point of A is inside or on the boundary of B. By OGC convention, `ST_Within(A, B)` is equivalent to `ST_Contains(B, A)`. Review Comment: Two doc issues to address: (1) the boundary semantics statement is ambiguous/possibly inconsistent with the stated OGC identity `Within(A,B) == Contains(B,A)`—please explicitly document whether boundary-only cases return true or false (and ideally add a small boundary example). (2) The SQL example uses `ST_GeogFromWKT` without the SRID argument, while other Geography docs/tests in this PR pass `4326`; update the example to match the supported signature so it can be copy-pasted and executed reliably. ########## spark/common/src/test/scala/org/apache/sedona/sql/geography/GeographyFunctionTest.scala: ########## @@ -153,6 +153,114 @@ class GeographyFunctionTest extends TestBaseScala { .first() assertTrue(!row.getBoolean(0)) } + + it("ST_DWithin true when within threshold") { + val row = sparkSession + .sql(""" + SELECT ST_DWithin( + ST_GeogFromWKT('POINT (0 0)', 4326), + ST_GeogFromWKT('POINT (0 1)', 4326), + 200000.0) AS r + """) + .first() + assertTrue(row.getBoolean(0)) + } + + it("ST_DWithin false when outside threshold") { + val row = sparkSession + .sql(""" + SELECT ST_DWithin( + ST_GeogFromWKT('POINT (0 0)', 4326), + ST_GeogFromWKT('POINT (0 1)', 4326), + 100000.0) AS r + """) + .first() + assertTrue(!row.getBoolean(0)) + } + + it("ST_DWithin null handling") { + // null as second arg + val r1 = sparkSession + .sql("SELECT ST_DWithin(ST_GeogFromWKT('POINT (0 0)', 4326), null, 1.0) AS r") + .first() + assertTrue(r1.isNullAt(0)) + // null as first arg + val r2 = sparkSession + .sql("SELECT ST_DWithin(null, ST_GeogFromWKT('POINT (0 0)', 4326), 1.0) AS r") + .first() + assertTrue(r2.isNullAt(0)) + // null distance + val r3 = sparkSession + .sql(""" + SELECT ST_DWithin( + ST_GeogFromWKT('POINT (0 0)', 4326), + ST_GeogFromWKT('POINT (0 1)', 4326), + CAST(null AS DOUBLE)) AS r + """) + .first() + assertTrue(r3.isNullAt(0)) + } + + it("ST_DWithin accepts INT distance literal") { + // Catalyst should coerce INT -> DOUBLE for the 3-arg Geography overload. + val row = sparkSession + .sql(""" + SELECT ST_DWithin( + ST_GeogFromWKT('POINT (0 0)', 4326), + ST_GeogFromWKT('POINT (0 1)', 4326), + 200000) AS r + """) + .first() + assertTrue(row.getBoolean(0)) + } + + it("ST_DWithin accepts FLOAT distance literal") { + // CAST to FLOAT forces a narrower type than DOUBLE; Catalyst should widen it. + val row = sparkSession + .sql(""" + SELECT ST_DWithin( + ST_GeogFromWKT('POINT (0 0)', 4326), + ST_GeogFromWKT('POINT (0 1)', 4326), + CAST(200000.5 AS FLOAT)) AS r + """) + .first() + assertTrue(row.getBoolean(0)) + } + + it("ST_Within point in polygon") { + val row = sparkSession + .sql(""" + SELECT ST_Within( + ST_GeogFromWKT('POINT (0.5 0.5)', 4326), + ST_GeogFromWKT('POLYGON ((0 0, 1 0, 1 1, 0 1, 0 0))', 4326) + ) AS r + """) + .first() + assertTrue(row.getBoolean(0)) + } + Review Comment: The new Geography `ST_Within` semantics around boundary interaction aren’t exercised at the Spark SQL layer. Please add a Spark-side test for a point on the polygon boundary (and/or a polygon touching the boundary) to lock in the intended behavior, since users will observe Spark’s result rather than the Java helper directly. ########## spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/strategy/join/JoinQueryDetector.scala: ########## @@ -222,6 +212,17 @@ class JoinQueryDetector(sparkSession: SparkSession) extends SparkStrategy { SpatialPredicate.CONTAINS, false, extraCondition)) + case ST_Within(Seq(leftShape, rightShape)) + if !isGeographyInput(leftShape) && !isGeographyInput(rightShape) => + Some( + JoinQueryDetection( + left, + right, + leftShape, + rightShape, + SpatialPredicate.WITHIN, + false, + extraCondition)) Review Comment: The `ST_Contains` and `ST_Within` inferred-expression branches duplicate the same Geography-guard + `JoinQueryDetection` construction. Consider extracting a small helper (e.g., `inferredJoin(predicate, spatialPredicate)`) to reduce duplication and keep future inferred predicates consistent (especially around `isGeography` and extra-condition handling). ########## docs/api/sql/geography/Geography-Functions/ST_Within.md: ########## @@ -0,0 +1,48 @@ +<!-- + Licensed to the Apache Software Foundation (ASF) under one + or more contributor license agreements. See the NOTICE file + distributed with this work for additional information + regarding copyright ownership. The ASF licenses this file + to you under the Apache License, Version 2.0 (the + "License"); you may not use this file except in compliance + with the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, + software distributed under the License is distributed on an + "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + KIND, either express or implied. See the License for the + specific language governing permissions and limitations + under the License. + --> + +# ST_Within + +Introduction: Tests whether geography A is fully within geography B using S2 spherical boolean operations. Returns true if every point of A is inside or on the boundary of B. By OGC convention, `ST_Within(A, B)` is equivalent to `ST_Contains(B, A)`. + + + + +Format: + +`ST_Within (A: Geography, B: Geography)` + +Return type: `Boolean` + +Since: `v1.9.1` + +SQL Example + Review Comment: Two doc issues to address: (1) the boundary semantics statement is ambiguous/possibly inconsistent with the stated OGC identity `Within(A,B) == Contains(B,A)`—please explicitly document whether boundary-only cases return true or false (and ideally add a small boundary example). (2) The SQL example uses `ST_GeogFromWKT` without the SRID argument, while other Geography docs/tests in this PR pass `4326`; update the example to match the supported signature so it can be copy-pasted and executed reliably. -- 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]
