Handle bounds properly when grid tiles crosses the dateline

docs/changelog/83348.yaml

@@ -0,0 +1,6 @@
+pr: 83348
+summary: Handle bounds properly when grid tiles crosses the dateline
+area: Geo
+type: bug
+issues:
+ - 83299

server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java

@@ -85,21 +85,45 @@ private static class BoundedCellValues extends CellValues {
         @Override
         protected int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) {
             final String hash = Geohash.stringEncode(target.getLon(), target.getLat(), precision);
-            if (validHash(hash)) {
+            if (validPoint(target.getLon(), target.getLat()) || validHash(hash)) {
                 values[valuesIdx] = Geohash.longEncode(hash);
                 return valuesIdx + 1;
             }
             return valuesIdx;
         }
 
         private boolean validHash(String hash) {
-            final Rectangle rectangle = Geohash.toBoundingBox(hash);
+            final Rectangle rect = Geohash.toBoundingBox(hash);
+            // hashes should not cross in theory the dateline but due to precision
+            // errors and normalization computing the hash, it might happen that they actually
+            // crosses the dateline.
+            if (rect.getMaxX() < rect.getMinX()) {
+                return intersects(-180, rect.getMaxX(), rect.getMinY(), rect.getMaxY())
+                    || intersects(rect.getMinX(), 180, rect.getMinY(), rect.getMaxY());
+            } else {
+                return intersects(rect.getMinX(), rect.getMaxX(), rect.getMinY(), rect.getMaxY());
+            }
+        }
+
+        private boolean intersects(double minX, double maxX, double minY, double maxY) {
             // touching hashes are excluded
-            if (bbox.top() > rectangle.getMinY() && bbox.bottom() < rectangle.getMaxY()) {
+            if (bbox.top() > minY && bbox.bottom() < maxY) {
+                if (crossesDateline) {
+                    return bbox.left() < maxX || bbox.right() > minX;
+                } else {
+                    return bbox.left() < maxX && bbox.right() > minX;
+                }
+            }
+            return false;
+        }
+
+        private boolean validPoint(double x, double y) {
+            if (bbox.top() > y && bbox.bottom() < y) {
+                boolean crossesDateline = bbox.left() > bbox.right();
                 if (crossesDateline) {
-                    return bbox.left() < rectangle.getMaxX() || bbox.right() > rectangle.getMinX();
+                    return bbox.left() < x || bbox.right() > x;
                 } else {
-                    return bbox.left() < rectangle.getMaxX() && bbox.right() > rectangle.getMinX();
+                    return bbox.left() < x && bbox.right() > x;
                 }
             }
             return false;

test/framework/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java

@@ -229,26 +229,6 @@ public void testBounds() throws IOException {
         expectThrows(IllegalArgumentException.class, () -> builder.precision(30));
 
         GeoBoundingBox bbox = randomBBox();
-        final double boundsTop = bbox.top();
-        final double boundsBottom = bbox.bottom();
-        final double boundsWestLeft;
-        final double boundsWestRight;
-        final double boundsEastLeft;
-        final double boundsEastRight;
-        final boolean crossesDateline;
-        if (bbox.right() < bbox.left()) {
-            boundsWestLeft = -180;
-            boundsWestRight = bbox.right();
-            boundsEastLeft = bbox.left();
-            boundsEastRight = 180;
-            crossesDateline = true;
-        } else { // only set east bounds
-            boundsEastLeft = bbox.left();
-            boundsEastRight = bbox.right();
-            boundsWestLeft = 0;
-            boundsWestRight = 0;
-            crossesDateline = false;
-        }
 
         Function<Double, Double> encodeDecodeLat = (lat) -> GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(lat));
         Function<Double, Double> encodeDecodeLon = (lon) -> GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(lon));
@@ -260,11 +240,7 @@ public void testBounds() throws IOException {
             double x = encodeDecodeLon.apply(p.getLon());
             double y = encodeDecodeLat.apply(p.getLat());
             Rectangle pointTile = getTile(x, y, precision);
-            boolean intersectsBounds = boundsTop > pointTile.getMinY()
-                && boundsBottom < pointTile.getMaxY()
-                && (boundsEastLeft < pointTile.getMaxX() && boundsEastRight > pointTile.getMinX()
-                    || (crossesDateline && boundsWestLeft < pointTile.getMaxX() && boundsWestRight > pointTile.getMinX()));
-            if (intersectsBounds) {
+            if (intersectsBounds(pointTile, bbox) || validPoint(x, y, bbox)) {
                 in++;
             }
             docs.add(new LatLonDocValuesField(FIELD_NAME, p.getLat(), p.getLon()));
@@ -289,7 +265,36 @@ public void testBounds() throws IOException {
         });
     }
 
-    private void testCase(
+    private boolean validPoint(double x, double y, GeoBoundingBox bbox) {
+        if (bbox.top() > y && bbox.bottom() < y) {
+            boolean crossesDateline = bbox.left() > bbox.right();
+            if (crossesDateline) {
+                return bbox.left() < x || bbox.right() > x;
+            } else {
+                return bbox.left() < x && bbox.right() > x;
+            }
+        }
+        return false;
+    }
+
+    private boolean intersectsBounds(Rectangle pointTile, GeoBoundingBox bbox) {
+        if (pointTile.getMinX() > pointTile.getMaxX()) {
+            Rectangle right = new Rectangle(pointTile.getMinX(), 180, pointTile.getMaxY(), pointTile.getMinY());
+            Rectangle left = new Rectangle(-180, pointTile.getMaxX(), pointTile.getMaxY(), pointTile.getMinY());
+            return intersectsBounds(left, bbox) || intersectsBounds(right, bbox);
+        }
+        if (bbox.top() > pointTile.getMinY() && bbox.bottom() < pointTile.getMaxY()) {
+            boolean crossesDateline = bbox.left() > bbox.right();
+            if (crossesDateline) {
+                return bbox.left() < pointTile.getMaxX() || bbox.right() > pointTile.getMinX();
+            } else {
+                return bbox.left() < pointTile.getMaxX() && bbox.right() > pointTile.getMinX();
+            }
+        }
+        return false;
+    }
+
+    protected void testCase(
         Query query,
         String field,
         int precision,
@@ -322,7 +327,7 @@ private void testCase(
             assertThat(aggregationBuilder.geoBoundingBox(), equalTo(geoBoundingBox));
         }
 
-        MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType(FIELD_NAME);
+        MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType(aggregationBuilder.field());
 
         Aggregator aggregator = createAggregator(aggregationBuilder, indexSearcher, fieldType);
         aggregator.preCollection();

x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexCellIdSource.java

@@ -96,11 +96,11 @@ public int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int values
         }
 
         private boolean validPoint(double lat, double lon) {
-            if (bbox.top() >= lat && bbox.bottom() <= lat) {
+            if (bbox.top() > lat && bbox.bottom() < lat) {
                 if (crossesDateline) {
-                    return bbox.left() <= lon || bbox.right() >= lon;
+                    return bbox.left() < lon || bbox.right() > lon;
                 } else {
-                    return bbox.left() <= lon && bbox.right() >= lon;
+                    return bbox.left() < lon && bbox.right() > lon;
                 }
             }
             return false;
@@ -120,6 +120,14 @@ private boolean validHex(long hex) {
                 minLat = Math.min(minLat, boundaryLat);
                 maxLat = Math.max(maxLat, boundaryLat);
             }
+            if (maxLon - minLon > 180) {
+                return intersects(-180, minLon, minLat, maxLat) || intersects(maxLon, 180, minLat, maxLat);
+            } else {
+                return intersects(minLon, maxLon, minLat, maxLat);
+            }
+        }
+
+        private boolean intersects(double minLon, double maxLon, double minLat, double maxLat) {
             if (bbox.top() > minLat && bbox.bottom() < maxLat) {
                 if (crossesDateline) {
                     return bbox.left() < maxLon || bbox.right() > minLon;

x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexAggregatorTests.java

@@ -6,7 +6,12 @@
  */
 package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid;
 
+import org.apache.lucene.document.LatLonDocValuesField;
+import org.apache.lucene.geo.GeoEncodingUtils;
+import org.apache.lucene.search.MatchAllDocsQuery;
 import org.elasticsearch.common.geo.GeoBoundingBox;
+import org.elasticsearch.common.geo.GeoPoint;
+import org.elasticsearch.common.geo.GeoUtils;
 import org.elasticsearch.geo.GeometryTestUtils;
 import org.elasticsearch.geometry.Point;
 import org.elasticsearch.geometry.Rectangle;
@@ -17,12 +22,15 @@
 import org.elasticsearch.search.aggregations.AggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregatorTestCase;
+import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils;
+import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper;
 import org.elasticsearch.search.aggregations.support.CoreValuesSourceType;
 import org.elasticsearch.search.aggregations.support.ValuesSourceType;
 import org.elasticsearch.xpack.spatial.LocalStateSpatialPlugin;
 import org.elasticsearch.xpack.spatial.search.aggregations.support.GeoShapeValuesSourceType;
-import org.elasticsearch.xpack.spatial.util.GeoTestUtils;
 
+import java.io.IOException;
+import java.util.Collections;
 import java.util.List;
 
 public class GeoHexAggregatorTests extends GeoGridAggregatorTestCase<InternalGeoHexGridBucket> {
@@ -39,7 +47,8 @@ protected List<ValuesSourceType> getSupportedValuesSourceTypes() {
 
     @Override
     protected int randomPrecision() {
-        return randomIntBetween(0, H3.MAX_H3_RES);
+        // avoid too big cells, so we don't go over the pole
+        return randomIntBetween(2, H3.MAX_H3_RES);
     }
 
     @Override
@@ -54,12 +63,33 @@ protected GeoGridAggregationBuilder createBuilder(String name) {
 
     @Override
     protected Point randomPoint() {
-        return GeometryTestUtils.randomPoint();
+        // don't go close to the poles
+        return new Point(
+            randomDoubleBetween(GeoUtils.MIN_LON, GeoUtils.MAX_LON, true),
+            randomDoubleBetween(-GeoTileUtils.LATITUDE_MASK, GeoTileUtils.LATITUDE_MASK, false)
+        );
     }
 
     @Override
     protected GeoBoundingBox randomBBox() {
-        return GeoTestUtils.randomBBox();
+        GeoBoundingBox bbox = randomValueOtherThanMany(
+            (b) -> b.top() > GeoTileUtils.LATITUDE_MASK || b.bottom() < -GeoTileUtils.LATITUDE_MASK,
+            () -> {
+                Rectangle rectangle = GeometryTestUtils.randomRectangle();
+                return new GeoBoundingBox(
+                    new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()),
+                    new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())
+                );
+            }
+        );
+        // Avoid numerical errors for sub-atomic values
+        double left = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(bbox.left()));
+        double right = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(bbox.right()));
+        double top = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(bbox.top()));
+        double bottom = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(bbox.bottom()));
+        bbox.topLeft().reset(top, left);
+        bbox.bottomRight().reset(bottom, right);
+        return bbox;
     }
 
     @Override
@@ -77,11 +107,30 @@ protected Rectangle getTile(double lng, double lat, int precision) {
             minLat = Math.min(minLat, boundaryLat);
             maxLat = Math.max(maxLat, boundaryLat);
         }
-        return new Rectangle(minLon, maxLon, maxLat, minLat);
+        if (maxLon - minLon > 180) {
+            return new Rectangle(maxLon, minLon, maxLat, minLat);
+        } else {
+            return new Rectangle(minLon, maxLon, maxLat, minLat);
+        }
     }
 
     @Override
     protected AggregationBuilder createAggBuilderForTypeTest(MappedFieldType fieldType, String fieldName) {
         return createBuilder("foo").field(fieldName);
     }
+
+    public void testHexCrossesDateline() throws IOException {
+        GeoBoundingBox bbox = new GeoBoundingBox(new GeoPoint(10, 179.5), new GeoPoint(0, 179.6));
+        double y = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(5));
+        double x = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(179.9));
+        LatLonDocValuesField field = new LatLonDocValuesField("bar", y, x);
+        testCase(
+            new MatchAllDocsQuery(),
+            "bar",
+            0,
+            bbox,
+            geoGrid -> assertTrue(AggregationInspectionHelper.hasValue(geoGrid)),
+            iw -> iw.addDocument(Collections.singletonList(field))
+        );
+    }
 }