From 22cac2e93cc9bfc06c2f2fdf24886e03f067de9e Mon Sep 17 00:00:00 2001 From: Craig Taverner Date: Mon, 18 Nov 2024 18:04:50 +0100 Subject: [PATCH] Remove multiple object creation in envelope calculation, and fixed test --- .../kibana/definition/st_envelope.json | 2 +- .../esql/functions/kibana/docs/st_envelope.md | 2 +- .../core/util/SpatialEnvelopeVisitor.java | 83 ++++++++----------- .../function/scalar/spatial/StEnvelope.java | 7 +- .../function/scalar/spatial/StXMax.java | 7 +- .../function/scalar/spatial/StXMin.java | 7 +- .../function/scalar/spatial/StYMax.java | 7 +- .../function/scalar/spatial/StYMin.java | 7 +- .../scalar/spatial/StEnvelopeTests.java | 7 +- .../function/scalar/spatial/StXMaxTests.java | 7 +- .../function/scalar/spatial/StXMinTests.java | 7 +- .../function/scalar/spatial/StYMaxTests.java | 7 +- .../function/scalar/spatial/StYMinTests.java | 7 +- 13 files changed, 87 insertions(+), 70 deletions(-) diff --git a/docs/reference/esql/functions/kibana/definition/st_envelope.json b/docs/reference/esql/functions/kibana/definition/st_envelope.json index a6c88f74a853e..6c00dda265ac7 100644 --- a/docs/reference/esql/functions/kibana/definition/st_envelope.json +++ b/docs/reference/esql/functions/kibana/definition/st_envelope.json @@ -54,7 +54,7 @@ } ], "examples" : [ - "FROM airport_city_boundaries\n| WHERE abbrev == \"CPH\"\n| EVAL envelope = ST_ENVELOPE(city_boundary)\n| KEEP abbrev, airport, xmin, xmax, ymin, ymax" + "FROM airport_city_boundaries\n| WHERE abbrev == \"CPH\"\n| EVAL envelope = ST_ENVELOPE(city_boundary)\n| KEEP abbrev, airport, envelope" ], "preview" : false, "snapshot_only" : false diff --git a/docs/reference/esql/functions/kibana/docs/st_envelope.md b/docs/reference/esql/functions/kibana/docs/st_envelope.md index 017e15ba6a75e..5f4c3e4809a82 100644 --- a/docs/reference/esql/functions/kibana/docs/st_envelope.md +++ b/docs/reference/esql/functions/kibana/docs/st_envelope.md @@ -9,5 +9,5 @@ Determines the minimum bounding box of the supplied geometry. FROM airport_city_boundaries | WHERE abbrev == "CPH" | EVAL envelope = ST_ENVELOPE(city_boundary) -| KEEP abbrev, airport, xmin, xmax, ymin, ymax +| KEEP abbrev, airport, envelope ``` diff --git a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/SpatialEnvelopeVisitor.java b/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/SpatialEnvelopeVisitor.java index b3a08a012151f..16fbe8559ba21 100644 --- a/x-pack/plugin/esql-core/src/main/java/org/elasticsearch/xpack/esql/core/util/SpatialEnvelopeVisitor.java +++ b/x-pack/plugin/esql-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 { +public class SpatialEnvelopeVisitor implements GeometryVisitor { 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,25 +69,17 @@ 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()); @@ -95,44 +87,39 @@ public Rectangle visit(MultiPoint multiPoint) throws RuntimeException { 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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelope.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelope.java index 3fa7131b77e90..9c880329e99e1 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelope.java +++ b/x-pack/plugin/esql/src/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMax.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMax.java index 5552887979ae2..df1ec064b779f 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMax.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMin.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMin.java index 6a9836ac0a6a5..5b26af048772b 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMin.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMax.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMax.java index 74a6064eb5573..4f10fc540fc55 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMax.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMin.java b/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMin.java index 81fc505b3619b..77ec5554cb619 100644 --- a/x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMin.java +++ b/x-pack/plugin/esql/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/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelopeTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelopeTests.java index 8a57c1ec3b692..345e46b42f4c4 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelopeTests.java +++ b/x-pack/plugin/esql/src/test/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/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMaxTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMaxTests.java index 7e33dc6e8499d..ffacf37c19a89 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMaxTests.java +++ b/x-pack/plugin/esql/src/test/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/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMinTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMinTests.java index e34c51397d0b9..bf388ddd0f77b 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMinTests.java +++ b/x-pack/plugin/esql/src/test/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/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMaxTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMaxTests.java index 46dc99de86cc2..645cf7c434b1c 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMaxTests.java +++ b/x-pack/plugin/esql/src/test/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/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMinTests.java b/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMinTests.java index f37bb0f417933..79396e03ed597 100644 --- a/x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StYMinTests.java +++ b/x-pack/plugin/esql/src/test/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