diff --git a/src/main/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapper.java b/src/main/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapper.java index 406efe63b28..66d22336456 100644 --- a/src/main/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapper.java +++ b/src/main/java/org/elasticsearch/index/mapper/geo/GeoPointFieldMapper.java @@ -35,6 +35,7 @@ import org.elasticsearch.index.mapper.core.StringFieldMapper; import org.elasticsearch.index.mapper.object.ArrayValueMapperParser; import org.elasticsearch.index.search.geo.GeoHashUtils; import org.elasticsearch.index.search.geo.GeoUtils; +import org.elasticsearch.index.search.geo.Point; import java.io.IOException; import java.util.Map; @@ -372,11 +373,11 @@ public class GeoPointFieldMapper implements Mapper, ArrayValueMapperParser { } private void parseLatLon(ParseContext context, double lat, double lon) throws IOException { - if (normalizeLon) { - lon = GeoUtils.normalizeLon(lon); - } - if (normalizeLat) { - lat = GeoUtils.normalizeLat(lat); + if (normalizeLat || normalizeLon) { + Point point = new Point(lat, lon); + GeoUtils.normalizePoint(point, normalizeLat, normalizeLon); + lat = point.lat; + lon = point.lon; } if (validateLat) { @@ -409,11 +410,11 @@ public class GeoPointFieldMapper implements Mapper, ArrayValueMapperParser { double lat = values[0]; double lon = values[1]; - if (normalizeLon) { - lon = GeoUtils.normalizeLon(lon); - } - if (normalizeLat) { - lat = GeoUtils.normalizeLat(lat); + if (normalizeLat || normalizeLon) { + Point point = new Point(lat, lon); + GeoUtils.normalizePoint(point, normalizeLat, normalizeLon); + lat = point.lat; + lon = point.lon; } if (validateLat) { diff --git a/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxFilterParser.java b/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxFilterParser.java index dcb5b87a4e7..432d588bab8 100644 --- a/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxFilterParser.java @@ -61,8 +61,7 @@ public class GeoBoundingBoxFilterParser implements FilterParser { String filterName = null; String currentFieldName = null; XContentParser.Token token; - boolean normalizeLon = true; - boolean normalizeLat = true; + boolean normalize = true; String type = "memory"; @@ -151,23 +150,18 @@ public class GeoBoundingBoxFilterParser implements FilterParser { } else if ("_cache_key".equals(currentFieldName) || "_cacheKey".equals(currentFieldName)) { cacheKey = new CacheKeyFilter.Key(parser.text()); } else if ("normalize".equals(currentFieldName)) { - normalizeLat = parser.booleanValue(); - normalizeLon = parser.booleanValue(); + normalize = parser.booleanValue(); } else if ("type".equals(currentFieldName)) { type = parser.text(); } else { - throw new QueryParsingException(parseContext.index(), "[qeo_bbox] filter does not support [" + currentFieldName + "]"); + throw new QueryParsingException(parseContext.index(), "[geo_bbox] filter does not support [" + currentFieldName + "]"); } } } - if (normalizeLat) { - topLeft.lat = GeoUtils.normalizeLat(topLeft.lat); - bottomRight.lat = GeoUtils.normalizeLat(bottomRight.lat); - } - if (normalizeLon) { - topLeft.lon = GeoUtils.normalizeLon(topLeft.lon); - bottomRight.lon = GeoUtils.normalizeLon(bottomRight.lon); + if (normalize) { + GeoUtils.normalizePoint(topLeft); + GeoUtils.normalizePoint(bottomRight); } MapperService.SmartNameFieldMappers smartMappers = parseContext.smartFieldMappers(fieldName); diff --git a/src/main/java/org/elasticsearch/index/query/GeoDistanceFilterParser.java b/src/main/java/org/elasticsearch/index/query/GeoDistanceFilterParser.java index 9b05dd988a6..0b216d85821 100644 --- a/src/main/java/org/elasticsearch/index/query/GeoDistanceFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/GeoDistanceFilterParser.java @@ -28,10 +28,7 @@ import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.geo.GeoPointFieldDataType; import org.elasticsearch.index.mapper.geo.GeoPointFieldMapper; -import org.elasticsearch.index.search.geo.GeoDistance; -import org.elasticsearch.index.search.geo.GeoDistanceFilter; -import org.elasticsearch.index.search.geo.GeoHashUtils; -import org.elasticsearch.index.search.geo.GeoUtils; +import org.elasticsearch.index.search.geo.*; import java.io.IOException; @@ -168,11 +165,11 @@ public class GeoDistanceFilterParser implements FilterParser { } distance = geoDistance.normalize(distance, DistanceUnit.MILES); - if (normalizeLat) { - lat = GeoUtils.normalizeLat(lat); - } - if (normalizeLon) { - lon = GeoUtils.normalizeLon(lon); + if (normalizeLat || normalizeLon) { + Point point = new Point(lat, lon); + GeoUtils.normalizePoint(point, normalizeLat, normalizeLon); + lat = point.lat; + lon = point.lon; } MapperService.SmartNameFieldMappers smartMappers = parseContext.smartFieldMappers(fieldName); diff --git a/src/main/java/org/elasticsearch/index/query/GeoDistanceRangeFilterParser.java b/src/main/java/org/elasticsearch/index/query/GeoDistanceRangeFilterParser.java index 213d9003c64..296ef8b2342 100644 --- a/src/main/java/org/elasticsearch/index/query/GeoDistanceRangeFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/GeoDistanceRangeFilterParser.java @@ -28,10 +28,7 @@ import org.elasticsearch.index.mapper.FieldMapper; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.geo.GeoPointFieldDataType; import org.elasticsearch.index.mapper.geo.GeoPointFieldMapper; -import org.elasticsearch.index.search.geo.GeoDistance; -import org.elasticsearch.index.search.geo.GeoDistanceRangeFilter; -import org.elasticsearch.index.search.geo.GeoHashUtils; -import org.elasticsearch.index.search.geo.GeoUtils; +import org.elasticsearch.index.search.geo.*; import java.io.IOException; @@ -224,11 +221,11 @@ public class GeoDistanceRangeFilterParser implements FilterParser { to = geoDistance.normalize(to, DistanceUnit.MILES); } - if (normalizeLat) { - lat = GeoUtils.normalizeLat(lat); - } - if (normalizeLon) { - lon = GeoUtils.normalizeLon(lon); + if (normalizeLat || normalizeLon) { + Point point = new Point(lat, lon); + GeoUtils.normalizePoint(point, normalizeLat, normalizeLon); + lat = point.lat; + lon = point.lon; } MapperService.SmartNameFieldMappers smartMappers = parseContext.smartFieldMappers(fieldName); diff --git a/src/main/java/org/elasticsearch/index/query/GeoPolygonFilterParser.java b/src/main/java/org/elasticsearch/index/query/GeoPolygonFilterParser.java index d8d47ec73b9..e8417e2f2a4 100644 --- a/src/main/java/org/elasticsearch/index/query/GeoPolygonFilterParser.java +++ b/src/main/java/org/elasticsearch/index/query/GeoPolygonFilterParser.java @@ -161,12 +161,9 @@ public class GeoPolygonFilterParser implements FilterParser { throw new QueryParsingException(parseContext.index(), "no points defined for geo_polygon filter"); } - for (Point point : points) { - if (normalizeLat) { - point.lat = GeoUtils.normalizeLat(point.lat); - } - if (normalizeLon) { - point.lon = GeoUtils.normalizeLon(point.lon); + if (normalizeLat || normalizeLon) { + for (Point point : points) { + GeoUtils.normalizePoint(point, normalizeLat, normalizeLon); } } diff --git a/src/main/java/org/elasticsearch/index/search/geo/GeoUtils.java b/src/main/java/org/elasticsearch/index/search/geo/GeoUtils.java index 493943f65b2..1bedff92b40 100644 --- a/src/main/java/org/elasticsearch/index/search/geo/GeoUtils.java +++ b/src/main/java/org/elasticsearch/index/search/geo/GeoUtils.java @@ -23,20 +23,106 @@ package org.elasticsearch.index.search.geo; */ public class GeoUtils { + /** + * Normalize longitude to lie within the -180 (exclusive) to 180 (inclusive) range. + * + * @param lon Longitude to normalize + * @see #normalizePoint(Point) + * @return The normalized longitude. + */ public static double normalizeLon(double lon) { return centeredModulus(lon, 360); } + /** + * Normalize latitude to lie within the -90 to 90 (both inclusive) range. + *

+ * Note: You should not normalize longitude and latitude separately, + * because when normalizing latitude it may be necessary to + * add a shift of 180° in the longitude. + * For this purpose, you should call the + * {@link #normalizePoint(Point)} function. + * + * @param lat Latitude to normalize + * @see #normalizePoint(Point) + * @return The normalized latitude. + */ public static double normalizeLat(double lat) { - return centeredModulus(lat, 180); + lat = centeredModulus(lat, 360); + if (lat < -90) { + lat = -180 - lat; + } else if (lat > 90) { + lat = 180 - lat; + } + return lat; + } + + /** + * Normalize the geo {@code Point} for its coordinates to lie within their + * respective normalized ranges. + *

+ * Note: A shift of 180° is applied in the longitude if necessary, + * in order to normalize properly the latitude. + * + * @param point The point to normalize in-place. + */ + public static void normalizePoint(Point point) { + normalizePoint(point, true, true); + } + + /** + * Normalize the geo {@code Point} for the given coordinates to lie within + * their respective normalized ranges. + * + * You can control which coordinate gets normalized with the two flags. + *

+ * Note: A shift of 180° is applied in the longitude if necessary, + * in order to normalize properly the latitude. + * If normalizing latitude but not longitude, it is assumed that + * the longitude is in the form x+k*360, with x in ]-180;180], + * and k is meaningful to the application. + * Therefore x will be adjusted while keeping k preserved. + * + * @param point The point to normalize in-place. + * @param normLat Whether to normalize latitude or leave it as is. + * @param normLon Whether to normalize longitude. + */ + public static void normalizePoint(Point point, boolean normLat, boolean normLon) { + if (normLat) { + point.lat = centeredModulus(point.lat, 360); + boolean shift = true; + if (point.lat < -90) { + point.lat = -180 - point.lat; + } else if (point.lat > 90) { + point.lat = 180 - point.lat; + } else { + // No need to shift the longitude, and the latitude is normalized + shift = false; + } + if (shift) { + if (normLon) { + point.lon += 180; + } else { + // Longitude won't be normalized, + // keep it in the form x+k*360 (with x in ]-180;180]) + // by only changing x, assuming k is meaningful for the user application. + point.lon += normalizeLon(point.lon) > 0 ? -180 : 180; + } + } + } + if (normLon) { + point.lon = centeredModulus(point.lon, 360); + } } private static double centeredModulus(double dividend, double divisor) { double rtn = dividend % divisor; - if (rtn <= 0) + if (rtn <= 0) { rtn += divisor; - if (rtn > divisor/2) + } + if (rtn > divisor / 2) { rtn -= divisor; + } return rtn; } diff --git a/src/main/java/org/elasticsearch/search/facet/geodistance/GeoDistanceFacetProcessor.java b/src/main/java/org/elasticsearch/search/facet/geodistance/GeoDistanceFacetProcessor.java index e456d91b182..9df9a8b2741 100644 --- a/src/main/java/org/elasticsearch/search/facet/geodistance/GeoDistanceFacetProcessor.java +++ b/src/main/java/org/elasticsearch/search/facet/geodistance/GeoDistanceFacetProcessor.java @@ -29,6 +29,7 @@ import org.elasticsearch.index.mapper.geo.GeoPointFieldMapper; import org.elasticsearch.index.search.geo.GeoDistance; import org.elasticsearch.index.search.geo.GeoHashUtils; import org.elasticsearch.index.search.geo.GeoUtils; +import org.elasticsearch.index.search.geo.Point; import org.elasticsearch.search.facet.Facet; import org.elasticsearch.search.facet.FacetCollector; import org.elasticsearch.search.facet.FacetPhaseExecutionException; @@ -171,11 +172,11 @@ public class GeoDistanceFacetProcessor extends AbstractComponent implements Face throw new FacetPhaseExecutionException(facetName, "no ranges defined for geo_distance facet"); } - if (normalizeLat) { - lat = GeoUtils.normalizeLat(lat); - } - if (normalizeLon) { - lon = GeoUtils.normalizeLon(lon); + if (normalizeLat || normalizeLon) { + Point point = new Point(lat, lon); + GeoUtils.normalizePoint(point, normalizeLat, normalizeLon); + lat = point.lat; + lon = point.lon; } if (valueFieldName != null) { diff --git a/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java b/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java index c4e7b892f65..be41bc03f55 100644 --- a/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java +++ b/src/main/java/org/elasticsearch/search/sort/GeoDistanceSortParser.java @@ -23,10 +23,7 @@ import org.apache.lucene.search.SortField; import org.elasticsearch.common.unit.DistanceUnit; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.mapper.geo.GeoPointFieldMapper; -import org.elasticsearch.index.search.geo.GeoDistance; -import org.elasticsearch.index.search.geo.GeoDistanceDataComparator; -import org.elasticsearch.index.search.geo.GeoHashUtils; -import org.elasticsearch.index.search.geo.GeoUtils; +import org.elasticsearch.index.search.geo.*; import org.elasticsearch.search.internal.SearchContext; /** @@ -113,11 +110,11 @@ public class GeoDistanceSortParser implements SortParser { } } - if (normalizeLat) { - lat = GeoUtils.normalizeLat(lat); - } - if (normalizeLon) { - lon = GeoUtils.normalizeLon(lon); + if (normalizeLat || normalizeLon) { + Point point = new Point(lat, lon); + GeoUtils.normalizePoint(point, normalizeLat, normalizeLon); + lat = point.lat; + lon = point.lon; } return new SortField(fieldName, GeoDistanceDataComparator.comparatorSource(fieldName, lat, lon, unit, geoDistance, context.fieldDataCache(), context.mapperService()), reverse); diff --git a/src/test/java/org/elasticsearch/test/unit/index/search/geo/GeoUtilsTests.java b/src/test/java/org/elasticsearch/test/unit/index/search/geo/GeoUtilsTests.java index 50c37e7a29d..0d3bda367f8 100644 --- a/src/test/java/org/elasticsearch/test/unit/index/search/geo/GeoUtilsTests.java +++ b/src/test/java/org/elasticsearch/test/unit/index/search/geo/GeoUtilsTests.java @@ -20,6 +20,7 @@ package org.elasticsearch.test.unit.index.search.geo; import org.elasticsearch.index.search.geo.GeoUtils; +import org.elasticsearch.index.search.geo.Point; import org.testng.annotations.Test; import static org.hamcrest.MatcherAssert.assertThat; @@ -61,7 +62,7 @@ public class GeoUtilsTests { assertThat(GeoUtils.normalizeLat(180.0), equalTo(0.0)); // and halves assertThat(GeoUtils.normalizeLon(-180.0), equalTo(180.0)); - assertThat(GeoUtils.normalizeLat(-90.0), equalTo(90.0)); + assertThat(GeoUtils.normalizeLat(-90.0), equalTo(-90.0)); assertThat(GeoUtils.normalizeLon(180.0), equalTo(180.0)); assertThat(GeoUtils.normalizeLat(90.0), equalTo(90.0)); } @@ -73,14 +74,19 @@ public class GeoUtilsTests { public void testNormal() { // Near bounds assertThat(GeoUtils.normalizeLon(-360.5), equalTo(-0.5)); - assertThat(GeoUtils.normalizeLat(-180.5), equalTo(-0.5)); + assertThat(GeoUtils.normalizeLat(-180.5), equalTo(0.5)); assertThat(GeoUtils.normalizeLon(360.5), equalTo(0.5)); - assertThat(GeoUtils.normalizeLat(180.5), equalTo(0.5)); + assertThat(GeoUtils.normalizeLat(180.5), equalTo(-0.5)); // and near halves assertThat(GeoUtils.normalizeLon(-180.5), equalTo(179.5)); - assertThat(GeoUtils.normalizeLat(-90.5), equalTo(89.5)); + assertThat(GeoUtils.normalizeLat(-90.5), equalTo(-89.5)); assertThat(GeoUtils.normalizeLon(180.5), equalTo(-179.5)); - assertThat(GeoUtils.normalizeLat(90.5), equalTo(-89.5)); + assertThat(GeoUtils.normalizeLat(90.5), equalTo(89.5)); + // Now with points, to check for longitude shifting with latitude normalization + // We've gone past the north pole and down the other side, the longitude will + // be shifted by 180 + assertNormalizedPoint(new Point(90.5, 10), new Point(89.5, -170)); + // Every 10-units, multiple full turns for (int shift = -20; shift <= 20; ++shift) { assertThat(GeoUtils.normalizeLon(shift * 360.0 + 0.0), equalTo(0.0)); @@ -122,25 +128,43 @@ public class GeoUtilsTests { assertThat(GeoUtils.normalizeLon(shift * 360.0 + 360.0), equalTo(0.0)); } for (int shift = -20; shift <= 20; ++shift) { - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 0.0), equalTo(0.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 10.0), equalTo(10.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 20.0), equalTo(20.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 30.0), equalTo(30.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 40.0), equalTo(40.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 50.0), equalTo(50.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 60.0), equalTo(60.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 70.0), equalTo(70.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 80.0), equalTo(80.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 90.0), equalTo(90.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 100.0), equalTo(-80.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 110.0), equalTo(-70.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 120.0), equalTo(-60.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 130.0), equalTo(-50.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 140.0), equalTo(-40.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 150.0), equalTo(-30.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 160.0), equalTo(-20.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 170.0), equalTo(-10.0)); - assertThat(GeoUtils.normalizeLat(shift * 180.0 + 180.0), equalTo(0.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 0.0), equalTo(0.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 10.0), equalTo(10.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 20.0), equalTo(20.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 30.0), equalTo(30.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 40.0), equalTo(40.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 50.0), equalTo(50.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 60.0), equalTo(60.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 70.0), equalTo(70.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 80.0), equalTo(80.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 90.0), equalTo(90.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 100.0), equalTo(80.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 110.0), equalTo(70.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 120.0), equalTo(60.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 130.0), equalTo(50.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 140.0), equalTo(40.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 150.0), equalTo(30.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 160.0), equalTo(20.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 170.0), equalTo(10.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 180.0), equalTo(0.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 190.0), equalTo(-10.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 200.0), equalTo(-20.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 210.0), equalTo(-30.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 220.0), equalTo(-40.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 230.0), equalTo(-50.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 240.0), equalTo(-60.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 250.0), equalTo(-70.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 260.0), equalTo(-80.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 270.0), equalTo(-90.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 280.0), equalTo(-80.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 290.0), equalTo(-70.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 300.0), equalTo(-60.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 310.0), equalTo(-50.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 320.0), equalTo(-40.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 330.0), equalTo(-30.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 340.0), equalTo(-20.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 350.0), equalTo(-10.0)); + assertThat(GeoUtils.normalizeLat(shift * 360.0 + 360.0), equalTo(0.0)); } } @@ -175,4 +199,8 @@ public class GeoUtilsTests { assertThat(GeoUtils.normalizeLat(+18000000000091.0), equalTo(GeoUtils.normalizeLat(+091.0))); } + private static void assertNormalizedPoint(Point input, Point expected) { + GeoUtils.normalizePoint(input); + assertThat(input, equalTo(expected)); + } }