Browse Source

Do not over-allocate when resizing in GeoHashTiler with bounds (#72539)

Ignacio Vera 4 years ago
parent
commit
893d4efc41

+ 21 - 1
x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/AbstractGeoHashGridTiler.java

@@ -23,6 +23,9 @@ abstract class AbstractGeoHashGridTiler extends GeoGridTiler {
     /** check if the provided hash is in the solution space of this tiler */
     protected abstract boolean validHash(String hash);
 
+    /** Max size of the solution space */
+    protected abstract long getMaxHashes();
+
     @Override
     public long encode(double x, double y) {
         return Geohash.longEncode(x, y, precision);
@@ -105,7 +108,8 @@ abstract class AbstractGeoHashGridTiler extends GeoGridTiler {
                     values.resizeCell(valuesIndex + 1);
                     values.add(valuesIndex++, Geohash.longEncode(hashes[i]));
                 } else {
-                    values.resizeCell(valuesIndex + (int) Math.pow(32, precision - hash.length()) + 1);
+                    int numTilesAtPrecision = getNumTilesAtPrecision(precision, hash.length());
+                    values.resizeCell(getNewSize(valuesIndex, numTilesAtPrecision + 1));
                     valuesIndex = setValuesForFullyContainedTile(hashes[i],values, valuesIndex, precision);
                 }
             }
@@ -113,6 +117,22 @@ abstract class AbstractGeoHashGridTiler extends GeoGridTiler {
         return valuesIndex;
     }
 
+    private int getNewSize(int valuesIndex, int increment) {
+        long newSize  = (long) valuesIndex + increment;
+        if (newSize > Integer.MAX_VALUE) {
+            throw new IllegalArgumentException("Tile aggregation array overflow");
+        }
+        return (int) newSize;
+    }
+
+    private int getNumTilesAtPrecision(int finalPrecision, int currentPrecision) {
+        final long numTilesAtPrecision  = Math.min((long) Math.pow(32, finalPrecision - currentPrecision) + 1, getMaxHashes());
+        if (numTilesAtPrecision > Integer.MAX_VALUE) {
+            throw new IllegalArgumentException("Tile aggregation array overflow");
+        }
+        return (int) numTilesAtPrecision;
+    }
+
     protected int setValuesForFullyContainedTile(String hash, GeoShapeCellValues values,
                                                  int valuesIndex, int targetPrecision) {
         String[] hashes = Geohash.getSubGeohashes(hash);

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

@@ -17,11 +17,25 @@ import org.elasticsearch.geometry.utils.Geohash;
 public class BoundedGeoHashGridTiler extends AbstractGeoHashGridTiler {
     private final GeoBoundingBox bbox;
     private final boolean crossesDateline;
+    private final long maxHashes;
 
     public BoundedGeoHashGridTiler(int precision, GeoBoundingBox bbox) {
         super(precision);
         this.bbox = bbox;
         this.crossesDateline = bbox.right() < bbox.left();
+        final long hashesY = (long)((bbox.top() - bbox.bottom()) / Geohash.latHeightInDegrees(precision)) + 1;
+        final long hashesX;
+        if (crossesDateline) {
+            hashesX = (long)((360 - bbox.left() + bbox.right()) / Geohash.lonWidthInDegrees(precision)) + 1;
+        } else {
+            hashesX = (long)((bbox.right() - bbox.left()) / Geohash.lonWidthInDegrees(precision)) + 1;
+        }
+        this.maxHashes = hashesX * hashesY;
+    }
+
+    @Override
+    protected long getMaxHashes() {
+        return maxHashes;
     }
 
     @Override

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

@@ -13,12 +13,21 @@ package org.elasticsearch.xpack.spatial.search.aggregations.bucket.geogrid;
  */
 public class UnboundedGeoHashGridTiler extends AbstractGeoHashGridTiler {
 
+    private final long maxHashes;
+
+
     public UnboundedGeoHashGridTiler(int precision) {
         super(precision);
+        this.maxHashes = (long) Math.pow(32, precision);
     }
 
     @Override
     protected boolean validHash(String hash) {
        return true;
     }
+
+    @Override
+    protected long getMaxHashes() {
+        return maxHashes;
+    }
 }

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

@@ -106,7 +106,7 @@ public class GeoGridTilerTests extends ESTestCase {
     // tests that bounding boxes of shapes crossing the dateline are correctly wrapped
     public void testGeoTileSetValuesBoundingBoxes_BoundedGeoShapeCellValues() throws Exception {
         for (int i = 0; i < 1; i++) {
-            int precision = randomIntBetween(0, 4);
+            int precision = randomIntBetween(1, 4);
             GeoShapeIndexer indexer = new GeoShapeIndexer(true, "test");
             Geometry geometry = indexer.prepareForIndexing(randomValueOtherThanMany(g -> {
                 try {
@@ -226,7 +226,6 @@ public class GeoGridTilerTests extends ESTestCase {
         }
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/72521")
     public void testGeoHashBoundsExcludeTouchingTiles() throws Exception {
         final int precision = randomIntBetween(1, 5);
         final String hash =
@@ -250,6 +249,7 @@ public class GeoGridTilerTests extends ESTestCase {
             final int numTiles = values.docValueCount();
             final int expected = (int) Math.pow(32, i);
             assertThat(numTiles, equalTo(expected));
+            assertThat((int) bounded.getMaxHashes(), greaterThanOrEqualTo(expected));
         }
     }