Grid aggregations with bounds should exclude touching tiles (#72295)

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

@@ -173,18 +173,4 @@ abstract class AbstractGeoTileGridTiler extends GeoGridTiler {
         }
         return valuesIndex;
     }
-
-    /**
-     * Return the number of tiles contained in the provided bounding box at the given zoom level
-     */
-    protected static long numTilesFromPrecision(int zoom, double minX, double maxX, double minY, double maxY) {
-        final long tiles = 1L << zoom;
-        final double xDeltaPrecision = 360.0 / (10 * tiles);
-        final double yHalfPrecision = 180.0 / (10 * tiles);
-        final int minXTile = GeoTileUtils.getXTile(Math.max(-180, minX - xDeltaPrecision), tiles);
-        final int minYTile = GeoTileUtils.getYTile(maxY + yHalfPrecision, tiles);
-        final int maxXTile = GeoTileUtils.getXTile(Math.min(180, maxX + xDeltaPrecision), tiles);
-        final int maxYTile = GeoTileUtils.getYTile(minY - yHalfPrecision, tiles);
-        return (long) (maxXTile - minXTile + 1) * (maxYTile - minYTile + 1);
-    }
 }

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

@@ -26,12 +26,13 @@ public class BoundedGeoHashGridTiler extends AbstractGeoHashGridTiler {
 
     @Override
     protected boolean validHash(String hash) {
-        Rectangle rectangle = Geohash.toBoundingBox(hash);
-        if (bbox.top() >= rectangle.getMinY() && bbox.bottom() <= rectangle.getMaxY()) {
+        final Rectangle rectangle = Geohash.toBoundingBox(hash);
+        // touching hashes are excluded
+        if (bbox.top() > rectangle.getMinY() && bbox.bottom() < rectangle.getMaxY()) {
             if (crossesDateline) {
-                return bbox.left() <= rectangle.getMaxX() || bbox.right() >= rectangle.getMinX();
+                return bbox.left() < rectangle.getMaxX() || bbox.right() > rectangle.getMinX();
             } else {
-                return bbox.left() <= rectangle.getMaxX() && bbox.right() >= rectangle.getMinX();
+                return bbox.left() < rectangle.getMaxX() && bbox.right() > rectangle.getMinX();
             }
         }
         return false;

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

@@ -16,31 +16,45 @@ import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils;
  * Bounded geotile aggregation. It accepts tiles that intersects the provided bounds.
  */
 public class BoundedGeoTileGridTiler extends AbstractGeoTileGridTiler {
-    private final GeoBoundingBox bbox;
     private final boolean crossesDateline;
     private final long maxTiles;
+    private final int minX, maxX, minY, maxY;
 
     public BoundedGeoTileGridTiler(int precision, GeoBoundingBox bbox) {
         super(precision);
-        this.bbox = bbox;
         this.crossesDateline = bbox.right() < bbox.left();
+        // compute minX, minY
+        final int minX = GeoTileUtils.getXTile(bbox.left(), this.tiles);
+        final int minY = GeoTileUtils.getYTile(bbox.top(), this.tiles);
+        final Rectangle minTile = GeoTileUtils.toBoundingBox(minX, minY, precision);
+        // touching tiles are excluded, they need to share at least one interior point
+        this.minX = minTile.getMaxX() == bbox.left() ? minX + 1: minX;
+        this.minY = minTile.getMinY() == bbox.top() ? minY + 1 : minY;
+        // compute maxX, maxY
+        final int maxX = GeoTileUtils.getXTile(bbox.right(), this.tiles);
+        final int maxY = GeoTileUtils.getYTile(bbox.bottom(), this.tiles);
+        final Rectangle maxTile = GeoTileUtils.toBoundingBox(maxX, maxY, precision);
+        // touching tiles are excluded, they need to share at least one interior point
+        this.maxX = maxTile.getMinX() == bbox.right() ? maxX - 1 : maxX;
+        this.maxY = maxTile.getMaxY() == bbox.bottom() ? maxY - 1 : maxY;
         if (crossesDateline) {
-            maxTiles =  numTilesFromPrecision(precision, bbox.left(), 180, bbox.bottom(), bbox.top())
-                + numTilesFromPrecision(precision, -180, bbox.right(), bbox.bottom(), bbox.top());
-
+            this.maxTiles = (tiles + this.maxX - this.minX + 1) * (this.maxY - this.minY + 1);
         } else {
-            maxTiles = numTilesFromPrecision(precision, bbox.left(), bbox.right(), bbox.bottom(), bbox.top());
+            this.maxTiles = (long) (this.maxX - this.minX + 1) * (this.maxY - this.minY + 1);
         }
     }
 
     @Override
     protected boolean validTile(int x, int y, int z) {
-        Rectangle rectangle = GeoTileUtils.toBoundingBox(x, y, z);
-        if (bbox.top() >= rectangle.getMinY() && bbox.bottom() <= rectangle.getMaxY()) {
+        // compute number of splits at precision
+        final int splits = 1 << precision - z;
+        final int yMin = y * splits;
+        if (maxY >= yMin && minY < yMin + splits) {
+            final int xMin = x * splits;
             if (crossesDateline) {
-                return bbox.left() <= rectangle.getMaxX() || bbox.right() >= rectangle.getMinX();
+                return maxX >= xMin || minX < xMin + splits;
             } else {
-                return bbox.left() <= rectangle.getMaxX() && bbox.right() >= rectangle.getMinX();
+                return maxX >= xMin && minX < xMin + splits;
             }
         }
         return false;

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

@@ -7,6 +7,7 @@
 
 package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid;
 
+import org.apache.lucene.geo.GeoTestUtil;
 import org.elasticsearch.common.breaker.CircuitBreaker;
 import org.elasticsearch.common.breaker.CircuitBreakingException;
 import org.elasticsearch.common.geo.GeoBoundingBox;
@@ -225,6 +226,58 @@ public class GeoGridTilerTests extends ESTestCase {
         }
     }
 
+    public void testGeoHashBoundsExcludeTouchingTiles() throws Exception {
+        final int precision = randomIntBetween(1, 5);
+        final String hash =
+            Geohash.stringEncode(GeoTestUtil.nextLongitude(), GeoTestUtil.nextLatitude(), precision);
+
+        final Rectangle rectangle = Geohash.toBoundingBox(hash);
+        final GeoBoundingBox box = new GeoBoundingBox(
+            new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()),
+            new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())
+        );
+        final Rectangle other = new Rectangle(
+            Math.max(-180, rectangle.getMinX() - 1),
+            Math.min(180, rectangle.getMaxX() + 1),
+            Math.min(90, rectangle.getMaxY() + 1),
+            Math.max(-90, rectangle.getMinY() - 1));
+        final GeoShapeValues.GeoShapeValue value = geoShapeValue(other);
+        for (int i = 0;  i < 4; i++) {
+            final BoundedGeoHashGridTiler bounded = new BoundedGeoHashGridTiler(precision + i, box);
+            final GeoShapeCellValues values = new GeoShapeCellValues(makeGeoShapeValues(value), bounded, NOOP_BREAKER);
+            assertTrue(values.advanceExact(0));
+            final int numTiles = values.docValueCount();
+            final int expected = (int) Math.pow(32, i);
+            assertThat(numTiles, equalTo(expected));
+        }
+    }
+
+    public void testGeoTileBoundsExcludeTouchingTiles() throws Exception {
+        final int z = randomIntBetween(1, GeoTileUtils.MAX_ZOOM - 10);
+        final int x = randomIntBetween(0, (1 << z) - 1);
+        final int y = randomIntBetween(0, (1 << z) - 1);
+        final Rectangle rectangle = GeoTileUtils.toBoundingBox(x, y, z);
+        final GeoBoundingBox box = new GeoBoundingBox(
+            new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()),
+            new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())
+        );
+        final Rectangle other = new Rectangle(
+            Math.max(-180, rectangle.getMinX() - 1),
+            Math.min(180, rectangle.getMaxX() + 1),
+            Math.min(90, rectangle.getMaxY() + 1),
+            Math.max(-90, rectangle.getMinY() - 1));
+        final GeoShapeValues.GeoShapeValue value = geoShapeValue(other);
+        for (int i = 0;  i < 10; i++) {
+            final BoundedGeoTileGridTiler bounded = new BoundedGeoTileGridTiler(z + i, box);
+            final GeoShapeCellValues values = new GeoShapeCellValues(makeGeoShapeValues(value), bounded, NOOP_BREAKER);
+            assertTrue(values.advanceExact(0));
+            final int numTiles = values.docValueCount();
+            final int expected = 1 << (2 * i);
+            assertThat(numTiles, equalTo(expected));
+            assertThat((int) bounded.getMaxTiles(), equalTo(expected));
+        }
+    }
+
     public void testGeoTileShapeContainsBound() throws Exception {
         Rectangle tile = GeoTileUtils.toBoundingBox(44140, 44140, 16);
         Rectangle shapeRectangle = new Rectangle(tile.getMinX() - 15, tile.getMaxX() + 15,
@@ -235,12 +288,32 @@ public class GeoGridTilerTests extends ESTestCase {
             new GeoPoint(tile.getMaxLat(), tile.getMinLon()),
             new GeoPoint(tile.getMinLat(), tile.getMaxLon())
         );
+        BoundedGeoTileGridTiler tiler = new BoundedGeoTileGridTiler(24, boundingBox);
+        GeoShapeCellValues values =
+            new GeoShapeCellValues(makeGeoShapeValues(value), tiler, NOOP_BREAKER);
+        assertTrue(values.advanceExact(0));
+        int numTiles = values.docValueCount();
+        int expectedTiles = Math.toIntExact(tiler.getMaxTiles());
+        assertThat(numTiles, equalTo(expectedTiles));
+        assertThat(numTiles, equalTo(256 * 256));
+    }
+
+    public void testGeoTileShapeContainsBound3() throws Exception {
+        Rectangle tile = GeoTileUtils.toBoundingBox(2, 2, 3);
+        Rectangle shapeRectangle = new Rectangle(tile.getMinX() - 1, tile.getMaxX() + 1,
+            tile.getMaxY() + 1, tile.getMinY() - 1);
+        GeoShapeValues.GeoShapeValue value = geoShapeValue(shapeRectangle);
+
+        GeoBoundingBox boundingBox = new GeoBoundingBox(
+            new GeoPoint(tile.getMaxLat(), tile.getMinLon()),
+            new GeoPoint(tile.getMinLat(), tile.getMaxLon())
+        );
+        BoundedGeoTileGridTiler tiler = new BoundedGeoTileGridTiler(4, boundingBox);
         GeoShapeCellValues values =
-            new GeoShapeCellValues(makeGeoShapeValues(value), new BoundedGeoTileGridTiler(24, boundingBox), NOOP_BREAKER);
+            new GeoShapeCellValues(makeGeoShapeValues(value), tiler, NOOP_BREAKER);
         assertTrue(values.advanceExact(0));
         int numTiles = values.docValueCount();
-        int expectedTiles =
-            (int) AbstractGeoTileGridTiler.numTilesFromPrecision(24, tile.getMinX(), tile.getMaxX(), tile.getMinY(), tile.getMaxY());
+        int expectedTiles = Math.toIntExact(tiler.getMaxTiles());
         assertThat(expectedTiles, equalTo(numTiles));
     }
 
@@ -253,43 +326,47 @@ public class GeoGridTilerTests extends ESTestCase {
             new GeoPoint(tile.getMaxLat(), tile.getMinLon()),
             new GeoPoint(tile.getMinLat(), tile.getMaxLon())
         );
+        BoundedGeoTileGridTiler tiler = new BoundedGeoTileGridTiler(13, boundingBox);
         GeoShapeCellValues values =
             new GeoShapeCellValues(makeGeoShapeValues(value), new BoundedGeoTileGridTiler(13, boundingBox), NOOP_BREAKER);
         assertTrue(values.advanceExact(0));
         int numTiles = values.docValueCount();
-        int expectedTiles = (int) (AbstractGeoTileGridTiler.numTilesFromPrecision(13, 178, 180, -2, 2)
-            + AbstractGeoTileGridTiler.numTilesFromPrecision(13, -180, -178, -2, 2));
+        int expectedTiles = Math.toIntExact(tiler.getMaxTiles());
         assertThat(expectedTiles, equalTo(numTiles));
     }
 
-    private boolean tileIntersectsBounds(int x, int y, int precision, GeoBoundingBox bounds) {
-        if (bounds == null) {
+    private boolean tileIntersectsBounds(int x, int y, int precision, GeoBoundingBox bbox) {
+        if (bbox == null) {
             return true;
         }
-        final double boundsWestLeft;
-        final double boundsWestRight;
-        final double boundsEastLeft;
-        final double boundsEastRight;
-        final boolean crossesDateline;
-        if (bounds.right() < bounds.left()) {
-            boundsWestLeft = -180;
-            boundsWestRight = bounds.right();
-            boundsEastLeft = bounds.left();
-            boundsEastRight = 180;
-            crossesDateline = true;
-        } else {
-            boundsEastLeft = bounds.left();
-            boundsEastRight = bounds.right();
-            boundsWestLeft = 0;
-            boundsWestRight = 0;
-            crossesDateline = false;
+        final int tiles = 1 << precision;
+        int minX = GeoTileUtils.getXTile(bbox.left(), tiles);
+        int minY = GeoTileUtils.getYTile(bbox.top(), tiles);
+        final Rectangle minTile = GeoTileUtils.toBoundingBox(minX, minY, precision);
+        if (minTile.getMaxX() == bbox.left()) {
+            minX++;
         }
-
-        Rectangle tile = GeoTileUtils.toBoundingBox(x, y, precision);
-
-        return (bounds.top() >= tile.getMinY() && bounds.bottom() <= tile.getMaxY()
-            && (boundsEastLeft <= tile.getMaxX() && boundsEastRight >= tile.getMinX()
-            || (crossesDateline && boundsWestLeft <= tile.getMaxX() && boundsWestRight >= tile.getMinX())));
+        if (minTile.getMinY() == bbox.top()) {
+            minY++;
+        }
+        // compute maxX, maxY
+        int maxX = GeoTileUtils.getXTile(bbox.right(), tiles);
+        int maxY = GeoTileUtils.getYTile(bbox.bottom(), tiles);
+        final Rectangle maxTile = GeoTileUtils.toBoundingBox(maxX, maxY, precision);
+        if (maxTile.getMinX() == bbox.right()) {
+            maxX--;
+        }
+        if (maxTile.getMaxY() == bbox.bottom()) {
+            maxY--;
+        }
+        if (maxY >= y && minY <= y) {
+            if (bbox.left() > bbox.right()) {
+                return maxX >= x || minX <= x;
+            } else {
+                return maxX >= x && minX <= x;
+            }
+        }
+        return false;
     }
 
     private int numTiles(GeoShapeValues.GeoShapeValue geoValue, int precision, GeoBoundingBox geoBox) {

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

@@ -191,9 +191,9 @@ public abstract class GeoShapeGeoGridTestCase<T extends InternalGeoGridBucket<T>
 
             GeoShapeValues.GeoShapeValue value = geoShapeValue(p);
             GeoRelation tileRelation =  value.relate(pointTile);
-            boolean intersectsBounds = boundsTop >= pointTile.getMinY() && boundsBottom <= pointTile.getMaxY()
-                && (boundsEastLeft <= pointTile.getMaxX() && boundsEastRight >= pointTile.getMinX()
-                || (crossesDateline && boundsWestLeft <= pointTile.getMaxX() && boundsWestRight >= pointTile.getMinX()));
+            boolean intersectsBounds = boundsTop > pointTile.getMinY() && boundsBottom < pointTile.getMaxY()
+                && (boundsEastLeft < pointTile.getMaxX() && boundsEastRight > pointTile.getMinX()
+                || (crossesDateline && boundsWestLeft < pointTile.getMaxX() && boundsWestRight > pointTile.getMinX()));
             if (tileRelation != GeoRelation.QUERY_DISJOINT && intersectsBounds) {
                 numDocsWithin += 1;
             }