From 126412c8bf4214176bd2aa0f89513ef3e32f145d Mon Sep 17 00:00:00 2001 From: Ignacio Vera Date: Wed, 25 Oct 2023 08:35:02 +0200 Subject: [PATCH] Don't generate bounding box touching tiles in GeoTileGridAggregatorTests (#101137) --- .../geogrid/GeoHashGridAggregatorTests.java | 2 +- .../geogrid/GeoTileGridAggregatorTests.java | 59 +++++++++++++------ .../geogrid/GeoGridAggregatorTestCase.java | 28 +++++---- .../bucket/geogrid/GeoHexAggregatorTests.java | 2 +- 4 files changed, 59 insertions(+), 32 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index daf97a2a49e23..7f6bedcea5277 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -30,7 +30,7 @@ protected Point randomPoint() { } @Override - protected GeoBoundingBox randomBBox() { + protected GeoBoundingBox randomBBox(int precision) { Rectangle rectangle = GeometryTestUtils.randomRectangle(); return new GeoBoundingBox( new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java index e92a268c49efe..2cf729992cfb4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java @@ -8,8 +8,6 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; -import org.apache.lucene.geo.GeoEncodingUtils; -import org.apache.lucene.tests.util.LuceneTestCase; import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; @@ -17,7 +15,11 @@ import org.elasticsearch.geometry.Point; import org.elasticsearch.geometry.Rectangle; -@LuceneTestCase.AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/101077") +import static org.apache.lucene.geo.GeoEncodingUtils.decodeLatitude; +import static org.apache.lucene.geo.GeoEncodingUtils.decodeLongitude; +import static org.apache.lucene.geo.GeoEncodingUtils.encodeLatitude; +import static org.apache.lucene.geo.GeoEncodingUtils.encodeLongitude; + public class GeoTileGridAggregatorTests extends GeoGridAggregatorTestCase { @Override @@ -39,7 +41,7 @@ protected Point randomPoint() { } @Override - protected GeoBoundingBox randomBBox() { + protected GeoBoundingBox randomBBox(int precision) { GeoBoundingBox bbox = randomValueOtherThanMany( (b) -> b.top() > GeoTileUtils.LATITUDE_MASK || b.bottom() < -GeoTileUtils.LATITUDE_MASK, () -> { @@ -50,11 +52,42 @@ protected GeoBoundingBox randomBBox() { ); } ); - // 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())); + final int tiles = 1 << precision; + + // Due to the way GeoTileBoundedPredicate works that adjust the given bounding box when it is touching the tiles, we need to + // adjust here in order not to generate bounding boxes touching tiles or the test will fail + + // compute tile at the top left + final Rectangle minTile = GeoTileUtils.toBoundingBox( + GeoTileUtils.getXTile(bbox.left(), tiles), + GeoTileUtils.getYTile(bbox.top(), tiles), + precision + ); + // adjust if it is touching the tile + final int encodedLeft = encodeLongitude(bbox.left()); + final double left = encodeLongitude(minTile.getMaxX()) == encodedLeft + ? decodeLongitude(encodedLeft + 1) + : decodeLongitude(encodedLeft); + final int encodedTop = encodeLatitude(bbox.top()); + final double bottom = encodeLatitude(minTile.getMinY()) == encodedTop + ? decodeLongitude(encodedTop + 1) + : decodeLatitude(encodedTop); + // compute tile at the bottom right + final Rectangle maxTile = GeoTileUtils.toBoundingBox( + GeoTileUtils.getXTile(bbox.right(), tiles), + GeoTileUtils.getYTile(bbox.bottom(), tiles), + precision + ); + // adjust if it is touching the tile + final int encodedRight = encodeLongitude(bbox.right()); + final double right = encodeLongitude(maxTile.getMinX()) == encodedRight + ? decodeLongitude(encodedRight) + : decodeLongitude(encodedRight + 1); + final int encodedBottom = encodeLatitude(bbox.bottom()); + final double top = encodeLatitude(maxTile.getMaxY()) == encodedBottom + ? decodeLatitude(encodedBottom) + : decodeLatitude(encodedBottom + 1); + bbox.topLeft().reset(top, left); bbox.bottomRight().reset(bottom, right); return bbox; @@ -62,14 +95,6 @@ protected GeoBoundingBox randomBBox() { @Override protected Rectangle getTile(double lng, double lat, int precision) { - int tiles = 1 << precision; - int x = GeoTileUtils.getXTile(lng, tiles); - int y = GeoTileUtils.getYTile(lat, tiles); - Rectangle r1 = GeoTileUtils.toBoundingBox(x, y, precision); - Rectangle r2 = GeoTileUtils.toBoundingBox(GeoTileUtils.longEncode(lng, lat, precision)); - if (r1.equals(r2) == false) { - int a = 0; - } return GeoTileUtils.toBoundingBox(GeoTileUtils.longEncode(lng, lat, precision)); } diff --git a/test/framework/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java b/test/framework/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java index d3012c4779024..1bdc39fdc8e5f 100644 --- a/test/framework/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java @@ -74,7 +74,7 @@ public abstract class GeoGridAggregatorTestCase /** * Return a random {@link GeoBoundingBox} within the bounds of the tile grid. */ - protected abstract GeoBoundingBox randomBBox(); + protected abstract GeoBoundingBox randomBBox(int precision); /** * Return the bounding tile as a {@link Rectangle} for a given point @@ -131,23 +131,24 @@ public void testUnmappedMissing() throws IOException { } public void testSingletonDocs() throws IOException { - testWithSeveralDocs(() -> true, null); + testWithSeveralDocs(() -> true, null, randomPrecision()); } public void testBoundedSingletonDocs() throws IOException { - testWithSeveralDocs(() -> true, randomBBox()); + int precision = randomPrecision(); + testWithSeveralDocs(() -> true, randomBBox(precision), precision); } public void testMultiValuedDocs() throws IOException { - testWithSeveralDocs(LuceneTestCase::rarely, null); + testWithSeveralDocs(LuceneTestCase::rarely, null, randomPrecision()); } public void testBoundedMultiValuedDocs() throws IOException { - testWithSeveralDocs(LuceneTestCase::rarely, randomBBox()); + int precision = randomPrecision(); + testWithSeveralDocs(LuceneTestCase::rarely, randomBBox(precision), precision); } - private void testWithSeveralDocs(BooleanSupplier supplier, GeoBoundingBox bbox) throws IOException { - int precision = randomPrecision(); + private void testWithSeveralDocs(BooleanSupplier supplier, GeoBoundingBox bbox, int precision) throws IOException { int numPoints = randomIntBetween(8, 128); Map expectedCountPerGeoHash = new HashMap<>(); testCase(new MatchAllDocsQuery(), FIELD_NAME, precision, bbox, geoHashGrid -> { @@ -185,23 +186,24 @@ private void testWithSeveralDocs(BooleanSupplier supplier, GeoBoundingBox bbox) } public void testSingletonDocsAsSubAgg() throws IOException { - testWithSeveralDocsAsSubAgg(() -> true, null); + testWithSeveralDocsAsSubAgg(() -> true, null, randomPrecision()); } public void testBoundedSingletonDocsAsSubAgg() throws IOException { - testWithSeveralDocsAsSubAgg(() -> true, randomBBox()); + int precision = randomPrecision(); + testWithSeveralDocsAsSubAgg(() -> true, randomBBox(precision), precision); } public void testMultiValuedDocsAsSubAgg() throws IOException { - testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, null); + testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, null, randomPrecision()); } public void testBoundedMultiValuedDocsAsSubAgg() throws IOException { - testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, randomBBox()); + int precision = randomPrecision(); + testWithSeveralDocsAsSubAgg(LuceneTestCase::rarely, randomBBox(precision), precision); } - private void testWithSeveralDocsAsSubAgg(BooleanSupplier supplier, GeoBoundingBox bbox) throws IOException { - int precision = randomPrecision(); + private void testWithSeveralDocsAsSubAgg(BooleanSupplier supplier, GeoBoundingBox bbox, int precision) throws IOException { int numPoints = randomIntBetween(8, 128); Map> expectedCountPerTPerGeoHash = new TreeMap<>(); TermsAggregationBuilder aggregationBuilder = new TermsAggregationBuilder("t").field("t").size(numPoints); diff --git a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexAggregatorTests.java b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexAggregatorTests.java index 4aa2340eef3b6..3f75b9830a96d 100644 --- a/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexAggregatorTests.java +++ b/x-pack/plugin/spatial/src/test/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexAggregatorTests.java @@ -72,7 +72,7 @@ protected Point randomPoint() { } @Override - protected GeoBoundingBox randomBBox() { + protected GeoBoundingBox randomBBox(int precision) { GeoBoundingBox bbox = randomValueOtherThanMany( (b) -> b.top() > GeoTileUtils.LATITUDE_MASK || b.bottom() < -GeoTileUtils.LATITUDE_MASK, () -> {