diff --git a/docs/changelog/83348.yaml b/docs/changelog/83348.yaml new file mode 100644 index 0000000000000..94a53919f5f3a --- /dev/null +++ b/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 diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java index 8eaaa949cd92c..93c44f431af7d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java @@ -85,7 +85,7 @@ 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; } @@ -93,13 +93,37 @@ protected int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int val } 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; 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 5672890605456..f5a5653f2b339 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 @@ -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 encodeDecodeLat = (lat) -> GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(lat)); Function 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(); diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexCellIdSource.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexCellIdSource.java index be2589a007707..ad5d008a8647f 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/bucket/geogrid/GeoHexCellIdSource.java +++ b/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; 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 18ec429a0c3a1..25f0e1884f0a2 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 @@ -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 { @@ -39,7 +47,8 @@ protected List 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)) + ); + } }