From d7bdc2c307a54ceb99b44adea25641f5a6a99bb9 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Thu, 13 Feb 2020 09:31:04 -0800 Subject: [PATCH] fix encoding bug when clipping geo-tile latitude mask (#52274) Due to how geometries are encoded, it is important to compare the bounds of a shape to that of the encoded latitude bounds for geo-tiles. --- .../aggregations/bucket/geogrid/GeoGridTiler.java | 8 +++++--- .../aggregations/bucket/geogrid/GeoTileUtils.java | 10 +++++++++- .../org/elasticsearch/common/geo/GeoTestUtils.java | 9 +++++++++ .../bucket/geogrid/GeoGridTilerTests.java | 13 +++++++------ .../bucket/geogrid/GeoTileUtilsTests.java | 5 ++--- 5 files changed, 32 insertions(+), 13 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTiler.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTiler.java index 37a5615dcc60d..ebf7ad21d208c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTiler.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTiler.java @@ -160,7 +160,8 @@ public long encode(double x, double y, int precision) { /** * Sets the values of the long[] underlying {@link GeoShapeCellValues}. * - * If the shape resides between GeoTileUtils.LATITUDE_MASK and 90 degree latitudes, then + * If the shape resides between GeoTileUtils.NORMALIZED_LATITUDE_MASK and 90 or + * between GeoTileUtils.NORMALIZED_NEGATIVE_LATITUDE_MASK and -90 degree latitudes, then * the shape is not accounted for since geo-tiles are only defined within those bounds. * * @param values the bucket values @@ -182,8 +183,9 @@ public int setValues(GeoShapeCellValues values, MultiGeoValues.GeoValue geoValue // geo tiles are not defined at the extreme latitudes due to them // tiling the world as a square. - if ((bounds.top > GeoTileUtils.LATITUDE_MASK && bounds.bottom > GeoTileUtils.LATITUDE_MASK) - || (bounds.top < -GeoTileUtils.LATITUDE_MASK && bounds.bottom < -GeoTileUtils.LATITUDE_MASK)) { + if ((bounds.top > GeoTileUtils.NORMALIZED_LATITUDE_MASK && bounds.bottom > GeoTileUtils.NORMALIZED_LATITUDE_MASK) + || (bounds.top < GeoTileUtils.NORMALIZED_NEGATIVE_LATITUDE_MASK + && bounds.bottom < GeoTileUtils.NORMALIZED_NEGATIVE_LATITUDE_MASK)) { return 0; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java index 4b98a4855d857..c97bb88135109 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java @@ -18,6 +18,7 @@ */ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.apache.lucene.geo.GeoEncodingUtils; import org.apache.lucene.util.SloppyMath; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoPoint; @@ -46,7 +47,14 @@ public final class GeoTileUtils { /** * The geo-tile map is clipped at 85.05112878 to 90 and -85.05112878 to -90 */ - public static final double LATITUDE_MASK = 85.05112878; + public static final double LATITUDE_MASK = 85.0511287798066; + + /** + * Since shapes are encoded, their boundaries are to be compared to against the encoded/decoded values of LATITUDE_MASK + */ + static final double NORMALIZED_LATITUDE_MASK = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(LATITUDE_MASK)); + static final double NORMALIZED_NEGATIVE_LATITUDE_MASK = + GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(-LATITUDE_MASK)); private static final double PI_DIV_2 = Math.PI / 2; diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoTestUtils.java b/server/src/test/java/org/elasticsearch/common/geo/GeoTestUtils.java index 79c81e2ee4545..2160340dc0a64 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoTestUtils.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoTestUtils.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.geo; import org.apache.lucene.document.ShapeField; +import org.apache.lucene.geo.GeoEncodingUtils; import org.apache.lucene.index.IndexableField; import org.apache.lucene.store.ByteBuffersDataOutput; import org.apache.lucene.util.BytesRef; @@ -75,6 +76,14 @@ public static TriangleTreeReader triangleTreeReader(Geometry geometry, Coordinat return reader; } + public static double encodeDecodeLat(double lat) { + return GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(lat)); + } + + public static double encodeDecodeLon(double lon) { + return GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(lon)); + } + public static String toGeoJsonString(Geometry geometry) throws IOException { XContentBuilder builder = XContentFactory.jsonBuilder(); GeoJson.toXContent(geometry, builder, ToXContent.EMPTY_PARAMS); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTilerTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTilerTests.java index cf68c7e35a9c0..8d3f8b04aa7d3 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTilerTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridTilerTests.java @@ -40,8 +40,11 @@ import java.util.Arrays; import java.util.List; +import static org.elasticsearch.common.geo.GeoTestUtils.encodeDecodeLat; +import static org.elasticsearch.common.geo.GeoTestUtils.encodeDecodeLon; import static org.elasticsearch.common.geo.GeoTestUtils.triangleTreeReader; -import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.LATITUDE_MASK; +import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.NORMALIZED_LATITUDE_MASK; +import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.NORMALIZED_NEGATIVE_LATITUDE_MASK; import static org.hamcrest.Matchers.equalTo; public class GeoGridTilerTests extends ESTestCase { @@ -178,14 +181,12 @@ public void testGeoTileSetValuesBoundingBoxes_UnboundedGeoShapeCellValues() thro } } - @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/37206") public void testTilerMatchPoint() throws Exception { int precision = randomIntBetween(0, 4); Point originalPoint = GeometryTestUtils.randomPoint(false); int xTile = GeoTileUtils.getXTile(originalPoint.getX(), 1 << precision); int yTile = GeoTileUtils.getYTile(originalPoint.getY(), 1 << precision); Rectangle bbox = GeoTileUtils.toBoundingBox(xTile, yTile, precision); - long originalTileHash = GeoTileUtils.longEncode(originalPoint.getX(), originalPoint.getY(), precision); Point[] pointCorners = new Point[] { // tile corners @@ -207,7 +208,7 @@ public void testTilerMatchPoint() throws Exception { int numTiles = GEOTILE.setValues(unboundedCellValues, value, precision); assertThat(numTiles, equalTo(1)); long tilerHash = unboundedCellValues.getValues()[0]; - long pointHash = GeoTileUtils.longEncode(point.getX(), point.getY(), precision); + long pointHash = GeoTileUtils.longEncode(encodeDecodeLon(point.getX()), encodeDecodeLat(point.getY()), precision); assertThat(tilerHash, equalTo(pointHash)); } } @@ -270,8 +271,8 @@ private int numTiles(MultiGeoValues.GeoValue geoValue, int precision) { return 1; } - if ((bounds.top > LATITUDE_MASK && bounds.bottom > LATITUDE_MASK) - || (bounds.top < -LATITUDE_MASK && bounds.bottom < -LATITUDE_MASK)) { + if ((bounds.top > NORMALIZED_LATITUDE_MASK && bounds.bottom > NORMALIZED_LATITUDE_MASK) + || (bounds.top < NORMALIZED_NEGATIVE_LATITUDE_MASK && bounds.bottom < NORMALIZED_NEGATIVE_LATITUDE_MASK)) { return 0; } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtilsTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtilsTests.java index 4090b2a5756f6..cf3e8699b6894 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtilsTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtilsTests.java @@ -23,7 +23,6 @@ import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.test.ESTestCase; -import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.LATITUDE_MASK; import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.MAX_ZOOM; import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.checkPrecisionRange; import static org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils.hashToGeoPoint; @@ -223,8 +222,8 @@ public void testGeoTileAsLongRoutines() { * so ensure they are clipped correctly. */ public void testSingularityAtPoles() { - double minLat = -LATITUDE_MASK; - double maxLat = LATITUDE_MASK; + double minLat = -GeoTileUtils.LATITUDE_MASK; + double maxLat = GeoTileUtils.LATITUDE_MASK; double lon = randomIntBetween(-180, 180); double lat = randomBoolean() ? randomDoubleBetween(-90, minLat, true)