Copilot commented on code in PR #2932:
URL: https://github.com/apache/sedona/pull/2932#discussion_r3206817861
##########
common/src/main/java/org/apache/sedona/common/Predicates.java:
##########
@@ -27,6 +28,30 @@ public static boolean contains(Geometry leftGeometry,
Geometry rightGeometry) {
return leftGeometry.contains(rightGeometry);
}
+ /**
+ * Closed-interval bbox intersection: true if {@code a} and {@code b} share
any point on either
+ * axis (matches PostGIS {@code &&} on box2d). Either argument being null
returns null at the SQL
+ * layer; this Java entry point throws {@link NullPointerException} on null
input.
+ */
+ public static boolean boxIntersects(Box2D a, Box2D b) {
+ return !(a.getXMax() < b.getXMin()
+ || a.getXMin() > b.getXMax()
+ || a.getYMax() < b.getYMin()
+ || a.getYMin() > b.getYMax());
+ }
+
+ /**
+ * True if {@code a} fully contains {@code b} (closed intervals; matches
PostGIS {@code ~} on
+ * box2d). Either argument being null returns null at the SQL layer; this
Java entry point throws
+ * {@link NullPointerException} on null input.
+ */
+ public static boolean boxContains(Box2D a, Box2D b) {
+ return a.getXMin() <= b.getXMin()
+ && a.getYMin() <= b.getYMin()
+ && a.getXMax() >= b.getXMax()
+ && a.getYMax() >= b.getYMax();
Review Comment:
`boxContains` has the same ordered-interval assumption as `boxIntersects`
(XMin<=XMax, YMin<=YMax). Since Box2D may legitimately carry inverted bounds
(e.g., from `ST_MakeBox2D` preserving swapped corners), this method can produce
misleading results. Consider validating/throwing on inverted bounds
(planar-only) or implementing explicit wraparound/inverted-interval semantics,
and cover it in unit tests.
##########
common/src/main/java/org/apache/sedona/common/Predicates.java:
##########
@@ -27,6 +28,30 @@ public static boolean contains(Geometry leftGeometry,
Geometry rightGeometry) {
return leftGeometry.contains(rightGeometry);
}
+ /**
+ * Closed-interval bbox intersection: true if {@code a} and {@code b} share
any point on either
+ * axis (matches PostGIS {@code &&} on box2d). Either argument being null
returns null at the SQL
+ * layer; this Java entry point throws {@link NullPointerException} on null
input.
+ */
+ public static boolean boxIntersects(Box2D a, Box2D b) {
+ return !(a.getXMax() < b.getXMin()
+ || a.getXMin() > b.getXMax()
+ || a.getYMax() < b.getYMin()
+ || a.getYMin() > b.getYMax());
Review Comment:
`boxIntersects` assumes each Box2D axis is an ordered interval (XMin<=XMax
and YMin<=YMax). However Box2D/`ST_MakeBox2D` explicitly preserve swapped
bounds (xmin>xmax / ymin>ymax) for future wraparound semantics, so this
implementation can return incorrect results for such values. Please either (a)
validate and fail fast on inverted bounds for these planar predicates, or (b)
define and implement consistent semantics for inverted intervals, and add tests
for that behavior.
##########
common/src/main/java/org/apache/sedona/common/Predicates.java:
##########
@@ -27,6 +28,30 @@ public static boolean contains(Geometry leftGeometry,
Geometry rightGeometry) {
return leftGeometry.contains(rightGeometry);
}
+ /**
+ * Closed-interval bbox intersection: true if {@code a} and {@code b} share
any point on either
+ * axis (matches PostGIS {@code &&} on box2d). Either argument being null
returns null at the SQL
+ * layer; this Java entry point throws {@link NullPointerException} on null
input.
Review Comment:
The Javadoc for `boxIntersects` says boxes “share any point on either axis”,
which is inaccurate/ambiguous for intersection (overlap is required on both
axes). Please reword to reflect the implemented predicate (closed-interval
overlap on X *and* Y).
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Predicates.scala:
##########
@@ -19,6 +19,7 @@
package org.apache.spark.sql.sedona_sql.expressions
import org.apache.sedona.common.Predicates
+import org.apache.sedona.common.geometryObjects.Box2D
Review Comment:
`Box2D` is imported here but not referenced in this file. Please remove the
unused import to keep the file clean and avoid IDE/linter warnings.
##########
spark/common/src/main/scala/org/apache/spark/sql/sedona_sql/expressions/Predicates.scala:
##########
@@ -95,6 +96,22 @@ private[apache] case class ST_Contains(inputExpressions:
Seq[Expression])
*
* @param inputExpressions
*/
+private[apache] case class ST_BoxIntersects(inputExpressions: Seq[Expression])
+ extends InferredExpression(Predicates.boxIntersects _) {
+
Review Comment:
The Scaladoc immediately above `ST_BoxIntersects` still describes geometry
`ST_Intersects` (“leftGeometry full intersects rightGeometry”, “Geometry (JTS)
and Geography (S2) inputs”). This is misleading for a Box2D-only
predicate—please update/move the comment so it documents the Box2D semantics,
and keep the geometry-intersects comment with `ST_Intersects`.
##########
common/src/test/java/org/apache/sedona/common/PredicatesTest.java:
##########
@@ -32,6 +33,38 @@ public class PredicatesTest extends TestBase {
private static final GeometryFactory GEOMETRY_FACTORY = new
GeometryFactory();
+ @Test
+ public void testBoxIntersects() {
+ Box2D a = new Box2D(0.0, 0.0, 5.0, 5.0);
+
+ // Full overlap
+ assertTrue(Predicates.boxIntersects(a, new Box2D(1.0, 1.0, 2.0, 2.0)));
+ // Partial overlap
+ assertTrue(Predicates.boxIntersects(a, new Box2D(3.0, 3.0, 7.0, 7.0)));
+ // Edge-touching (closed intervals)
+ assertTrue(Predicates.boxIntersects(a, new Box2D(5.0, 0.0, 10.0, 5.0)));
+ // Corner-touching (closed intervals)
+ assertTrue(Predicates.boxIntersects(a, new Box2D(5.0, 5.0, 10.0, 10.0)));
+ // Disjoint on X
+ assertFalse(Predicates.boxIntersects(a, new Box2D(6.0, 0.0, 10.0, 5.0)));
+ // Disjoint on Y
+ assertFalse(Predicates.boxIntersects(a, new Box2D(0.0, 6.0, 5.0, 10.0)));
+ }
Review Comment:
These new tests cover normal ordered Box2D ranges, but they don’t cover the
behavior when xmin>xmax and/or ymin>ymax (which is currently allowed/preserved
by ST_MakeBox2D for future wraparound semantics). Please add test cases
asserting the intended behavior for inverted bounds (either exception or
defined truth table), so regressions are caught.
--
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]
