Expand tests to use random bounds, after multi-thread fix

Also fix an issue around nulls for out-of-bounds results

And investigate and affirm that the use of -1 internally to mark invalid grid-ids is fine for all three grid types (ie. -1 is not a valid grid-id in any of these cases, for different reasons in each).

Author: craigtaverner

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StGeohash.java

@@ -66,7 +66,8 @@ public long calculateGridId(Point point) {
             if (bounds.validHash(geohash)) {
                 return Geohash.longEncode(geohash);
             }
-            // TODO: Are negative values allowed in geohash long encoding?
+            // Geohash uses the lowest 4 bites for precision, and if all four are set, we get 15, which is invalid since the
+            // max precision allowed is 12. This allows us to return -1 to indicate invalid geohashes (since all bits are set to 1).
             return -1;
         }
 
@@ -209,7 +210,8 @@ public Object fold(FoldContext ctx) {
         } else {
             GeoBoundingBox bbox = asGeoBoundingBox(bounds().fold(ctx));
             GeoHashBoundedGrid bounds = new GeoHashBoundedGrid(precision, bbox);
-            return bounds.calculateGridId(GEO.wkbAsPoint(point));
+            long gridId = bounds.calculateGridId(GEO.wkbAsPoint(point));
+            return gridId < 0 ? null : gridId;
         }
     }
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StGeohex.java

@@ -64,7 +64,7 @@ public long calculateGridId(Point point) {
             if (bounds.validHex(geohex)) {
                 return geohex;
             }
-            // TODO: Are we sure negative numbers are not valid
+            // H3 explicitly requires the highest bit to be zero, freeing up all negative numbers as invalid ids. See H3.isValidHex()
             return -1L;
         }
 
@@ -212,7 +212,8 @@ public Object fold(FoldContext ctx) {
         } else {
             GeoBoundingBox bbox = asGeoBoundingBox(bounds().fold(ctx));
             GeoHexBoundedGrid bounds = new GeoHexBoundedGrid(precision, bbox);
-            return bounds.calculateGridId(GEO.wkbAsPoint(point));
+            long gridId = bounds.calculateGridId(GEO.wkbAsPoint(point));
+            return gridId < 0 ? null : gridId;
         }
     }
 

x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StGeotile.java

@@ -69,7 +69,8 @@ public long calculateGridId(Point point) {
             if (bounds.validTile(x, y, precision)) {
                 return GeoTileUtils.longEncodeTiles(precision, x, y);
             }
-            // TODO: Are we sure negative numbers are not valid
+            // GeoTileUtils uses the highest 6 bits to store the zoom level. However, MAX_ZOOM is 29, which takes 5 bits.
+            // This leaves the sign bit unused, so it can be used to indicate an invalid tile.
             return -1L;
         }
 
@@ -203,7 +204,8 @@ public Object fold(FoldContext ctx) {
         } else {
             GeoBoundingBox bbox = asGeoBoundingBox(bounds().fold(ctx));
             GeoTileBoundedGrid bounds = new GeoTileBoundedGrid(precision, bbox);
-            return bounds.calculateGridId(GEO.wkbAsPoint(point));
+            long gridId = bounds.calculateGridId(GEO.wkbAsPoint(point));
+            return gridId < 0 ? null : gridId;
         }
     }
 

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/SpatialGridFunctionTestCase.java

@@ -102,21 +102,17 @@ protected static void addTestCaseSuppliers(
                     BytesRef geometry = (BytesRef) geoTypedData.data();
                     int precision = between(1, 8);
                     TestCaseSupplier.TypedData precisionData = new TestCaseSupplier.TypedData(precision, INTEGER, "precision");
-                    Rectangle bounds = new Rectangle(-180, 180, 90, -90);
                     String evaluatorName = "FromFieldAndLiteralAndLiteralEvaluator[in=Attribute[channel=0], bounds=[";
                     if (literalPrecision) {
                         precisionData = precisionData.forceLiteral();
                         evaluatorName = "FromFieldAndLiteralAndLiteralEvaluator[in=Attribute[channel=0]";
                     }
-                    TestCaseSupplier.TypedData boundsData;
-                    // Create a rectangle as bounds
-                    BytesRef boundsBytesRef = GEO.asWkb(bounds);
-                    boundsData = new TestCaseSupplier.TypedData(boundsBytesRef, GEO_SHAPE, "bounds").forceLiteral();
+                    var boundsData = randomBoundsData();
                     return new TestCaseSupplier.TestCase(
-                        List.of(geoTypedData, precisionData, boundsData),
+                        List.of(geoTypedData, precisionData, boundsData.typedData),
                         startsWith(getFunctionClassName() + evaluatorName),
                         LONG,
-                        equalTo(expectedValueWithBounds.apply(geometry, precision, SpatialGridFunction.asGeoBoundingBox(bounds)))
+                        equalTo(expectedValueWithBounds.apply(geometry, precision, boundsData.geoBoundingBox()))
                     );
                 }));
             }
@@ -141,6 +137,25 @@ public static TestCaseSupplier.TypedDataSupplier testCaseSupplier(DataType dataT
         }
     }
 
+    protected record TestBoundsData(GeoBoundingBox geoBoundingBox, TestCaseSupplier.TypedData typedData) {}
+
+    private static TestBoundsData randomBoundsData() {
+        // Create a rectangle with random center and random size, that does not exceed geographic bounds
+        double x = randomDoubleBetween(-180.1, 179.1, false);
+        double y = randomDoubleBetween(-90.1, 89.1, false);
+        double width = randomDoubleBetween(0.1, 180.0, true);
+        double height = randomDoubleBetween(0.1, 90.0, true);
+        double minX = Math.max(-180, x - width / 2);
+        double maxX = Math.min(180, x + width / 2);
+        double minY = Math.max(-90, y - height / 2);
+        double maxY = Math.min(90, y + height / 2);
+        Rectangle bounds = new Rectangle(minX, maxX, maxY, minY);
+        return new TestBoundsData(
+            SpatialGridFunction.asGeoBoundingBox(bounds),
+            new TestCaseSupplier.TypedData(GEO.asWkb(bounds), GEO_SHAPE, "bounds").forceLiteral()
+        );
+    }
+
     protected Long process(int precision, BiFunction<BytesRef, Integer, Long> expectedValue) {
         Object spatialObj = this.testCase.getDataValues().getFirst();
         assumeNotNull(spatialObj);

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StGeohashTests.java

@@ -50,9 +50,10 @@ private static long valueOf(BytesRef wkb, int precision) {
         return StGeohash.unboundedGrid.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb), precision);
     }
 
-    private static long boundedValueOf(BytesRef wkb, int precision, GeoBoundingBox bbox) {
+    private static Long boundedValueOf(BytesRef wkb, int precision, GeoBoundingBox bbox) {
         StGeohash.GeoHashBoundedGrid bounds = new StGeohash.GeoHashBoundedGrid.Factory(precision, bbox).get(null);
-        return bounds.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb));
+        long gridId = bounds.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb));
+        return gridId < 0 ? null : gridId;
     }
 
     @Override

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StGeohexTests.java

@@ -50,9 +50,10 @@ private static long valueOf(BytesRef wkb, int precision) {
         return StGeohex.unboundedGrid.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb), precision);
     }
 
-    private static long boundedValueOf(BytesRef wkb, int precision, GeoBoundingBox bbox) {
+    private static Long boundedValueOf(BytesRef wkb, int precision, GeoBoundingBox bbox) {
         StGeohex.GeoHexBoundedGrid bounds = new StGeohex.GeoHexBoundedGrid.Factory(precision, bbox).get(null);
-        return bounds.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb));
+        long gridId = bounds.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb));
+        return gridId < 0 ? null : gridId;
     }
 
     @Override

x-pack/plugin/esql/src/test/java/org/elasticsearch/xpack/esql/expression/function/scalar/spatial/StGeotileTests.java

@@ -13,7 +13,6 @@
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.geo.GeoBoundingBox;
 import org.elasticsearch.license.License;
-import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils;
 import org.elasticsearch.xpack.esql.core.expression.Expression;
 import org.elasticsearch.xpack.esql.core.tree.Source;
 import org.elasticsearch.xpack.esql.core.type.DataType;
@@ -23,6 +22,7 @@
 import java.util.List;
 import java.util.function.Supplier;
 
+import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.MAX_ZOOM;
 import static org.elasticsearch.xpack.esql.core.util.SpatialCoordinateTypes.UNSPECIFIED;
 import static org.hamcrest.Matchers.containsString;
 
@@ -50,9 +50,10 @@ private static long valueOf(BytesRef wkb, int precision) {
         return StGeotile.unboundedGrid.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb), precision);
     }
 
-    private static long boundedValueOf(BytesRef wkb, int precision, GeoBoundingBox bbox) {
+    private static Long boundedValueOf(BytesRef wkb, int precision, GeoBoundingBox bbox) {
         StGeotile.GeoTileBoundedGrid bounds = new StGeotile.GeoTileBoundedGrid.Factory(precision, bbox).get(null);
-        return bounds.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb));
+        long gridId = bounds.calculateGridId(UNSPECIFIED.wkbAsPoint(wkb));
+        return gridId < 0 ? null : gridId;
     }
 
     @Override
@@ -64,7 +65,7 @@ protected Expression build(Source source, List<Expression> args) {
     public void testInvalidPrecision() {
         IllegalArgumentException ex = expectThrows(IllegalArgumentException.class, () -> process(-1, StGeotileTests::valueOf));
         assertThat(ex.getMessage(), containsString("Invalid geotile_grid precision of -1. Must be between 0 and 29."));
-        ex = expectThrows(IllegalArgumentException.class, () -> process(GeoTileUtils.MAX_ZOOM + 1, StGeotileTests::valueOf));
+        ex = expectThrows(IllegalArgumentException.class, () -> process(MAX_ZOOM + 1, StGeotileTests::valueOf));
         assertThat(ex.getMessage(), containsString("Invalid geotile_grid precision of 30. Must be between 0 and 29."));
     }
 }