Skip to content

Commit

Permalink
Don't normalize coordinates in GeoTileUtils (elastic#114929)
Browse files Browse the repository at this point in the history 
The main users of this class use as input latitudes and longitudes read from doc values. These coordinates are always 
on bounds so there is no point to try to normalise them, more over when this piece of code is in the hot path for 
aggregations.
  • Loading branch information
@iverase
iverase authored Oct 17, 2024
1 parent 8670dd7 commit 1ebe1b3
Show file tree
Hide file tree
Showing 3 changed files with 29 additions and 28 deletions.
18 changes: 8 additions & 10 deletions server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
import java.io.IOException;
import java.util.Locale;

import static org.elasticsearch.common.geo.GeoUtils.normalizeLat;
import static org.elasticsearch.common.geo.GeoUtils.normalizeLon;
import static org.elasticsearch.common.geo.GeoUtils.quantizeLat;

/**
Expand Down Expand Up @@ -113,15 +111,13 @@ public static int checkPrecisionRange(int precision) {
* Calculates the x-coordinate in the tile grid for the specified longitude given
* the number of tile columns for a pre-determined zoom-level.
*
* @param longitude the longitude to use when determining the tile x-coordinate
* @param longitude the longitude to use when determining the tile x-coordinate. Longitude is in degrees
* and must be between -180 and 180 degrees.
* @param tiles the number of tiles per row for a pre-determined zoom-level
*/
public static int getXTile(double longitude, int tiles) {
// normalizeLon treats this as 180, which is not friendly for tile mapping
if (longitude == -180) {
return 0;
}
final double xTile = (normalizeLon(longitude) + 180.0) / 360.0 * tiles;
assert longitude >= -180 && longitude <= 180 : "Longitude must be between -180 and 180 degrees";
final double xTile = (longitude + 180.0) / 360.0 * tiles;
// Edge values may generate invalid values, and need to be clipped.
return Math.max(0, Math.min(tiles - 1, (int) Math.floor(xTile)));
}
Expand All @@ -130,11 +126,13 @@ public static int getXTile(double longitude, int tiles) {
* Calculates the y-coordinate in the tile grid for the specified longitude given
* the number of tile rows for pre-determined zoom-level.
*
* @param latitude the latitude to use when determining the tile y-coordinate
* @param latitude the latitude to use when determining the tile y-coordinate. Latitude is in degrees
* and must be between -90 and 90 degrees.
* @param tiles the number of tiles per column for a pre-determined zoom-level
*/
public static int getYTile(double latitude, int tiles) {
final double latSin = SloppyMath.cos(PI_DIV_2 - Math.toRadians(normalizeLat(latitude)));
assert latitude >= -90 && latitude <= 90 : "Latitude must be between -90 and 90 degrees";
final double latSin = SloppyMath.cos(PI_DIV_2 - Math.toRadians(latitude));
final double yTile = (0.5 - (ESSloppyMath.log((1.0 + latSin) / (1.0 - latSin)) / PI_TIMES_4)) * tiles;
// Edge values may generate invalid values, and need to be clipped.
// For example, polar regions (above/below lat 85.05112878) get normalized.
Expand Down
5 changes: 3 additions & 2 deletions server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

import org.apache.lucene.document.InetAddressPoint;
import org.apache.lucene.util.BytesRef;
import org.elasticsearch.common.geo.GeoUtils;
import org.elasticsearch.common.io.stream.BytesStreamOutput;
import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
Expand Down Expand Up @@ -173,8 +174,8 @@ public void testGeoTileFormat() {
assertEquals("29/536869420/0", DocValueFormat.GEOTILE.format(longEncode(179.999, 89.999, 29)));
assertEquals("29/1491/536870911", DocValueFormat.GEOTILE.format(longEncode(-179.999, -89.999, 29)));
assertEquals("2/2/1", DocValueFormat.GEOTILE.format(longEncode(1, 1, 2)));
assertEquals("1/1/0", DocValueFormat.GEOTILE.format(longEncode(13, 95, 1)));
assertEquals("1/1/1", DocValueFormat.GEOTILE.format(longEncode(13, -95, 1)));
assertEquals("1/1/0", DocValueFormat.GEOTILE.format(longEncode(13, GeoUtils.normalizeLat(95), 1)));
assertEquals("1/1/1", DocValueFormat.GEOTILE.format(longEncode(13, GeoUtils.normalizeLat(-95), 1)));
}

public void testRawParse() {
Expand Down
34 changes: 18 additions & 16 deletions ...src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileUtilsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
import org.elasticsearch.test.ESTestCase;
import org.hamcrest.Matchers;

import static org.elasticsearch.common.geo.GeoUtils.normalizeLat;
import static org.elasticsearch.common.geo.GeoUtils.normalizeLon;
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 @@ -53,20 +55,20 @@ public void testLongEncode() {
assertEquals(0x77FFFF4580000000L, longEncode(179.999, 89.999, 29));
assertEquals(0x740000BA7FFFFFFFL, longEncode(-179.999, -89.999, 29));
assertEquals(0x0800000040000001L, longEncode(1, 1, 2));
assertEquals(0x0C00000060000000L, longEncode(-20, 100, 3));
assertEquals(0x0C00000060000000L, longEncode(-20, normalizeLat(100), 3));
assertEquals(0x71127D27C8ACA67AL, longEncode(13, -15, 28));
assertEquals(0x4C0077776003A9ACL, longEncode(-12, 15, 19));
assertEquals(0x140000024000000EL, longEncode(-328.231870, 16.064082, 5));
assertEquals(0x6436F96B60000000L, longEncode(-590.769588, 89.549167, 25));
assertEquals(0x6411BD6BA0A98359L, longEncode(999.787079, 51.830093, 25));
assertEquals(0x751BD6BBCA983596L, longEncode(999.787079, 51.830093, 29));
assertEquals(0x77CF880A20000000L, longEncode(-557.039740, -632.103969, 29));
assertEquals(0x140000024000000EL, longEncode(normalizeLon(-328.231870), 16.064082, 5));
assertEquals(0x6436F96B60000000L, longEncode(normalizeLon(-590.769588), 89.549167, 25));
assertEquals(0x6411BD6BA0A98359L, longEncode(normalizeLon(999.787079), 51.830093, 25));
assertEquals(0x751BD6BBCA983596L, longEncode(normalizeLon(999.787079), 51.830093, 29));
assertEquals(0x77CF880A20000000L, longEncode(normalizeLon(-557.039740), normalizeLat(-632.103969), 29));
assertEquals(0x7624FA4FA0000000L, longEncode(13, 88, 29));
assertEquals(0x7624FA4FBFFFFFFFL, longEncode(13, -88, 29));
assertEquals(0x0400000020000000L, longEncode(13, 89, 1));
assertEquals(0x0400000020000001L, longEncode(13, -89, 1));
assertEquals(0x0400000020000000L, longEncode(13, 95, 1));
assertEquals(0x0400000020000001L, longEncode(13, -95, 1));
assertEquals(0x0400000020000000L, longEncode(13, normalizeLat(95), 1));
assertEquals(0x0400000020000001L, longEncode(13, normalizeLat(-95), 1));

expectThrows(IllegalArgumentException.class, () -> longEncode(0, 0, -1));
expectThrows(IllegalArgumentException.class, () -> longEncode(-1, 0, MAX_ZOOM + 1));
Expand All @@ -78,20 +80,20 @@ public void testLongEncodeFromString() {
assertEquals(0x77FFFF4580000000L, longEncode(stringEncode(longEncode(179.999, 89.999, 29))));
assertEquals(0x740000BA7FFFFFFFL, longEncode(stringEncode(longEncode(-179.999, -89.999, 29))));
assertEquals(0x0800000040000001L, longEncode(stringEncode(longEncode(1, 1, 2))));
assertEquals(0x0C00000060000000L, longEncode(stringEncode(longEncode(-20, 100, 3))));
assertEquals(0x0C00000060000000L, longEncode(stringEncode(longEncode(-20, normalizeLat(100), 3))));
assertEquals(0x71127D27C8ACA67AL, longEncode(stringEncode(longEncode(13, -15, 28))));
assertEquals(0x4C0077776003A9ACL, longEncode(stringEncode(longEncode(-12, 15, 19))));
assertEquals(0x140000024000000EL, longEncode(stringEncode(longEncode(-328.231870, 16.064082, 5))));
assertEquals(0x6436F96B60000000L, longEncode(stringEncode(longEncode(-590.769588, 89.549167, 25))));
assertEquals(0x6411BD6BA0A98359L, longEncode(stringEncode(longEncode(999.787079, 51.830093, 25))));
assertEquals(0x751BD6BBCA983596L, longEncode(stringEncode(longEncode(999.787079, 51.830093, 29))));
assertEquals(0x77CF880A20000000L, longEncode(stringEncode(longEncode(-557.039740, -632.103969, 29))));
assertEquals(0x140000024000000EL, longEncode(stringEncode(longEncode(normalizeLon(-328.231870), 16.064082, 5))));
assertEquals(0x6436F96B60000000L, longEncode(stringEncode(longEncode(normalizeLon(-590.769588), 89.549167, 25))));
assertEquals(0x6411BD6BA0A98359L, longEncode(stringEncode(longEncode(normalizeLon(999.787079), 51.830093, 25))));
assertEquals(0x751BD6BBCA983596L, longEncode(stringEncode(longEncode(normalizeLon(999.787079), 51.830093, 29))));
assertEquals(0x77CF880A20000000L, longEncode(stringEncode(longEncode(normalizeLon(-557.039740), normalizeLat(-632.103969), 29))));
assertEquals(0x7624FA4FA0000000L, longEncode(stringEncode(longEncode(13, 88, 29))));
assertEquals(0x7624FA4FBFFFFFFFL, longEncode(stringEncode(longEncode(13, -88, 29))));
assertEquals(0x0400000020000000L, longEncode(stringEncode(longEncode(13, 89, 1))));
assertEquals(0x0400000020000001L, longEncode(stringEncode(longEncode(13, -89, 1))));
assertEquals(0x0400000020000000L, longEncode(stringEncode(longEncode(13, 95, 1))));
assertEquals(0x0400000020000001L, longEncode(stringEncode(longEncode(13, -95, 1))));
assertEquals(0x0400000020000000L, longEncode(stringEncode(longEncode(13, normalizeLat(95), 1))));
assertEquals(0x0400000020000001L, longEncode(stringEncode(longEncode(13, normalizeLat(-95), 1))));

expectThrows(IllegalArgumentException.class, () -> longEncode("12/asdf/1"));
expectThrows(IllegalArgumentException.class, () -> longEncode("foo"));
Expand Down

0 comments on commit 1ebe1b3

Please sign in to comment.