Remove multiple object creation in envelope calculation, and fixed test

craigtaverner · Nov 18, 2024 · 22cac2e · 22cac2e
1 parent 0d51e35
commit 22cac2e
Show file tree

Hide file tree

Showing 13 changed files with 87 additions and 70 deletions.
diff --git a/docs/reference/esql/functions/kibana/definition/st_envelope.json b/docs/reference/esql/functions/kibana/definition/st_envelope.json
diff --git a/docs/reference/esql/functions/kibana/docs/st_envelope.md b/docs/reference/esql/functions/kibana/docs/st_envelope.md
diff --git a/...sql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/SpatialEnvelopeVisitor.java b/...sql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/SpatialEnvelopeVisitor.java
@@ -19,35 +19,35 @@
 import org.elasticsearch.geometry.Polygon;
 import org.elasticsearch.geometry.Rectangle;
 
-public class SpatialEnvelopeVisitor implements GeometryVisitor<Rectangle, RuntimeException> {
+public class SpatialEnvelopeVisitor implements GeometryVisitor<Boolean, RuntimeException> {
 
     private double minX = Double.POSITIVE_INFINITY;
     private double minY = Double.POSITIVE_INFINITY;
     private double maxX = Double.NEGATIVE_INFINITY;
     private double maxY = Double.NEGATIVE_INFINITY;
 
+    public Rectangle getResult() {
+        return new Rectangle(minX, maxX, maxY, minY);
+    }
+
+    private boolean isValid() {
+        return minX != Double.POSITIVE_INFINITY;
+    }
+
     @Override
-    public Rectangle visit(Circle circle) throws RuntimeException {
+    public Boolean visit(Circle circle) throws RuntimeException {
         // TODO: Support circle, if given CRS (needed for radius to x/y coordinate transformation)
         throw new UnsupportedOperationException("Circle is not supported");
     }
 
     @Override
-    public Rectangle visit(GeometryCollection<?> collection) throws RuntimeException {
-        collection.forEach(geometry -> {
-            Rectangle envelope = geometry.visit(this);
-            if (envelope != null) {
-                minX = Math.min(minX, envelope.getMinX());
-                minY = Math.min(minY, envelope.getMinY());
-                maxX = Math.max(maxX, envelope.getMaxX());
-                maxY = Math.max(maxY, envelope.getMaxY());
-            }
-        });
-        return new Rectangle(minX, maxX, maxY, minY);
+    public Boolean visit(GeometryCollection<?> collection) throws RuntimeException {
+        collection.forEach(geometry -> geometry.visit(this));
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(Line line) throws RuntimeException {
+    public Boolean visit(Line line) throws RuntimeException {
         for (int i = 0; i < line.length(); i++) {
             double x = line.getX(i);
             double y = line.getY(i);
@@ -56,11 +56,11 @@ public Rectangle visit(Line line) throws RuntimeException {
             maxX = Math.max(maxX, x);
             maxY = Math.max(maxY, y);
         }
-        return new Rectangle(minX, maxX, maxY, minY);
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(LinearRing ring) throws RuntimeException {
+    public Boolean visit(LinearRing ring) throws RuntimeException {
         for (int i = 0; i < ring.length(); i++) {
             double x = ring.getX(i);
             double y = ring.getY(i);
@@ -69,70 +69,57 @@ public Rectangle visit(LinearRing ring) throws RuntimeException {
             maxX = Math.max(maxX, x);
             maxY = Math.max(maxY, y);
         }
-        return new Rectangle(minX, maxX, maxY, minY);
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(MultiLine multiLine) throws RuntimeException {
-        multiLine.forEach(line -> {
-            Rectangle envelope = line.visit(this);
-            if (envelope != null) {
-                minX = Math.min(minX, envelope.getMinX());
-                minY = Math.min(minY, envelope.getMinY());
-                maxX = Math.max(maxX, envelope.getMaxX());
-                maxY = Math.max(maxY, envelope.getMaxY());
-            }
-        });
-        return new Rectangle(minX, maxX, maxY, minY);
+    public Boolean visit(MultiLine multiLine) throws RuntimeException {
+        multiLine.forEach(line -> line.visit(this));
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(MultiPoint multiPoint) throws RuntimeException {
+    public Boolean visit(MultiPoint multiPoint) throws RuntimeException {
         for (int i = 0; i < multiPoint.size(); i++) {
             Point point = multiPoint.get(i);
             minX = Math.min(minX, point.getX());
             minY = Math.min(minY, point.getY());
             maxX = Math.max(maxX, point.getX());
             maxY = Math.max(maxY, point.getY());
         }
-        return new Rectangle(minX, maxX, maxY, minY);
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(MultiPolygon multiPolygon) throws RuntimeException {
-        multiPolygon.forEach(polygon -> {
-            Rectangle envelope = polygon.visit(this);
-            if (envelope != null) {
-                minX = Math.min(minX, envelope.getMinX());
-                minY = Math.min(minY, envelope.getMinY());
-                maxX = Math.max(maxX, envelope.getMaxX());
-                maxY = Math.max(maxY, envelope.getMaxY());
-            }
-        });
-        return new Rectangle(minX, maxX, maxY, minY);
+    public Boolean visit(MultiPolygon multiPolygon) throws RuntimeException {
+        multiPolygon.forEach(polygon -> polygon.visit(this));
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(Point point) throws RuntimeException {
+    public Boolean visit(Point point) throws RuntimeException {
         minX = Math.min(minX, point.getX());
         minY = Math.min(minY, point.getY());
         maxX = Math.max(maxX, point.getX());
         maxY = Math.max(maxY, point.getY());
-        return new Rectangle(minX, maxX, maxY, minY);
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(Polygon polygon) throws RuntimeException {
-        // TODO: Should we visit the holes also?
-        return visit(polygon.getPolygon());
+    public Boolean visit(Polygon polygon) throws RuntimeException {
+        visit(polygon.getPolygon());
+        for (int i = 0; i < polygon.getNumberOfHoles(); i++) {
+            visit(polygon.getHole(i));
+        }
+        return isValid();
     }
 
     @Override
-    public Rectangle visit(Rectangle rectangle) throws RuntimeException {
+    public Boolean visit(Rectangle rectangle) throws RuntimeException {
         minX = Math.min(minX, rectangle.getMinX());
         minY = Math.min(minY, rectangle.getMinY());
         maxX = Math.max(maxX, rectangle.getMaxX());
         maxY = Math.max(maxY, rectangle.getMaxY());
-        return new Rectangle(minX, maxX, maxY, minY);
+        return isValid();
     }
 }
diff --git a/...main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelope.java b/...main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelope.java
@@ -113,7 +113,10 @@ static BytesRef fromWellKnownBinary(BytesRef wkb) {
         if (geometry instanceof Point) {
             return wkb;
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return UNSPECIFIED.asWkb(envelope);
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return UNSPECIFIED.asWkb(envelope.getResult());
+        }
+        throw new IllegalArgumentException("Cannot determine envelope of geometry");
     }
 }
diff --git a/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMax.java b/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMax.java
@@ -97,7 +97,10 @@ static double fromWellKnownBinary(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getX();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMaxX();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMaxX();
+        }
+        throw new IllegalArgumentException("Cannot determine envelope of geometry");
     }
 }
diff --git a/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMin.java b/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMin.java
@@ -97,7 +97,10 @@ static double fromWellKnownBinary(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getX();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMinX();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMinX();
+        }
+        throw new IllegalArgumentException("Cannot determine envelope of geometry");
     }
 }
diff --git a/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMax.java b/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMax.java
@@ -97,7 +97,10 @@ static double fromWellKnownBinary(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getY();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMaxY();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMaxY();
+        }
+        throw new IllegalArgumentException("Cannot determine envelope of geometry");
     }
 }
diff --git a/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMin.java b/...src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMin.java
@@ -97,7 +97,10 @@ static double fromWellKnownBinary(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getY();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMinY();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMinY();
+        }
+        throw new IllegalArgumentException("Cannot determine envelope of geometry");
     }
 }
diff --git a/...java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelopeTests.java b/...java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelopeTests.java
@@ -53,8 +53,11 @@ private static BytesRef valueOf(BytesRef wkb) {
         if (geometry instanceof Point) {
             return wkb;
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return UNSPECIFIED.asWkb(envelope);
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return UNSPECIFIED.asWkb(envelope.getResult());
+        }
+        throw new IllegalArgumentException("Geometry is empty");
     }
 
     @Override

diff --git a/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMaxTests.java b/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMaxTests.java
@@ -52,8 +52,11 @@ private static double valueOf(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getX();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMaxX();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMaxX();
+        }
+        throw new IllegalArgumentException("Geometry is empty");
     }
 
     @Override

diff --git a/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMinTests.java b/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMinTests.java
@@ -52,8 +52,11 @@ private static double valueOf(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getX();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMinX();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMinX();
+        }
+        throw new IllegalArgumentException("Geometry is empty");
     }
 
     @Override

diff --git a/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMaxTests.java b/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMaxTests.java
@@ -52,8 +52,11 @@ private static double valueOf(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getY();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMaxY();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMaxY();
+        }
+        throw new IllegalArgumentException("Geometry is empty");
     }
 
     @Override

diff --git a/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMinTests.java b/...est/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMinTests.java
@@ -52,8 +52,11 @@ private static double valueOf(BytesRef wkb) {
         if (geometry instanceof Point point) {
             return point.getY();
         }
-        var envelope = geometry.visit(new SpatialEnvelopeVisitor());
-        return envelope.getMinY();
+        var envelope = new SpatialEnvelopeVisitor();
+        if (geometry.visit(envelope)) {
+            return envelope.getResult().getMinY();
+        }
+        throw new IllegalArgumentException("Geometry is empty");
     }
 
     @Override