From 71bbecab1f7203c05492bc70ee0beebb94a85d74 Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 15 May 2018 13:54:34 -0400 Subject: [PATCH 1/3] Use geohash cell instead of just a corner in geo_bounding_box Treats geohashes as grid cells instead of just points when the geohashes are used to specify the edges in the geo_bounding_box query. For example, if a geohash is used to specify the top_left corner, the top left corner of the geohash cell will be used as the corner of the bounding box. Closes #25154 --- .../migration/migrate_7_0/search.asciidoc | 3 + .../query-dsl/geo-bounding-box-query.asciidoc | 32 ++++++++++ .../elasticsearch/common/geo/GeoPoint.java | 29 +++++---- .../elasticsearch/common/geo/GeoUtils.java | 48 ++++++++++++++- .../query/GeoBoundingBoxQueryBuilder.java | 10 ++-- .../GeoBoundingBoxQueryBuilderTests.java | 59 +++++++++++++++++++ .../index/search/geo/GeoUtilsTests.java | 15 +++++ 7 files changed, 177 insertions(+), 19 deletions(-) diff --git a/docs/reference/migration/migrate_7_0/search.asciidoc b/docs/reference/migration/migrate_7_0/search.asciidoc index 529bd1fa5995b..1fe0bc62418bb 100644 --- a/docs/reference/migration/migrate_7_0/search.asciidoc +++ b/docs/reference/migration/migrate_7_0/search.asciidoc @@ -12,6 +12,9 @@ * Purely negative queries (only MUST_NOT clauses) now return a score of `0` rather than `1`. +* The boundary specified using geohashes in the `geo_bounding_box` query + now include entire geohash cell, instead of just geohash center. + ==== Adaptive replica selection enabled by default Adaptive replica selection has been enabled by default. If you wish to return to diff --git a/docs/reference/query-dsl/geo-bounding-box-query.asciidoc b/docs/reference/query-dsl/geo-bounding-box-query.asciidoc index 21a689703e01e..fdf5ca5de16e5 100644 --- a/docs/reference/query-dsl/geo-bounding-box-query.asciidoc +++ b/docs/reference/query-dsl/geo-bounding-box-query.asciidoc @@ -231,6 +231,38 @@ GET /_search -------------------------------------------------- // CONSOLE + +When geohashes are used to specify the bounding the edges of the +bounding box, the geohashes are treated as rectangles. The bounding +box is defined in such a way that its top left corresponds to the top +left corner of the geohash specified in the `top_left` parameter and +its bottom right is defined as the bottom right of the geohash +specified in the `bottom_right` parameter. + +In order to specify a bounding box that would match entire area of a +geohash the geohash can be specified in both `top_left` and +`bottom_right` parameters: + +[source,js] +-------------------------------------------------- +GET /_search +{ + "query": { + "geo_bounding_box" : { + "pin.location" : { + "top_left" : "dr", + "bottom_right" : "dr" + } + } + } +} +-------------------------------------------------- +// CONSOLE + +In this example, the geohash `dr` will produce the bounding box +query with the top left corner at `45.0,-78.75` and the bottom right +corner at `39.375,-67.5`. + [float] ==== Vertices diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java index 8a0c3efa5afd9..bb22cb9e01f69 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoPoint.java @@ -22,6 +22,7 @@ import org.apache.lucene.document.LatLonDocValuesField; import org.apache.lucene.document.LatLonPoint; import org.apache.lucene.geo.GeoEncodingUtils; +import org.apache.lucene.geo.Rectangle; import org.apache.lucene.index.IndexableField; import org.apache.lucene.util.BitUtil; import org.apache.lucene.util.BytesRef; @@ -85,21 +86,27 @@ public GeoPoint resetFromString(String value) { public GeoPoint resetFromString(String value, final boolean ignoreZValue) { if (value.contains(",")) { - String[] vals = value.split(","); - if (vals.length > 3) { - throw new ElasticsearchParseException("failed to parse [{}], expected 2 or 3 coordinates " - + "but found: [{}]", vals.length); - } - double lat = Double.parseDouble(vals[0].trim()); - double lon = Double.parseDouble(vals[1].trim()); - if (vals.length > 2) { - GeoPoint.assertZValue(ignoreZValue, Double.parseDouble(vals[2].trim())); - } - return reset(lat, lon); + return resetFromCoordinates(value, ignoreZValue); } return resetFromGeoHash(value); } + + public GeoPoint resetFromCoordinates(String value, final boolean ignoreZValue) { + String[] vals = value.split(","); + if (vals.length > 3) { + throw new ElasticsearchParseException("failed to parse [{}], expected 2 or 3 coordinates " + + "but found: [{}]", vals.length); + } + double lat = Double.parseDouble(vals[0].trim()); + double lon = Double.parseDouble(vals[1].trim()); + if (vals.length > 2) { + GeoPoint.assertZValue(ignoreZValue, Double.parseDouble(vals[2].trim())); + } + return reset(lat, lon); + } + + public GeoPoint resetFromIndexHash(long hash) { lon = GeoHashUtils.decodeLongitude(hash); lat = GeoHashUtils.decodeLatitude(hash); diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 57e87e06389c4..886c4d168726c 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -387,6 +387,22 @@ public static GeoPoint parseGeoPoint(Object value, final boolean ignoreZValue) t } } + /** + * Represents the point of the geohash cell that should be used as the value of geohas + */ + public enum EffectivePoint { + DEFAULT, + TOP_LEFT, + TOP_RIGHT, + BOTTOM_LEFT, + BOTTOM_RIGHT + } + + public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue) + throws IOException, ElasticsearchParseException { + return parseGeoPoint(parser, point, ignoreZValue, EffectivePoint.DEFAULT); + } + /** * Parse a {@link GeoPoint} with a {@link XContentParser}. A geopoint has one of the following forms: * @@ -401,7 +417,7 @@ public static GeoPoint parseGeoPoint(Object value, final boolean ignoreZValue) t * @param point A {@link GeoPoint} that will be reset by the values parsed * @return new {@link GeoPoint} parsed from the parse */ - public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue) + public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue, EffectivePoint effectivePoint) throws IOException, ElasticsearchParseException { double lat = Double.NaN; double lon = Double.NaN; @@ -458,7 +474,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina if(!Double.isNaN(lat) || !Double.isNaN(lon)) { throw new ElasticsearchParseException("field must be either lat/lon or geohash"); } else { - return point.resetFromGeoHash(geohash); + return parseGeoHash(point, geohash, effectivePoint); } } else if (numberFormatException != null) { throw new ElasticsearchParseException("[{}] and [{}] must be valid double values", numberFormatException, LATITUDE, @@ -489,12 +505,38 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } return point.reset(lat, lon); } else if(parser.currentToken() == Token.VALUE_STRING) { - return point.resetFromString(parser.text(), ignoreZValue); + String val = parser.text(); + if (val.contains(",")) { + return point.resetFromString(val, ignoreZValue); + } else { + return parseGeoHash(point, val, effectivePoint); + } + } else { throw new ElasticsearchParseException("geo_point expected"); } } + private static GeoPoint parseGeoHash(GeoPoint point, String geohash, EffectivePoint effectivePoint) { + if (effectivePoint == EffectivePoint.DEFAULT) { + return point.resetFromGeoHash(geohash); + } else { + Rectangle rectangle = GeoHashUtils.bbox(geohash); + switch (effectivePoint) { + case TOP_LEFT: + return point.reset(rectangle.maxLat, rectangle.minLon); + case TOP_RIGHT: + return point.reset(rectangle.maxLat, rectangle.maxLon); + case BOTTOM_LEFT: + return point.reset(rectangle.minLat, rectangle.minLon); + case BOTTOM_RIGHT: + return point.reset(rectangle.minLat, rectangle.maxLon); + default: + throw new IllegalArgumentException("Unsupported effective point " + effectivePoint); + } + } + } + /** * Parse a precision that can be expressed as an integer or a distance measure like "1km", "10m". * diff --git a/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java index 3fea896342270..53b91a7a2e14f 100644 --- a/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java @@ -491,19 +491,19 @@ public static Rectangle parseBoundingBox(XContentParser parser) throws IOExcepti right = parser.doubleValue(); } else { if (TOP_LEFT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - GeoUtils.parseGeoPoint(parser, sparse); + GeoUtils.parseGeoPoint(parser, sparse, false, GeoUtils.EffectivePoint.TOP_LEFT); top = sparse.getLat(); left = sparse.getLon(); } else if (BOTTOM_RIGHT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - GeoUtils.parseGeoPoint(parser, sparse); + GeoUtils.parseGeoPoint(parser, sparse, false, GeoUtils.EffectivePoint.BOTTOM_RIGHT); bottom = sparse.getLat(); right = sparse.getLon(); } else if (TOP_RIGHT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - GeoUtils.parseGeoPoint(parser, sparse); + GeoUtils.parseGeoPoint(parser, sparse, false, GeoUtils.EffectivePoint.TOP_RIGHT); top = sparse.getLat(); right = sparse.getLon(); } else if (BOTTOM_LEFT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { - GeoUtils.parseGeoPoint(parser, sparse); + GeoUtils.parseGeoPoint(parser, sparse, false, GeoUtils.EffectivePoint.BOTTOM_LEFT); bottom = sparse.getLat(); left = sparse.getLon(); } else { @@ -515,7 +515,7 @@ public static Rectangle parseBoundingBox(XContentParser parser) throws IOExcepti } } if (envelope != null) { - if ((Double.isNaN(top) || Double.isNaN(bottom) || Double.isNaN(left) || Double.isNaN(right)) == false) { + if ((Double.isNaN(top) && Double.isNaN(bottom) && Double.isNaN(left) && Double.isNaN(right)) == false) { throw new ElasticsearchParseException("failed to parse bounding box. Conflicting definition found " + "using well-known text and explicit corners."); } diff --git a/server/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java b/server/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java index aeaca328ceb7b..0f17609ceeee8 100644 --- a/server/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilderTests.java @@ -24,6 +24,7 @@ import org.apache.lucene.search.IndexOrDocValuesQuery; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.index.mapper.MappedFieldType; @@ -450,6 +451,64 @@ public void testFromWKT() throws IOException { assertEquals(expectedJson, GeoExecType.MEMORY, parsed.type()); } + public void testFromGeohash() throws IOException { + String json = + "{\n" + + " \"geo_bounding_box\" : {\n" + + " \"pin.location\" : {\n" + + " \"top_left\" : \"dr\",\n" + + " \"bottom_right\" : \"dq\"\n" + + " },\n" + + " \"validation_method\" : \"STRICT\",\n" + + " \"type\" : \"MEMORY\",\n" + + " \"ignore_unmapped\" : false,\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}"; + + String expectedJson = + "{\n" + + " \"geo_bounding_box\" : {\n" + + " \"pin.location\" : {\n" + + " \"top_left\" : [ -78.75, 45.0 ],\n" + + " \"bottom_right\" : [ -67.5, 33.75 ]\n" + + " },\n" + + " \"validation_method\" : \"STRICT\",\n" + + " \"type\" : \"MEMORY\",\n" + + " \"ignore_unmapped\" : false,\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}"; + GeoBoundingBoxQueryBuilder parsed = (GeoBoundingBoxQueryBuilder) parseQuery(json); + checkGeneratedJson(expectedJson, parsed); + assertEquals(json, "pin.location", parsed.fieldName()); + assertEquals(json, -78.75, parsed.topLeft().getLon(), 0.0001); + assertEquals(json, 45.0, parsed.topLeft().getLat(), 0.0001); + assertEquals(json, -67.5, parsed.bottomRight().getLon(), 0.0001); + assertEquals(json, 33.75, parsed.bottomRight().getLat(), 0.0001); + assertEquals(json, 1.0, parsed.boost(), 0.0001); + assertEquals(json, GeoExecType.MEMORY, parsed.type()); + } + + public void testMalformedGeohashes() { + String jsonGeohashAndWkt = + "{\n" + + " \"geo_bounding_box\" : {\n" + + " \"pin.location\" : {\n" + + " \"top_left\" : [ -78.75, 45.0 ],\n" + + " \"wkt\" : \"BBOX (-74.1, -71.12, 40.73, 40.01)\"\n" + + " },\n" + + " \"validation_method\" : \"STRICT\",\n" + + " \"type\" : \"MEMORY\",\n" + + " \"ignore_unmapped\" : false,\n" + + " \"boost\" : 1.0\n" + + " }\n" + + "}"; + + ElasticsearchParseException e1 = expectThrows(ElasticsearchParseException.class, () -> parseQuery(jsonGeohashAndWkt)); + assertThat(e1.getMessage(), containsString("Conflicting definition found using well-known text and explicit corners.")); + } + @Override public void testMustRewrite() throws IOException { assumeTrue("test runs only when at least a type is registered", getCurrentTypes().length > 0); diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java index 4ddb80c4b0633..7b4e30960f15a 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java @@ -610,6 +610,21 @@ public void testPrefixTreeCellSizes() { } } + public void testParseGeoPointGeohashPositions() throws IOException { + // The default behaviour is bottom left for now + assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.DEFAULT), new GeoPoint(42.71484375, -71.71875)); + assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.TOP_LEFT), new GeoPoint(42.890625, -71.71875)); + assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.TOP_RIGHT), new GeoPoint(42.890625, -71.3671875)); + assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.BOTTOM_LEFT), new GeoPoint(42.71484375, -71.71875)); + assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.BOTTOM_RIGHT), new GeoPoint(42.71484375, -71.3671875)); + } + + private GeoPoint parseGeohash(String geohash, GeoUtils.EffectivePoint effectivePoint) throws IOException { + XContentParser parser = createParser(jsonBuilder().startObject().field("geohash", geohash).endObject()); + parser.nextToken(); + return GeoUtils.parseGeoPoint(parser, new GeoPoint(), randomBoolean(), effectivePoint); + } + private static void assertNormalizedPoint(GeoPoint input, GeoPoint expected) { GeoUtils.normalizePoint(input); if (Double.isNaN(expected.lat())) { From 3f8bb1a45cd3042f6f6c7c8ec732fa46c6df67ee Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Tue, 22 May 2018 13:33:59 -0400 Subject: [PATCH 2/3] Remove EffectivePoint.DEFAULT --- .../main/java/org/elasticsearch/common/geo/GeoUtils.java | 7 ++----- .../org/elasticsearch/index/search/geo/GeoUtilsTests.java | 3 +-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 886c4d168726c..06c50a3dcf75e 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -391,7 +391,6 @@ public static GeoPoint parseGeoPoint(Object value, final boolean ignoreZValue) t * Represents the point of the geohash cell that should be used as the value of geohas */ public enum EffectivePoint { - DEFAULT, TOP_LEFT, TOP_RIGHT, BOTTOM_LEFT, @@ -400,7 +399,7 @@ public enum EffectivePoint { public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue) throws IOException, ElasticsearchParseException { - return parseGeoPoint(parser, point, ignoreZValue, EffectivePoint.DEFAULT); + return parseGeoPoint(parser, point, ignoreZValue, EffectivePoint.BOTTOM_LEFT); } /** @@ -518,7 +517,7 @@ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, fina } private static GeoPoint parseGeoHash(GeoPoint point, String geohash, EffectivePoint effectivePoint) { - if (effectivePoint == EffectivePoint.DEFAULT) { + if (effectivePoint == EffectivePoint.BOTTOM_LEFT) { return point.resetFromGeoHash(geohash); } else { Rectangle rectangle = GeoHashUtils.bbox(geohash); @@ -527,8 +526,6 @@ private static GeoPoint parseGeoHash(GeoPoint point, String geohash, EffectivePo return point.reset(rectangle.maxLat, rectangle.minLon); case TOP_RIGHT: return point.reset(rectangle.maxLat, rectangle.maxLon); - case BOTTOM_LEFT: - return point.reset(rectangle.minLat, rectangle.minLon); case BOTTOM_RIGHT: return point.reset(rectangle.minLat, rectangle.maxLon); default: diff --git a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java index 7b4e30960f15a..d390490dd225c 100644 --- a/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java +++ b/server/src/test/java/org/elasticsearch/index/search/geo/GeoUtilsTests.java @@ -611,12 +611,11 @@ public void testPrefixTreeCellSizes() { } public void testParseGeoPointGeohashPositions() throws IOException { - // The default behaviour is bottom left for now - assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.DEFAULT), new GeoPoint(42.71484375, -71.71875)); assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.TOP_LEFT), new GeoPoint(42.890625, -71.71875)); assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.TOP_RIGHT), new GeoPoint(42.890625, -71.3671875)); assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.BOTTOM_LEFT), new GeoPoint(42.71484375, -71.71875)); assertNormalizedPoint(parseGeohash("drt5", GeoUtils.EffectivePoint.BOTTOM_RIGHT), new GeoPoint(42.71484375, -71.3671875)); + assertNormalizedPoint(parseGeohash("drtk", GeoUtils.EffectivePoint.BOTTOM_LEFT), new GeoPoint(42.890625, -71.3671875)); } private GeoPoint parseGeohash(String geohash, GeoUtils.EffectivePoint effectivePoint) throws IOException { From 309f50017194f04d1eaf502fd88fbca432fd41ed Mon Sep 17 00:00:00 2001 From: Igor Motov Date: Thu, 24 May 2018 10:32:12 -0400 Subject: [PATCH 3/3] Address review comments --- .../main/java/org/elasticsearch/common/geo/GeoUtils.java | 6 +++++- .../index/query/GeoBoundingBoxQueryBuilder.java | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java index 06c50a3dcf75e..2f3443639cdb7 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -388,7 +388,7 @@ public static GeoPoint parseGeoPoint(Object value, final boolean ignoreZValue) t } /** - * Represents the point of the geohash cell that should be used as the value of geohas + * Represents the point of the geohash cell that should be used as the value of geohash */ public enum EffectivePoint { TOP_LEFT, @@ -397,6 +397,10 @@ public enum EffectivePoint { BOTTOM_RIGHT } + /** + * Parse a geopoint represented as an object, string or an array. If the geopoint is represented as a geohash, + * the left bottom corner of the geohash cell is used as the geopoint coordinates.GeoBoundingBoxQueryBuilder.java + */ public static GeoPoint parseGeoPoint(XContentParser parser, GeoPoint point, final boolean ignoreZValue) throws IOException, ElasticsearchParseException { return parseGeoPoint(parser, point, ignoreZValue, EffectivePoint.BOTTOM_LEFT); diff --git a/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java b/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java index 53b91a7a2e14f..3fd325afe0914 100644 --- a/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java @@ -515,7 +515,8 @@ public static Rectangle parseBoundingBox(XContentParser parser) throws IOExcepti } } if (envelope != null) { - if ((Double.isNaN(top) && Double.isNaN(bottom) && Double.isNaN(left) && Double.isNaN(right)) == false) { + if (Double.isNaN(top) == false || Double.isNaN(bottom) == false || Double.isNaN(left) == false || + Double.isNaN(right) == false) { throw new ElasticsearchParseException("failed to parse bounding box. Conflicting definition found " + "using well-known text and explicit corners."); }