Skip to content

Commit

Permalink
fix encoding bug when clipping geo-tile latitude mask (#52274)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
talevy committed Feb 13, 2020
1 parent 3bae5f9 commit d7bdc2c
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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 <code>GeoTileUtils.LATITUDE_MASK</code> and 90 degree latitudes, then
* If the shape resides between <code>GeoTileUtils.NORMALIZED_LATITUDE_MASK</code> and 90 or
* between <code>GeoTileUtils.NORMALIZED_NEGATIVE_LATITUDE_MASK</code> 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
Expand All @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 <code>LATITUDE_MASK</code>
*/
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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
Expand All @@ -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));
}
}
Expand Down Expand Up @@ -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;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit d7bdc2c

Please sign in to comment.