Browse Source

Fixes 92541 and 92562 (#92592)

An optimization in the GeoHexGridTiler has exceptions for polygons
with two bounding points within the polar cells, but not all points
within those cells. So when the cells are polar we do an additional
check for the other two bounds.

Fixes https://github.com/elastic/elasticsearch/issues/92541
Fixes https://github.com/elastic/elasticsearch/issues/92562
Craig Taverner 2 years ago
parent
commit
d012ee3eae

+ 28 - 9
x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/AbstractGeoHexGridTiler.java

@@ -8,6 +8,7 @@
 package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid;
 
 import org.elasticsearch.h3.H3;
+import org.elasticsearch.xpack.spatial.common.H3CartesianUtil;
 import org.elasticsearch.xpack.spatial.index.fielddata.GeoRelation;
 import org.elasticsearch.xpack.spatial.index.fielddata.GeoShapeValues;
 
@@ -48,10 +49,9 @@ abstract class AbstractGeoHexGridTiler extends GeoGridTiler {
         assert bounds.minX() <= bounds.maxX();
         // first check if we are touching just fetch cells
         if (bounds.maxX() - bounds.minX() < 180d) {
-            final long minH3 = H3.geoToH3(bounds.minY(), bounds.minX(), precision);
-            final long maxH3 = H3.geoToH3(bounds.maxY(), bounds.maxX(), precision);
-            if (minH3 == maxH3) {
-                return setValuesFromPointResolution(minH3, values, geoValue);
+            final long singleCell = boundsInSameCell(bounds, precision);
+            if (singleCell > 0) {
+                return setValuesFromPointResolution(singleCell, values, geoValue);
             }
             // TODO: specialize when they are neighbour cells.
         }
@@ -85,6 +85,25 @@ abstract class AbstractGeoHexGridTiler extends GeoGridTiler {
         return valueIndex;
     }
 
+    private long boundsInSameCell(GeoShapeValues.BoundingBox bounds, int res) {
+        final long minH3 = H3.geoToH3(bounds.minY(), bounds.minX(), res);
+        final long maxH3 = H3.geoToH3(bounds.maxY(), bounds.maxX(), res);
+        if (minH3 != maxH3) {
+            // Normally sufficient to check only bottom-left against top-right
+            return -1;
+        }
+        if (H3CartesianUtil.isPolar(minH3)) {
+            // But with polar cells we must check the other two corners too
+            final long minMax = H3.geoToH3(bounds.minY(), bounds.maxX(), res);
+            final long maxMin = H3.geoToH3(bounds.maxY(), bounds.minX(), res);
+            if (minMax != minH3 || maxMin != minH3) {
+                return -1;
+            }
+        }
+        // If all checks passed, we can use this cell in an optimization
+        return minH3;
+    }
+
     /**
      * Adds {@code h3} to {@link GeoShapeCellValues} if {@link #relateTile(GeoShapeValues.GeoShapeValue, long)} returns
      * a relation different to {@link GeoRelation#QUERY_DISJOINT}.
@@ -107,11 +126,11 @@ abstract class AbstractGeoHexGridTiler extends GeoGridTiler {
         // NOTE: When we recurse, we cannot shortcut for CONTAINS relationship because it might fail when visiting noChilds.
         int valueIndex = 0;
         if (bounds.maxX() - bounds.minX() < 180d) {
-            final long minH3 = H3.geoToH3(bounds.minY(), bounds.minX(), 0);
-            final long maxH3 = H3.geoToH3(bounds.maxY(), bounds.maxX(), 0);
-            if (minH3 == maxH3) {
-                valueIndex = setValuesByRecursion(values, geoValue, minH3, 0, valueIndex);
-                for (long n : H3.hexRing(minH3)) {
+            final long singleCell = boundsInSameCell(bounds, 0);
+            if (singleCell > 0) {
+                // When the level 0 bounds are within a single cell, we can search that cell and its immediate neighbours
+                valueIndex = setValuesByRecursion(values, geoValue, singleCell, 0, valueIndex);
+                for (long n : H3.hexRing(singleCell)) {
                     valueIndex = setValuesByRecursion(values, geoValue, n, 0, valueIndex);
                 }
                 return valueIndex;

+ 82 - 10
x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexTilerTests.java

@@ -15,6 +15,7 @@ import org.elasticsearch.geo.GeometryTestUtils;
 import org.elasticsearch.geometry.Geometry;
 import org.elasticsearch.geometry.Point;
 import org.elasticsearch.geometry.Rectangle;
+import org.elasticsearch.geometry.utils.StandardValidator;
 import org.elasticsearch.geometry.utils.WellKnownText;
 import org.elasticsearch.h3.H3;
 import org.elasticsearch.xpack.spatial.common.H3CartesianUtil;
@@ -59,16 +60,6 @@ public class GeoHexTilerTests extends GeoGridTilerTestCase {
         return UnboundedGeoHexGridTiler.calcMaxAddresses(precisionDiff);
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/92541")
-    public void testGeoGridSetValuesBoundingBoxes_BoundedGeoShapeCellValues() throws Exception {
-        super.testGeoGridSetValuesBoundingBoxes_BoundedGeoShapeCellValues();
-    }
-
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/92562")
-    public void testGeoGridSetValuesBoundingBoxes_UnboundedGeoShapeCellValues() throws Exception {
-        super.testGeoGridSetValuesBoundingBoxes_UnboundedGeoShapeCellValues();
-    }
-
     public void testLargeShape() throws Exception {
         // We have a shape and a tile both covering all mercator space, so we expect all level0 H3 cells to match
         Rectangle shapeRectangle = new Rectangle(-180, 180, 90, -90);
@@ -128,6 +119,87 @@ public class GeoHexTilerTests extends GeoGridTilerTestCase {
         }
     }
 
+    // Polygons with bounds inside the South Pole cell break a tiler optimization
+    public void testTroublesomeShapeAlmostWithinSouthPole_BoundedGeoShapeCellValues() throws Exception {
+        int precision = 1;
+        String polygon = """
+            POLYGON((180.0 -90.0, 180.0 -73.80002960532788, 1.401298464324817E-45 -73.80002960532788,
+            1.401298464324817E-45 -90.0, 180.0 -90.0))""";
+        GeoBoundingBox geoBoundingBox = new GeoBoundingBox(
+            new GeoPoint(19.585157879020088, 0.9999999403953552),
+            new GeoPoint(-90.0, -26.405694642531472)
+        );
+        Geometry geometry = WellKnownText.fromWKT(StandardValidator.instance(true), true, polygon);
+        GeoShapeValues.GeoShapeValue value = geoShapeValue(geometry);
+        GeoShapeCellValues cellValues = new GeoShapeCellValues(
+            makeGeoShapeValues(value),
+            getBoundedGridTiler(geoBoundingBox, precision),
+            NOOP_BREAKER
+        );
+
+        assertTrue(cellValues.advanceExact(0));
+        int numBuckets = cellValues.docValueCount();
+        int expected = expectedBuckets(value, precision, geoBoundingBox);
+        assertThat("[" + precision + "] bucket count", numBuckets, equalTo(expected));
+    }
+
+    // Polygons with bounds inside the South Pole cell break a tiler optimization
+    public void testTroublesomeShapeAlmostWithinSouthPoleCell_UnboundedGeoShapeCellValues() throws Exception {
+        int precision = 0;
+        String polygon = """
+            POLYGON((1.7481549674935762E-110 -90.0, 180.0 -90.0, 180.0 -75.113250736563,
+            1.7481549674935762E-110 -75.113250736563, 1.7481549674935762E-110 -90.0))""";
+        Geometry geometry = WellKnownText.fromWKT(StandardValidator.instance(true), true, polygon);
+        GeoShapeValues.GeoShapeValue value = geoShapeValue(geometry);
+        GeoShapeCellValues unboundedCellValues = new GeoShapeCellValues(
+            makeGeoShapeValues(value),
+            getUnboundedGridTiler(precision),
+            NOOP_BREAKER
+        );
+
+        assertTrue(unboundedCellValues.advanceExact(0));
+        int numBuckets = unboundedCellValues.docValueCount();
+        int expected = expectedBuckets(value, precision, null);
+        assertThat("[" + precision + "] bucket count", numBuckets, equalTo(expected));
+    }
+
+    // Polygons with bounds inside the North Pole cell break a tiler optimization
+    public void testTroublesomeShapeAlmostWithinNorthPoleCell_UnboundedGeoShapeCellValues() throws Exception {
+        int precision = 1;
+        String polygon = """
+            POLYGON((36.98661841690625 69.44049730644747, 180.0 69.44049730644747,
+            180.0 90.0, 36.98661841690625 90.0, 36.98661841690625 69.44049730644747))""";
+        Geometry geometry = WellKnownText.fromWKT(StandardValidator.instance(true), true, polygon);
+        GeoShapeValues.GeoShapeValue value = geoShapeValue(geometry);
+        GeoShapeCellValues unboundedCellValues = new GeoShapeCellValues(
+            makeGeoShapeValues(value),
+            getUnboundedGridTiler(precision),
+            NOOP_BREAKER
+        );
+
+        assertTrue(unboundedCellValues.advanceExact(0));
+        int numBuckets = unboundedCellValues.docValueCount();
+        int expected = expectedBuckets(value, precision, null);
+        assertThat("[" + precision + "] bucket count", numBuckets, equalTo(expected));
+    }
+
+    public void testTroublesomePolarCellLevel1_UnboundedGeoShapeCellValues() throws Exception {
+        int precision = 1;
+        String polygon = "BBOX (-84.24596376729815, 43.36113427778119, 90.0, 83.51476833522361)";
+        Geometry geometry = WellKnownText.fromWKT(StandardValidator.instance(true), true, polygon);
+        GeoShapeValues.GeoShapeValue value = geoShapeValue(geometry);
+        GeoShapeCellValues unboundedCellValues = new GeoShapeCellValues(
+            makeGeoShapeValues(value),
+            getUnboundedGridTiler(precision),
+            NOOP_BREAKER
+        );
+
+        assertTrue(unboundedCellValues.advanceExact(0));
+        int numBuckets = unboundedCellValues.docValueCount();
+        int expected = expectedBuckets(value, precision, null);
+        assertThat("[" + precision + "] bucket count", numBuckets, equalTo(expected));
+    }
+
     private void assertCorner(long[] h3bins, Point point, int precision, String msg) throws IOException {
         GeoShapeValues.GeoShapeValue cornerValue = geoShapeValue(point);
         GeoShapeCellValues cornerValues = new GeoShapeCellValues(