Support ST_ENVELOPE and related ST_XMIN, etc. #116964

craigtaverner · 2024-11-18T16:03:50Z

Support ST_ENVELOPE and related ST_XMIN, etc.

Based on the PostGIS equivalents:

Using ST_XMIN and similar can be done directly to the geometry, or by first calculating the envelope so the same geometry does not need to be iterated through multiple times:

Note that when the incoming data is Geo (geo_point and geo_shape) the bounding box can wrap the dateline, in which case the final result will have xMin > xMax. This is the same as the default behaviour of the existing geo_bounds aggregation. However, in that aggregation it is possible to control this behaviour with the wrap_longitude boolean flag. In the current work we do not yet support that flag, and always wrap the longitude for geographic data.

Fixes #104875

github-actions · 2024-11-18T16:04:04Z

Documentation preview:

✨ Changed pages

elasticsearchmachine · 2024-11-18T16:04:14Z

Pinging @elastic/es-analytical-engine (Team:Analytics)

elasticsearchmachine · 2024-11-18T16:04:59Z

Hi @craigtaverner, I've created a changelog YAML for you.

elasticsearchmachine · 2024-11-19T14:27:13Z

Hi @craigtaverner, I've updated the changelog YAML for you.

…into st_envelope

This will allow wrapping longitude around the dateline. TODO: Write tests and use different Evaluator for the Geo version.

Asserting in particular that the geo-wrapping over the dateline works.

craigtaverner · 2024-11-26T09:38:04Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

+ * Code that wishes to modify the behaviour of the visitor can implement the <code>PointVisitor</code> interface,
+ * or extend the existing implementations.
+ */
+public class SpatialEnvelopeVisitor implements GeometryVisitor<Boolean, RuntimeException> {


This class is in lib/geo because it is needed by both ESQL for the ST_ENVELOPE function as well as in the compute engine for the ST_EXTENT aggregation. It seems generic enough for libs/geo, but another choice would be to put it in the compute engine. I did not do that because there was no obvious place for generic spatial utils there. We could make one, of course.

I think this implementation might evolve quite a bit so let's leave it here for now

GalLalouche · 2024-11-27T08:57:59Z

...c/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMaxTests.java

+import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.UNSPECIFIED;
+
+@FunctionName("st_xmax")
+public class StXMaxTests extends AbstractScalarFunctionTestCase {


There's a lot of copy paste between all 4 X/Y Min/Max tests. Although this is test code, it could easily be refactored out using an abstract test template.

I looked through the differences between two tests and the the number of lines that are actually the same is quite low. Mostly they are very similar, but not identical. I think any effort to reduce duplication will add complexity and make the tests harder to follow.

GalLalouche · 2024-11-27T09:03:25Z

...ql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StXMax.java

+    }
+
+    @ConvertEvaluator(extraName = "FromWKB", warnExceptions = { IllegalArgumentException.class })
+    static double fromWellKnownBinary(BytesRef wkb) {


This 4x copy paste with 2 line differences could easily be factored out, and the JIT should take care of any performance issues.

I thought about common code, where we pass in functions for the two unique calls (eg. getY and getYMax), but that creates much harder to read code, and saves only a few lines of code duplication, so I thought it was not worth it.

GalLalouche · 2024-11-27T09:03:44Z

...rc/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StEnvelope.java

+ * The function `st_envelope` is defined in the <a href="https://www.ogc.org/standard/sfs/">OGC Simple Feature Access</a> standard.
+ * Alternatively it is well described in PostGIS documentation at
+ * <a href="https://postgis.net/docs/ST_ENVELOPE.html">PostGIS:ST_ENVELOPE</a>.
+ *


Nit: remove empty line

GalLalouche · 2024-11-27T09:12:17Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

+    /**
+     * Determine the BBOX assuming the CRS is geographic (eg WGS84) and optionally wrapping the longitude around the dateline.
+     */
+    public static Optional<Rectangle> visit(Geometry geometry, boolean wrapLongitude) {


Avoid overloading here: it isn't clear which visit refers to Cartesian and which to geo coordinates.

Replace boolean with an enum or an overload (between two geo shape versions) to avoid Boolean Blindness.

I would use different names to specify what is geo and what is cartesian. no so fuzzy about the use of boolean here, enum is fine too.

Used different names, by copying the version in ST_EXTENT PR. If we want to use an enum, we can make that change in that PR.

GalLalouche · 2024-11-27T09:27:04Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

+
+        boolean isValid();
+
+        Rectangle getResult();


Since this can return null, it should return Optional<Rectangle> (Or at the very least be marked with @Nullable, but Optional is better)

It should never return null. I removed the one case where it does.

GalLalouche · 2024-11-27T09:44:12Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

+            visitLongitude(maxX);
+        }
+
+        private void visitLongitude(double x) {


While mathematically correct, I find this more confusing than it should be, since you're comparing both min and maxes with both min and maxes :)

GalLalouche · 2024-11-27T09:45:55Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

+
+        @Override
+        public Rectangle getResult() {
+            if (Double.isInfinite(maxY)) {


For consistency's sake, consider using isValid instead of this check.

Removed this check. The SpatialEnvelopeVisitor already calls isValid before this, and this check is an unnecessary secondary call.

GalLalouche · 2024-11-27T09:53:26Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

+        }
+
+        @Override
+        public Rectangle getResult() {


To allow this to be used outside of this class, consider extracting this to a static method, maybe in another class.

iverase

We need to consider quantization of the coordinates but I spoke to Craig offline and we will do it once we push things down to lucene,

GalLalouche · 2024-11-27T12:35:41Z

libs/geo/src/main/java/org/elasticsearch/geometry/utils/SpatialEnvelopeVisitor.java

+     * It also disallows invalid rectangles where minX > maxX.
+     */
+    public static class CartesianPointVisitor implements PointVisitor {
+        private double minX = Double.POSITIVE_INFINITY;


These should be either be protected or exposed via getters so they can be used by its clients, e.g., during intermediate serialization. For my money, I prefer getters, due to the whole "prefer composition to inheritance".

Got this from the changes in ST_EXTENT PR.

Support ST_ENVELOPE and related ST_XMIN, etc. Based on the PostGIS equivalents: https://postgis.net/docs/ST_Envelope.html https://postgis.net/docs/ST_XMin.html https://postgis.net/docs/ST_XMax.html https://postgis.net/docs/ST_YMin.html https://postgis.net/docs/ST_YMax.html

* Support ST_ENVELOPE and related ST_XMIN, etc. (#116964) Support ST_ENVELOPE and related ST_XMIN, etc. Based on the PostGIS equivalents: https://postgis.net/docs/ST_Envelope.html https://postgis.net/docs/ST_XMin.html https://postgis.net/docs/ST_XMax.html https://postgis.net/docs/ST_YMin.html https://postgis.net/docs/ST_YMax.html * Fix off-by-one error reported in #118051

…lastic#118743) * Support ST_ENVELOPE and related ST_XMIN, etc. (elastic#116964) Support ST_ENVELOPE and related ST_XMIN, etc. Based on the PostGIS equivalents: https://postgis.net/docs/ST_Envelope.html https://postgis.net/docs/ST_XMin.html https://postgis.net/docs/ST_XMax.html https://postgis.net/docs/ST_YMin.html https://postgis.net/docs/ST_YMax.html * Fix off-by-one error reported in elastic#118051

Support ST_ENVELOPE and related ST_XMIN, etc.

62ae981

craigtaverner added >feature :Analytics/Geo Indexing, search aggregations of geo points and shapes Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) :Analytics/ES|QL AKA ESQL labels Nov 18, 2024

elasticsearchmachine added the v9.0.0 label Nov 18, 2024

craigtaverner added 5 commits November 18, 2024 17:04

Update docs/changelog/116964.yaml

713c8c4

Fix changelog

0d51e35

Remove multiple object creation in envelope calculation, and fixed test

22cac2e

Increase function count on esql.functions

8e76a4b

Simpler wrapper around SpatialEnvelopeVisitor

90869f3

craigtaverner requested review from GalLalouche and iverase November 18, 2024 18:16

Merge remote-tracking branch 'origin/main' into st_envelope

f91722e

craigtaverner added 11 commits November 19, 2024 15:27

Update docs/changelog/116964.yaml

9666a58

Merge remote-tracking branch 'origin/main' into st_envelope

e268188

Merge branch 'st_envelope' of github.com:craigtaverner/elasticsearch …

6eea94b

…into st_envelope

Started working on geo version of envelope calculation

6d8f7d9

This will allow wrapping longitude around the dateline. TODO: Write tests and use different Evaluator for the Geo version.

Merge remote-tracking branch 'origin/main' into st_envelope

17afd1a

After merging main, this code moved

0bcdea3

Added tests for SpatialEnvelopeVisitor

2f17bcc

Asserting in particular that the geo-wrapping over the dateline works.

Java docs for the SpatialEnvelopeVisitor

4aed3fc

Move SpatialEnvelopeVisitor to libs, and separate Geo from Cartesian

4560ac5

Merge remote-tracking branch 'origin/main' into st_envelope

150d736

Added missing generated code

80762bc

craigtaverner commented Nov 26, 2024

View reviewed changes

GalLalouche requested changes Nov 27, 2024

View reviewed changes

GalLalouche reviewed Nov 27, 2024

View reviewed changes

iverase approved these changes Nov 27, 2024

View reviewed changes

GalLalouche reviewed Nov 27, 2024

View reviewed changes

craigtaverner added 6 commits December 3, 2024 15:12

Merge remote-tracking branch 'origin/main' into st_envelope

9e9e66b

Update SpatialEnvelopeVisitor to version required for ST_EXTENT

96a845a

Merge remote-tracking branch 'origin/main' into st_envelope

3d6597a

Fix compile errors after merging in ST_EXTENT changes

088a8c5

Small updates after code review

9246abd

Merge branch 'main' into st_envelope

f48ebe2

GalLalouche approved these changes Dec 4, 2024

View reviewed changes

craigtaverner added auto-backport Automatically create backport pull requests when merged v8.18.0 labels Dec 4, 2024

craigtaverner merged commit c7e985c into elastic:main Dec 4, 2024
16 checks passed

luigidellaquila mentioned this pull request Dec 5, 2024

ES|QL: Fix ESQL usage output (telemetry) non-snapshot version test #118051

Merged

astefan added backport pending and removed auto-backport Automatically create backport pull requests when merged labels Dec 5, 2024

craigtaverner mentioned this pull request Dec 16, 2024

[8.x] Support ST_ENVELOPE and related ST_XMIN, etc. (#116964) #118743

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support ST_ENVELOPE and related ST_XMIN, etc. #116964

Support ST_ENVELOPE and related ST_XMIN, etc. #116964

craigtaverner commented Nov 18, 2024 •

edited

Loading

github-actions bot commented Nov 18, 2024

elasticsearchmachine commented Nov 18, 2024

elasticsearchmachine commented Nov 18, 2024

elasticsearchmachine commented Nov 19, 2024

craigtaverner Nov 26, 2024

iverase Nov 27, 2024

GalLalouche Nov 27, 2024

craigtaverner Dec 4, 2024

GalLalouche Nov 27, 2024

craigtaverner Dec 4, 2024

GalLalouche Nov 27, 2024

craigtaverner Dec 4, 2024

GalLalouche Nov 27, 2024

iverase Nov 27, 2024

craigtaverner Dec 4, 2024

GalLalouche Nov 27, 2024

craigtaverner Dec 4, 2024

GalLalouche Nov 27, 2024

GalLalouche Nov 27, 2024

iverase Nov 27, 2024

craigtaverner Dec 4, 2024

GalLalouche Nov 27, 2024

iverase left a comment

GalLalouche Nov 27, 2024 •

edited

Loading

craigtaverner Dec 4, 2024

Support ST_ENVELOPE and related ST_XMIN, etc. #116964

Support ST_ENVELOPE and related ST_XMIN, etc. #116964

Conversation

craigtaverner commented Nov 18, 2024 • edited Loading

github-actions bot commented Nov 18, 2024

elasticsearchmachine commented Nov 18, 2024

elasticsearchmachine commented Nov 18, 2024

elasticsearchmachine commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase left a comment

Choose a reason for hiding this comment

GalLalouche Nov 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

craigtaverner commented Nov 18, 2024 •

edited

Loading

GalLalouche Nov 27, 2024 •

edited

Loading