From cceda1f1ec53abcf98edda7900ea37ba0e8da582 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Mon, 16 Dec 2019 14:22:18 -0800 Subject: [PATCH 01/12] Centralize BoundingBox logic to a dedicated class Both geo_bounding_box query and geo_bounds aggregation have a very similar definition of a "bounding box". A lot of this logic (serialization, xcontent-parsing, etc) can be centralized instead of having separated efforts to do the same things --- .../elasticsearch/common/geo/BoundingBox.java | 217 ++++++++++++++++++ .../query/GeoBoundingBoxQueryBuilder.java | 125 ++-------- .../metrics/InternalGeoBounds.java | 56 +---- .../aggregations/metrics/ParsedGeoBounds.java | 34 +-- 4 files changed, 258 insertions(+), 174 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java diff --git a/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java b/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java new file mode 100644 index 0000000000000..c37d1e8bf1f83 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java @@ -0,0 +1,217 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.common.geo; + +import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; +import org.elasticsearch.common.io.stream.Writeable; +import org.elasticsearch.common.xcontent.ToXContentObject; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.geometry.Geometry; +import org.elasticsearch.geometry.Rectangle; +import org.elasticsearch.geometry.ShapeType; + +import java.io.IOException; +import java.text.ParseException; +import java.util.Objects; + +public class BoundingBox implements ToXContentObject, Writeable { + private static final GeometryParser GEOMETRY_PARSER = new GeometryParser(true, true, true); + static final ParseField TOP_RIGHT_FIELD = new ParseField("top_right"); + static final ParseField BOTTOM_LEFT_FIELD = new ParseField("bottom_left"); + static final ParseField TOP_FIELD = new ParseField("top"); + static final ParseField BOTTOM_FIELD = new ParseField("bottom"); + static final ParseField LEFT_FIELD = new ParseField("left"); + static final ParseField RIGHT_FIELD = new ParseField("right"); + static final ParseField WKT_FIELD = new ParseField("wkt"); + public static final ParseField BOUNDS_FIELD = new ParseField("bounds"); + public static final ParseField LAT_FIELD = new ParseField("lat"); + public static final ParseField LON_FIELD = new ParseField("lon"); + public static final ParseField TOP_LEFT_FIELD = new ParseField("top_left"); + public static final ParseField BOTTOM_RIGHT_FIELD = new ParseField("bottom_right"); + + private final GeoPoint topLeft; + private final GeoPoint bottomRight; + + public BoundingBox(GeoPoint topLeft, GeoPoint bottomRight) { + this.topLeft = topLeft; + this.bottomRight = bottomRight; + } + + public BoundingBox(StreamInput input) throws IOException { + this.topLeft = input.readGeoPoint(); + this.bottomRight = input.readGeoPoint(); + } + + /** + * Parses the bounding box and returns bottom, top, left, right coordinates + */ + public static BoundingBox parseBoundingBox(XContentParser parser) throws IOException, ElasticsearchParseException { + XContentParser.Token token = parser.currentToken(); + if (token != XContentParser.Token.START_OBJECT) { + throw new ElasticsearchParseException("failed to parse bounding box. Expected start object but found [{}]", token); + } + + double top = Double.NaN; + double bottom = Double.NaN; + double left = Double.NaN; + double right = Double.NaN; + + String currentFieldName; + GeoPoint sparse = new GeoPoint(); + Rectangle envelope = null; + + while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { + if (token == XContentParser.Token.FIELD_NAME) { + currentFieldName = parser.currentName(); + token = parser.nextToken(); + if (WKT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + try { + Geometry geometry = GEOMETRY_PARSER.parse(parser); + if (ShapeType.ENVELOPE.equals(geometry.type()) == false) { + throw new ElasticsearchParseException("failed to parse WKT bounding box. [" + + geometry.type() + "] found instead"); + } + envelope = (Rectangle) geometry; + } catch (ParseException e) { + throw new ElasticsearchParseException("failed to parse WKT bounding box", e); + } + } else if (TOP_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + top = parser.doubleValue(); + } else if (BOTTOM_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + bottom = parser.doubleValue(); + } else if (LEFT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + left = parser.doubleValue(); + } else if (RIGHT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + right = parser.doubleValue(); + } else { + if (TOP_LEFT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + 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, false, GeoUtils.EffectivePoint.BOTTOM_RIGHT); + bottom = sparse.getLat(); + right = sparse.getLon(); + } else if (TOP_RIGHT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { + 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, false, GeoUtils.EffectivePoint.BOTTOM_LEFT); + bottom = sparse.getLat(); + left = sparse.getLon(); + } else { + throw new ElasticsearchParseException("failed to parse bounding box. unexpected field [{}]", currentFieldName); + } + } + } else { + throw new ElasticsearchParseException("failed to parse bounding box. field name expected but [{}] found", token); + } + } + if (envelope != null) { + 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."); + } + GeoPoint topLeft = new GeoPoint(envelope.getMaxLat(), envelope.getMinLon()); + GeoPoint bottomRight = new GeoPoint(envelope.getMinLat(), envelope.getMaxLon()); + return new BoundingBox(topLeft, bottomRight); + } + GeoPoint topLeft = new GeoPoint(top, left); + GeoPoint bottomRight = new GeoPoint(bottom, right); + return new BoundingBox(topLeft, bottomRight); + } + + public GeoPoint topLeft() { + return topLeft; + } + + public GeoPoint bottomRight() { + return bottomRight; + } + + public double top() { + return topLeft.lat(); + } + + public double bottom() { + return bottomRight.lat(); + } + + public double left() { + return topLeft.lon(); + } + + public double right() { + return bottomRight.lon(); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(BOUNDS_FIELD.getPreferredName()); + toXContentFragment(builder, true); + builder.endObject(); + return null; + } + + public XContentBuilder toXContentFragment(XContentBuilder builder, boolean buildLatLonFields) throws IOException { + if (buildLatLonFields) { + builder.startObject(TOP_LEFT_FIELD.getPreferredName()); + builder.field(LAT_FIELD.getPreferredName(), topLeft.lat()); + builder.field(LON_FIELD.getPreferredName(), topLeft.lon()); + builder.endObject(); + } else { + builder.array(TOP_LEFT_FIELD.getPreferredName(), topLeft.lon(), topLeft.lat()); + } + if (buildLatLonFields) { + builder.startObject(BOTTOM_RIGHT_FIELD.getPreferredName()); + builder.field(LAT_FIELD.getPreferredName(), bottomRight.lat()); + builder.field(LON_FIELD.getPreferredName(), bottomRight.lon()); + builder.endObject(); + } else { + builder.array(BOTTOM_RIGHT_FIELD.getPreferredName(), bottomRight.lon(), bottomRight.lat()); + } + return builder; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeGeoPoint(topLeft); + out.writeGeoPoint(bottomRight); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + BoundingBox that = (BoundingBox) o; + return topLeft.equals(that.topLeft) && + bottomRight.equals(that.bottomRight); + } + + @Override + public int hashCode() { + return Objects.hash(topLeft, bottomRight); + } +} 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 699d4b79aa72b..14fcc314b9d72 100644 --- a/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java @@ -21,7 +21,6 @@ import org.apache.lucene.document.LatLonDocValuesField; import org.apache.lucene.document.LatLonPoint; -//import org.apache.lucene.geo.Rectangle; import org.apache.lucene.search.IndexOrDocValuesQuery; import org.apache.lucene.search.MatchNoDocsQuery; import org.apache.lucene.search.Query; @@ -29,11 +28,9 @@ import org.elasticsearch.common.Numbers; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; +import org.elasticsearch.common.geo.BoundingBox; import org.elasticsearch.common.geo.GeoPoint; -import org.elasticsearch.common.geo.GeoShapeType; import org.elasticsearch.common.geo.GeoUtils; -import org.elasticsearch.common.geo.builders.EnvelopeBuilder; -import org.elasticsearch.common.geo.parsers.GeoWKTParser; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -66,24 +63,12 @@ public class GeoBoundingBoxQueryBuilder extends AbstractQueryBuilder path) { String bBoxSide = path.get(0); switch (bBoxSide) { case "top": - return boundingBox.topLeft.lat(); + return boundingBox.top(); case "left": - return boundingBox.topLeft.lon(); + return boundingBox.left(); case "bottom": - return boundingBox.bottomRight.lat(); + return boundingBox.bottom(); case "right": - return boundingBox.bottomRight.lon(); + return boundingBox.right(); default: throw new IllegalArgumentException("Found unknown path element [" + bBoxSide + "] in [" + getName() + "]"); } @@ -152,10 +144,10 @@ public Object getProperty(List path) { String cornerString = path.get(0); switch (cornerString) { case "top_left": - cornerPoint = boundingBox.topLeft; + cornerPoint = boundingBox.topLeft(); break; case "bottom_right": - cornerPoint = boundingBox.bottomRight; + cornerPoint = boundingBox.bottomRight(); break; default: throw new IllegalArgumentException("Found unknown path element [" + cornerString + "] in [" + getName() + "]"); @@ -176,41 +168,13 @@ public Object getProperty(List path) { @Override public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - GeoPoint topLeft = topLeft(); - GeoPoint bottomRight = bottomRight(); - if (topLeft != null) { - builder.startObject(BOUNDS_FIELD.getPreferredName()); - builder.startObject(TOP_LEFT_FIELD.getPreferredName()); - builder.field(LAT_FIELD.getPreferredName(), topLeft.lat()); - builder.field(LON_FIELD.getPreferredName(), topLeft.lon()); - builder.endObject(); - builder.startObject(BOTTOM_RIGHT_FIELD.getPreferredName()); - builder.field(LAT_FIELD.getPreferredName(), bottomRight.lat()); - builder.field(LON_FIELD.getPreferredName(), bottomRight.lon()); - builder.endObject(); - builder.endObject(); + BoundingBox bbox = resolveBoundingBox(); + if (bbox != null) { + bbox.toXContent(builder, params); } return builder; } - private static class BoundingBox { - private final GeoPoint topLeft; - private final GeoPoint bottomRight; - - BoundingBox(GeoPoint topLeft, GeoPoint bottomRight) { - this.topLeft = topLeft; - this.bottomRight = bottomRight; - } - - public GeoPoint topLeft() { - return topLeft; - } - - public GeoPoint bottomRight() { - return bottomRight; - } - } - private BoundingBox resolveBoundingBox() { if (Double.isInfinite(top)) { return null; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java index 11d36d2ceeead..635b853d466b9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.common.collect.Tuple; +import org.elasticsearch.common.geo.BoundingBox; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; @@ -30,15 +31,14 @@ import java.io.IOException; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; -import static org.elasticsearch.search.aggregations.metrics.InternalGeoBounds.BOTTOM_RIGHT_FIELD; -import static org.elasticsearch.search.aggregations.metrics.InternalGeoBounds.BOUNDS_FIELD; -import static org.elasticsearch.search.aggregations.metrics.InternalGeoBounds.LAT_FIELD; -import static org.elasticsearch.search.aggregations.metrics.InternalGeoBounds.LON_FIELD; -import static org.elasticsearch.search.aggregations.metrics.InternalGeoBounds.TOP_LEFT_FIELD; +import static org.elasticsearch.common.geo.BoundingBox.BOTTOM_RIGHT_FIELD; +import static org.elasticsearch.common.geo.BoundingBox.BOUNDS_FIELD; +import static org.elasticsearch.common.geo.BoundingBox.LAT_FIELD; +import static org.elasticsearch.common.geo.BoundingBox.LON_FIELD; +import static org.elasticsearch.common.geo.BoundingBox.TOP_LEFT_FIELD; public class ParsedGeoBounds extends ParsedAggregation implements GeoBounds { - private GeoPoint topLeft; - private GeoPoint bottomRight; + private BoundingBox boundingBox; @Override public String getType() { @@ -47,29 +47,20 @@ public String getType() { @Override public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - if (topLeft != null) { - builder.startObject(BOUNDS_FIELD.getPreferredName()); - builder.startObject(TOP_LEFT_FIELD.getPreferredName()); - builder.field(LAT_FIELD.getPreferredName(), topLeft.getLat()); - builder.field(LON_FIELD.getPreferredName(), topLeft.getLon()); - builder.endObject(); - builder.startObject(BOTTOM_RIGHT_FIELD.getPreferredName()); - builder.field(LAT_FIELD.getPreferredName(), bottomRight.getLat()); - builder.field(LON_FIELD.getPreferredName(), bottomRight.getLon()); - builder.endObject(); - builder.endObject(); + if (boundingBox != null) { + boundingBox.toXContent(builder, params); } return builder; } @Override public GeoPoint topLeft() { - return topLeft; + return boundingBox.topLeft(); } @Override public GeoPoint bottomRight() { - return bottomRight; + return boundingBox.bottomRight(); } private static final ObjectParser PARSER = new ObjectParser<>(ParsedGeoBounds.class.getSimpleName(), true, @@ -85,8 +76,7 @@ public GeoPoint bottomRight() { static { declareAggregationFields(PARSER); PARSER.declareObject((agg, bbox) -> { - agg.topLeft = bbox.v1(); - agg.bottomRight = bbox.v2(); + agg.boundingBox = new BoundingBox(bbox.v1(), bbox.v2()); }, BOUNDS_PARSER, BOUNDS_FIELD); BOUNDS_PARSER.declareObject(constructorArg(), GEO_POINT_PARSER, TOP_LEFT_FIELD); From 216348e1ae5274c2a33d94ab3bbf6f73385b36cc Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Tue, 17 Dec 2019 12:33:58 -0800 Subject: [PATCH 02/12] add tests --- .../elasticsearch/common/geo/BoundingBox.java | 162 ++++++++++-------- .../common/geo/BoundingBoxTests.java | 139 +++++++++++++++ 2 files changed, 226 insertions(+), 75 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java diff --git a/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java b/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java index c37d1e8bf1f83..729c27c14b843 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java +++ b/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java @@ -29,13 +29,19 @@ import org.elasticsearch.geometry.Geometry; import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.geometry.ShapeType; +import org.elasticsearch.geometry.utils.StandardValidator; +import org.elasticsearch.geometry.utils.WellKnownText; import java.io.IOException; import java.text.ParseException; import java.util.Objects; +/** + * A class representing a Geo-Bounding-Box for use by Geo queries and aggregations + * that deal with extents/rectangles representing rectangular areas of interest. + */ public class BoundingBox implements ToXContentObject, Writeable { - private static final GeometryParser GEOMETRY_PARSER = new GeometryParser(true, true, true); + private static final WellKnownText WKT_PARSER = new WellKnownText(true, new StandardValidator(true)); static final ParseField TOP_RIGHT_FIELD = new ParseField("top_right"); static final ParseField BOTTOM_LEFT_FIELD = new ParseField("bottom_left"); static final ParseField TOP_FIELD = new ParseField("top"); @@ -62,6 +68,83 @@ public BoundingBox(StreamInput input) throws IOException { this.bottomRight = input.readGeoPoint(); } + public GeoPoint topLeft() { + return topLeft; + } + + public GeoPoint bottomRight() { + return bottomRight; + } + + public double top() { + return topLeft.lat(); + } + + public double bottom() { + return bottomRight.lat(); + } + + public double left() { + return topLeft.lon(); + } + + public double right() { + return bottomRight.lon(); + } + + @Override + public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { + builder.startObject(BOUNDS_FIELD.getPreferredName()); + toXContentFragment(builder, true); + builder.endObject(); + return builder; + } + + public XContentBuilder toXContentFragment(XContentBuilder builder, boolean buildLatLonFields) throws IOException { + if (buildLatLonFields) { + builder.startObject(TOP_LEFT_FIELD.getPreferredName()); + builder.field(LAT_FIELD.getPreferredName(), topLeft.lat()); + builder.field(LON_FIELD.getPreferredName(), topLeft.lon()); + builder.endObject(); + } else { + builder.array(TOP_LEFT_FIELD.getPreferredName(), topLeft.lon(), topLeft.lat()); + } + if (buildLatLonFields) { + builder.startObject(BOTTOM_RIGHT_FIELD.getPreferredName()); + builder.field(LAT_FIELD.getPreferredName(), bottomRight.lat()); + builder.field(LON_FIELD.getPreferredName(), bottomRight.lon()); + builder.endObject(); + } else { + builder.array(BOTTOM_RIGHT_FIELD.getPreferredName(), bottomRight.lon(), bottomRight.lat()); + } + return builder; + } + + @Override + public void writeTo(StreamOutput out) throws IOException { + out.writeGeoPoint(topLeft); + out.writeGeoPoint(bottomRight); + } + + @Override + public boolean equals(Object o) { + if (this == o) return true; + if (o == null || getClass() != o.getClass()) return false; + BoundingBox that = (BoundingBox) o; + return topLeft.equals(that.topLeft) && + bottomRight.equals(that.bottomRight); + } + + @Override + public int hashCode() { + return Objects.hash(topLeft, bottomRight); + } + + @Override + public String toString() { + return "BBOX (" + topLeft.lon() + ", " + bottomRight.lon() + ", " + topLeft.lat() + ", " + bottomRight.lat() + ")"; + } + /** * Parses the bounding box and returns bottom, top, left, right coordinates */ @@ -86,13 +169,13 @@ public static BoundingBox parseBoundingBox(XContentParser parser) throws IOExcep token = parser.nextToken(); if (WKT_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { try { - Geometry geometry = GEOMETRY_PARSER.parse(parser); + Geometry geometry = WKT_PARSER.fromWKT(parser.text()); if (ShapeType.ENVELOPE.equals(geometry.type()) == false) { throw new ElasticsearchParseException("failed to parse WKT bounding box. [" - + geometry.type() + "] found instead"); + + geometry.type() + "] found. expected [" + ShapeType.ENVELOPE + "]"); } envelope = (Rectangle) geometry; - } catch (ParseException e) { + } catch (ParseException|IllegalArgumentException e) { throw new ElasticsearchParseException("failed to parse WKT bounding box", e); } } else if (TOP_FIELD.match(currentFieldName, parser.getDeprecationHandler())) { @@ -143,75 +226,4 @@ public static BoundingBox parseBoundingBox(XContentParser parser) throws IOExcep return new BoundingBox(topLeft, bottomRight); } - public GeoPoint topLeft() { - return topLeft; - } - - public GeoPoint bottomRight() { - return bottomRight; - } - - public double top() { - return topLeft.lat(); - } - - public double bottom() { - return bottomRight.lat(); - } - - public double left() { - return topLeft.lon(); - } - - public double right() { - return bottomRight.lon(); - } - - @Override - public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { - builder.startObject(BOUNDS_FIELD.getPreferredName()); - toXContentFragment(builder, true); - builder.endObject(); - return null; - } - - public XContentBuilder toXContentFragment(XContentBuilder builder, boolean buildLatLonFields) throws IOException { - if (buildLatLonFields) { - builder.startObject(TOP_LEFT_FIELD.getPreferredName()); - builder.field(LAT_FIELD.getPreferredName(), topLeft.lat()); - builder.field(LON_FIELD.getPreferredName(), topLeft.lon()); - builder.endObject(); - } else { - builder.array(TOP_LEFT_FIELD.getPreferredName(), topLeft.lon(), topLeft.lat()); - } - if (buildLatLonFields) { - builder.startObject(BOTTOM_RIGHT_FIELD.getPreferredName()); - builder.field(LAT_FIELD.getPreferredName(), bottomRight.lat()); - builder.field(LON_FIELD.getPreferredName(), bottomRight.lon()); - builder.endObject(); - } else { - builder.array(BOTTOM_RIGHT_FIELD.getPreferredName(), bottomRight.lon(), bottomRight.lat()); - } - return builder; - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - out.writeGeoPoint(topLeft); - out.writeGeoPoint(bottomRight); - } - - @Override - public boolean equals(Object o) { - if (this == o) return true; - if (o == null || getClass() != o.getClass()) return false; - BoundingBox that = (BoundingBox) o; - return topLeft.equals(that.topLeft) && - bottomRight.equals(that.bottomRight); - } - - @Override - public int hashCode() { - return Objects.hash(topLeft, bottomRight); - } } diff --git a/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java b/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java new file mode 100644 index 0000000000000..fe30bb919ce76 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java @@ -0,0 +1,139 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.common.geo; + +import org.elasticsearch.ElasticsearchParseException; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.test.ESTestCase; + +import java.io.IOException; +import java.util.List; + +import static org.hamcrest.Matchers.equalTo; + + +/** + * Tests for {@code GeoJSONShapeParser} + */ +public class BoundingBoxTests extends ESTestCase { + + public void testInvalidParseInvalidWKT() throws IOException { + XContentBuilder bboxBuilder = XContentFactory.jsonBuilder() + .startObject() + .field("wkt", "invalid") + .endObject(); + XContentParser parser = createParser(bboxBuilder); + parser.nextToken(); + ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> BoundingBox.parseBoundingBox(parser)); + assertThat(e.getMessage(), equalTo("failed to parse WKT bounding box")); + } + + public void testInvalidParsePoint() throws IOException { + XContentBuilder bboxBuilder = XContentFactory.jsonBuilder() + .startObject() + .field("wkt", "POINT (100.0 100.0)") + .endObject(); + XContentParser parser = createParser(bboxBuilder); + parser.nextToken(); + ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> BoundingBox.parseBoundingBox(parser)); + assertThat(e.getMessage(), equalTo("failed to parse WKT bounding box. [POINT] found. expected [ENVELOPE]")); + } + + public void testWKT() throws IOException { + BoundingBox boundingBox = randomBBox(); + assertBBox(boundingBox, + XContentFactory.jsonBuilder() + .startObject() + .field("wkt", boundingBox.toString()) + .endObject() + ); + } + + public void testTopBottomLeftRight() throws Exception { + BoundingBox boundingBox = randomBBox(); + assertBBox(boundingBox, + XContentFactory.jsonBuilder() + .startObject() + .field("top", boundingBox.top()) + .field("bottom", boundingBox.bottom()) + .field("left", boundingBox.left()) + .field("right", boundingBox.right()) + .endObject() + ); + } + + public void testTopLeftBottomRight() throws Exception { + BoundingBox boundingBox = randomBBox(); + assertBBox(boundingBox, + XContentFactory.jsonBuilder() + .startObject() + .field("top_left", boundingBox.topLeft()) + .field("bottom_right", boundingBox.bottomRight()) + .endObject() + ); + } + + public void testTopRightBottomLeft() throws Exception { + BoundingBox boundingBox = randomBBox(); + assertBBox(boundingBox, + XContentFactory.jsonBuilder() + .startObject() + .field("top_right", new GeoPoint(boundingBox.top(), boundingBox.right())) + .field("bottom_left", new GeoPoint(boundingBox.bottom(), boundingBox.left())) + .endObject() + ); + } + + // test that no exception is thrown. BBOX parsing is not validated + public void testNullTopBottomLeftRight() throws Exception { + BoundingBox boundingBox = randomBBox(); + XContentBuilder builder = XContentFactory.jsonBuilder().startObject(); + for (String field : randomSubsetOf(List.of("top", "bottom", "left", "right"))) { + switch (field) { + case "top": builder.field("top", boundingBox.top()); break; + case "bottom": builder.field("bottom", boundingBox.bottom()); break; + case "left": builder.field("left", boundingBox.left()); break; + case "right": builder.field("right", boundingBox.right()); break; + } + } + builder.endObject(); + try (XContentParser parser = createParser(builder)) { + parser.nextToken(); + BoundingBox.parseBoundingBox(parser); + } + } + + private void assertBBox(BoundingBox expected, XContentBuilder builder) throws IOException { + try (XContentParser parser = createParser(builder)) { + parser.nextToken(); + assertThat(BoundingBox.parseBoundingBox(parser), equalTo(expected)); + } + } + + private BoundingBox randomBBox() { + double topLat = GeometryTestUtils.randomLat(); + double bottomLat = randomDoubleBetween(GeoUtils.MIN_LAT, topLat, true); + return new BoundingBox(new GeoPoint(topLat, GeometryTestUtils.randomLon()), + new GeoPoint(bottomLat, GeometryTestUtils.randomLon())); + } +} From 6eab3fa148451cde95696c04492e9738b96b922d Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Tue, 17 Dec 2019 13:26:29 -0800 Subject: [PATCH 03/12] fix checkstyle --- .../common/geo/BoundingBoxTests.java | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java b/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java index fe30bb919ce76..252b42f825d57 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java @@ -110,10 +110,20 @@ public void testNullTopBottomLeftRight() throws Exception { XContentBuilder builder = XContentFactory.jsonBuilder().startObject(); for (String field : randomSubsetOf(List.of("top", "bottom", "left", "right"))) { switch (field) { - case "top": builder.field("top", boundingBox.top()); break; - case "bottom": builder.field("bottom", boundingBox.bottom()); break; - case "left": builder.field("left", boundingBox.left()); break; - case "right": builder.field("right", boundingBox.right()); break; + case "top": + builder.field("top", boundingBox.top()); + break; + case "bottom": + builder.field("bottom", boundingBox.bottom()); + break; + case "left": + builder.field("left", boundingBox.left()); + break; + case "right": + builder.field("right", boundingBox.right()); + break; + default: + throw new IllegalStateException("unexpected branching"); } } builder.endObject(); From 4371bdfc6d1b9089be87c0de0cee3a7ecff7eee2 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Wed, 18 Dec 2019 09:45:31 -0800 Subject: [PATCH 04/12] rename BoundingBox -> GeoBoundingBox --- .../{BoundingBox.java => GeoBoundingBox.java} | 14 ++--- .../query/GeoBoundingBoxQueryBuilder.java | 34 +++++------ .../metrics/InternalGeoBounds.java | 44 +++++++------- .../aggregations/metrics/ParsedGeoBounds.java | 24 ++++---- ...BoxTests.java => GeoBoundingBoxTests.java} | 60 +++++++++---------- ...gBoxIT.java => GeoBoundingBoxQueryIT.java} | 2 +- 6 files changed, 89 insertions(+), 89 deletions(-) rename server/src/main/java/org/elasticsearch/common/geo/{BoundingBox.java => GeoBoundingBox.java} (95%) rename server/src/test/java/org/elasticsearch/common/geo/{BoundingBoxTests.java => GeoBoundingBoxTests.java} (69%) rename server/src/test/java/org/elasticsearch/search/geo/{GeoBoundingBoxIT.java => GeoBoundingBoxQueryIT.java} (99%) diff --git a/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java b/server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java similarity index 95% rename from server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java rename to server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java index 729c27c14b843..78e81925275d7 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/BoundingBox.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java @@ -40,7 +40,7 @@ * A class representing a Geo-Bounding-Box for use by Geo queries and aggregations * that deal with extents/rectangles representing rectangular areas of interest. */ -public class BoundingBox implements ToXContentObject, Writeable { +public class GeoBoundingBox implements ToXContentObject, Writeable { private static final WellKnownText WKT_PARSER = new WellKnownText(true, new StandardValidator(true)); static final ParseField TOP_RIGHT_FIELD = new ParseField("top_right"); static final ParseField BOTTOM_LEFT_FIELD = new ParseField("bottom_left"); @@ -58,12 +58,12 @@ public class BoundingBox implements ToXContentObject, Writeable { private final GeoPoint topLeft; private final GeoPoint bottomRight; - public BoundingBox(GeoPoint topLeft, GeoPoint bottomRight) { + public GeoBoundingBox(GeoPoint topLeft, GeoPoint bottomRight) { this.topLeft = topLeft; this.bottomRight = bottomRight; } - public BoundingBox(StreamInput input) throws IOException { + public GeoBoundingBox(StreamInput input) throws IOException { this.topLeft = input.readGeoPoint(); this.bottomRight = input.readGeoPoint(); } @@ -130,7 +130,7 @@ public void writeTo(StreamOutput out) throws IOException { public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; - BoundingBox that = (BoundingBox) o; + GeoBoundingBox that = (GeoBoundingBox) o; return topLeft.equals(that.topLeft) && bottomRight.equals(that.bottomRight); } @@ -148,7 +148,7 @@ public String toString() { /** * Parses the bounding box and returns bottom, top, left, right coordinates */ - public static BoundingBox parseBoundingBox(XContentParser parser) throws IOException, ElasticsearchParseException { + public static GeoBoundingBox parseBoundingBox(XContentParser parser) throws IOException, ElasticsearchParseException { XContentParser.Token token = parser.currentToken(); if (token != XContentParser.Token.START_OBJECT) { throw new ElasticsearchParseException("failed to parse bounding box. Expected start object but found [{}]", token); @@ -219,11 +219,11 @@ public static BoundingBox parseBoundingBox(XContentParser parser) throws IOExcep } GeoPoint topLeft = new GeoPoint(envelope.getMaxLat(), envelope.getMinLon()); GeoPoint bottomRight = new GeoPoint(envelope.getMinLat(), envelope.getMaxLon()); - return new BoundingBox(topLeft, bottomRight); + return new GeoBoundingBox(topLeft, bottomRight); } GeoPoint topLeft = new GeoPoint(top, left); GeoPoint bottomRight = new GeoPoint(bottom, right); - return new BoundingBox(topLeft, bottomRight); + return new GeoBoundingBox(topLeft, bottomRight); } } 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 14fcc314b9d72..59025dc03e161 100644 --- a/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java +++ b/server/src/main/java/org/elasticsearch/index/query/GeoBoundingBoxQueryBuilder.java @@ -28,7 +28,7 @@ import org.elasticsearch.common.Numbers; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.ParsingException; -import org.elasticsearch.common.geo.BoundingBox; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.common.io.stream.StreamInput; @@ -68,7 +68,7 @@ public class GeoBoundingBoxQueryBuilder extends AbstractQueryBuilder path) { if (path.isEmpty()) { return this; } else if (path.size() == 1) { - BoundingBox boundingBox = resolveBoundingBox(); + GeoBoundingBox geoBoundingBox = resolveGeoBoundingBox(); String bBoxSide = path.get(0); switch (bBoxSide) { case "top": - return boundingBox.top(); + return geoBoundingBox.top(); case "left": - return boundingBox.left(); + return geoBoundingBox.left(); case "bottom": - return boundingBox.bottom(); + return geoBoundingBox.bottom(); case "right": - return boundingBox.right(); + return geoBoundingBox.right(); default: throw new IllegalArgumentException("Found unknown path element [" + bBoxSide + "] in [" + getName() + "]"); } } else if (path.size() == 2) { - BoundingBox boundingBox = resolveBoundingBox(); + GeoBoundingBox geoBoundingBox = resolveGeoBoundingBox(); GeoPoint cornerPoint = null; String cornerString = path.get(0); switch (cornerString) { case "top_left": - cornerPoint = boundingBox.topLeft(); + cornerPoint = geoBoundingBox.topLeft(); break; case "bottom_right": - cornerPoint = boundingBox.bottomRight(); + cornerPoint = geoBoundingBox.bottomRight(); break; default: throw new IllegalArgumentException("Found unknown path element [" + cornerString + "] in [" + getName() + "]"); @@ -168,50 +168,50 @@ public Object getProperty(List path) { @Override public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - BoundingBox bbox = resolveBoundingBox(); + GeoBoundingBox bbox = resolveGeoBoundingBox(); if (bbox != null) { bbox.toXContent(builder, params); } return builder; } - private BoundingBox resolveBoundingBox() { + private GeoBoundingBox resolveGeoBoundingBox() { if (Double.isInfinite(top)) { return null; } else if (Double.isInfinite(posLeft)) { - return new BoundingBox(new GeoPoint(top, negLeft), new GeoPoint(bottom, negRight)); + return new GeoBoundingBox(new GeoPoint(top, negLeft), new GeoPoint(bottom, negRight)); } else if (Double.isInfinite(negLeft)) { - return new BoundingBox(new GeoPoint(top, posLeft), new GeoPoint(bottom, posRight)); + return new GeoBoundingBox(new GeoPoint(top, posLeft), new GeoPoint(bottom, posRight)); } else if (wrapLongitude) { double unwrappedWidth = posRight - negLeft; double wrappedWidth = (180 - posLeft) - (-180 - negRight); if (unwrappedWidth <= wrappedWidth) { - return new BoundingBox(new GeoPoint(top, negLeft), new GeoPoint(bottom, posRight)); + return new GeoBoundingBox(new GeoPoint(top, negLeft), new GeoPoint(bottom, posRight)); } else { - return new BoundingBox(new GeoPoint(top, posLeft), new GeoPoint(bottom, negRight)); + return new GeoBoundingBox(new GeoPoint(top, posLeft), new GeoPoint(bottom, negRight)); } } else { - return new BoundingBox(new GeoPoint(top, negLeft), new GeoPoint(bottom, posRight)); + return new GeoBoundingBox(new GeoPoint(top, negLeft), new GeoPoint(bottom, posRight)); } } @Override public GeoPoint topLeft() { - BoundingBox boundingBox = resolveBoundingBox(); - if (boundingBox == null) { + GeoBoundingBox geoBoundingBox = resolveGeoBoundingBox(); + if (geoBoundingBox == null) { return null; } else { - return boundingBox.topLeft(); + return geoBoundingBox.topLeft(); } } @Override public GeoPoint bottomRight() { - BoundingBox boundingBox = resolveBoundingBox(); - if (boundingBox == null) { + GeoBoundingBox geoBoundingBox = resolveGeoBoundingBox(); + if (geoBoundingBox == null) { return null; } else { - return boundingBox.bottomRight(); + return geoBoundingBox.bottomRight(); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java index 635b853d466b9..2b29ae15119d9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ParsedGeoBounds.java @@ -20,7 +20,7 @@ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.common.collect.Tuple; -import org.elasticsearch.common.geo.BoundingBox; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.xcontent.ConstructingObjectParser; import org.elasticsearch.common.xcontent.ObjectParser; @@ -31,14 +31,14 @@ import java.io.IOException; import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg; -import static org.elasticsearch.common.geo.BoundingBox.BOTTOM_RIGHT_FIELD; -import static org.elasticsearch.common.geo.BoundingBox.BOUNDS_FIELD; -import static org.elasticsearch.common.geo.BoundingBox.LAT_FIELD; -import static org.elasticsearch.common.geo.BoundingBox.LON_FIELD; -import static org.elasticsearch.common.geo.BoundingBox.TOP_LEFT_FIELD; +import static org.elasticsearch.common.geo.GeoBoundingBox.BOTTOM_RIGHT_FIELD; +import static org.elasticsearch.common.geo.GeoBoundingBox.BOUNDS_FIELD; +import static org.elasticsearch.common.geo.GeoBoundingBox.LAT_FIELD; +import static org.elasticsearch.common.geo.GeoBoundingBox.LON_FIELD; +import static org.elasticsearch.common.geo.GeoBoundingBox.TOP_LEFT_FIELD; public class ParsedGeoBounds extends ParsedAggregation implements GeoBounds { - private BoundingBox boundingBox; + private GeoBoundingBox geoBoundingBox; @Override public String getType() { @@ -47,20 +47,20 @@ public String getType() { @Override public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException { - if (boundingBox != null) { - boundingBox.toXContent(builder, params); + if (geoBoundingBox != null) { + geoBoundingBox.toXContent(builder, params); } return builder; } @Override public GeoPoint topLeft() { - return boundingBox.topLeft(); + return geoBoundingBox.topLeft(); } @Override public GeoPoint bottomRight() { - return boundingBox.bottomRight(); + return geoBoundingBox.bottomRight(); } private static final ObjectParser PARSER = new ObjectParser<>(ParsedGeoBounds.class.getSimpleName(), true, @@ -76,7 +76,7 @@ public GeoPoint bottomRight() { static { declareAggregationFields(PARSER); PARSER.declareObject((agg, bbox) -> { - agg.boundingBox = new BoundingBox(bbox.v1(), bbox.v2()); + agg.geoBoundingBox = new GeoBoundingBox(bbox.v1(), bbox.v2()); }, BOUNDS_PARSER, BOUNDS_FIELD); BOUNDS_PARSER.declareObject(constructorArg(), GEO_POINT_PARSER, TOP_LEFT_FIELD); diff --git a/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java similarity index 69% rename from server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java rename to server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java index 252b42f825d57..da7758a720022 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/BoundingBoxTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java @@ -35,7 +35,7 @@ /** * Tests for {@code GeoJSONShapeParser} */ -public class BoundingBoxTests extends ESTestCase { +public class GeoBoundingBoxTests extends ESTestCase { public void testInvalidParseInvalidWKT() throws IOException { XContentBuilder bboxBuilder = XContentFactory.jsonBuilder() @@ -44,7 +44,7 @@ public void testInvalidParseInvalidWKT() throws IOException { .endObject(); XContentParser parser = createParser(bboxBuilder); parser.nextToken(); - ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> BoundingBox.parseBoundingBox(parser)); + ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> GeoBoundingBox.parseBoundingBox(parser)); assertThat(e.getMessage(), equalTo("failed to parse WKT bounding box")); } @@ -55,72 +55,72 @@ public void testInvalidParsePoint() throws IOException { .endObject(); XContentParser parser = createParser(bboxBuilder); parser.nextToken(); - ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> BoundingBox.parseBoundingBox(parser)); + ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> GeoBoundingBox.parseBoundingBox(parser)); assertThat(e.getMessage(), equalTo("failed to parse WKT bounding box. [POINT] found. expected [ENVELOPE]")); } public void testWKT() throws IOException { - BoundingBox boundingBox = randomBBox(); - assertBBox(boundingBox, + GeoBoundingBox geoBoundingBox = randomBBox(); + assertBBox(geoBoundingBox, XContentFactory.jsonBuilder() .startObject() - .field("wkt", boundingBox.toString()) + .field("wkt", geoBoundingBox.toString()) .endObject() ); } public void testTopBottomLeftRight() throws Exception { - BoundingBox boundingBox = randomBBox(); - assertBBox(boundingBox, + GeoBoundingBox geoBoundingBox = randomBBox(); + assertBBox(geoBoundingBox, XContentFactory.jsonBuilder() .startObject() - .field("top", boundingBox.top()) - .field("bottom", boundingBox.bottom()) - .field("left", boundingBox.left()) - .field("right", boundingBox.right()) + .field("top", geoBoundingBox.top()) + .field("bottom", geoBoundingBox.bottom()) + .field("left", geoBoundingBox.left()) + .field("right", geoBoundingBox.right()) .endObject() ); } public void testTopLeftBottomRight() throws Exception { - BoundingBox boundingBox = randomBBox(); - assertBBox(boundingBox, + GeoBoundingBox geoBoundingBox = randomBBox(); + assertBBox(geoBoundingBox, XContentFactory.jsonBuilder() .startObject() - .field("top_left", boundingBox.topLeft()) - .field("bottom_right", boundingBox.bottomRight()) + .field("top_left", geoBoundingBox.topLeft()) + .field("bottom_right", geoBoundingBox.bottomRight()) .endObject() ); } public void testTopRightBottomLeft() throws Exception { - BoundingBox boundingBox = randomBBox(); - assertBBox(boundingBox, + GeoBoundingBox geoBoundingBox = randomBBox(); + assertBBox(geoBoundingBox, XContentFactory.jsonBuilder() .startObject() - .field("top_right", new GeoPoint(boundingBox.top(), boundingBox.right())) - .field("bottom_left", new GeoPoint(boundingBox.bottom(), boundingBox.left())) + .field("top_right", new GeoPoint(geoBoundingBox.top(), geoBoundingBox.right())) + .field("bottom_left", new GeoPoint(geoBoundingBox.bottom(), geoBoundingBox.left())) .endObject() ); } // test that no exception is thrown. BBOX parsing is not validated public void testNullTopBottomLeftRight() throws Exception { - BoundingBox boundingBox = randomBBox(); + GeoBoundingBox geoBoundingBox = randomBBox(); XContentBuilder builder = XContentFactory.jsonBuilder().startObject(); for (String field : randomSubsetOf(List.of("top", "bottom", "left", "right"))) { switch (field) { case "top": - builder.field("top", boundingBox.top()); + builder.field("top", geoBoundingBox.top()); break; case "bottom": - builder.field("bottom", boundingBox.bottom()); + builder.field("bottom", geoBoundingBox.bottom()); break; case "left": - builder.field("left", boundingBox.left()); + builder.field("left", geoBoundingBox.left()); break; case "right": - builder.field("right", boundingBox.right()); + builder.field("right", geoBoundingBox.right()); break; default: throw new IllegalStateException("unexpected branching"); @@ -129,21 +129,21 @@ public void testNullTopBottomLeftRight() throws Exception { builder.endObject(); try (XContentParser parser = createParser(builder)) { parser.nextToken(); - BoundingBox.parseBoundingBox(parser); + GeoBoundingBox.parseBoundingBox(parser); } } - private void assertBBox(BoundingBox expected, XContentBuilder builder) throws IOException { + private void assertBBox(GeoBoundingBox expected, XContentBuilder builder) throws IOException { try (XContentParser parser = createParser(builder)) { parser.nextToken(); - assertThat(BoundingBox.parseBoundingBox(parser), equalTo(expected)); + assertThat(GeoBoundingBox.parseBoundingBox(parser), equalTo(expected)); } } - private BoundingBox randomBBox() { + private GeoBoundingBox randomBBox() { double topLat = GeometryTestUtils.randomLat(); double bottomLat = randomDoubleBetween(GeoUtils.MIN_LAT, topLat, true); - return new BoundingBox(new GeoPoint(topLat, GeometryTestUtils.randomLon()), + return new GeoBoundingBox(new GeoPoint(topLat, GeometryTestUtils.randomLon()), new GeoPoint(bottomLat, GeometryTestUtils.randomLon())); } } diff --git a/server/src/test/java/org/elasticsearch/search/geo/GeoBoundingBoxIT.java b/server/src/test/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIT.java similarity index 99% rename from server/src/test/java/org/elasticsearch/search/geo/GeoBoundingBoxIT.java rename to server/src/test/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIT.java index 6e3f853e90eab..fe7800e137c9c 100644 --- a/server/src/test/java/org/elasticsearch/search/geo/GeoBoundingBoxIT.java +++ b/server/src/test/java/org/elasticsearch/search/geo/GeoBoundingBoxQueryIT.java @@ -39,7 +39,7 @@ import static org.hamcrest.Matchers.anyOf; import static org.hamcrest.Matchers.equalTo; -public class GeoBoundingBoxIT extends ESIntegTestCase { +public class GeoBoundingBoxQueryIT extends ESIntegTestCase { @Override protected boolean forbidPrivateIndexSettings() { From af29e5e197266195b8d2c90f36b08d1426432196 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Mon, 9 Dec 2019 15:47:07 -0800 Subject: [PATCH 05/12] Adds support for geo-bounds filtering in geogrid aggregations It is fairly common to filter the geo point candidates in geohash_grid and geotile_grid aggregations according to some viewable bounding box. This change introduces the option of specifying this filter directly in the tiling aggregation. --- .../bucket/geohashgrid-aggregation.asciidoc | 58 +++++++++++ .../bucket/geotilegrid-aggregation.asciidoc | 58 +++++++++++ .../query-dsl/geo-bounding-box-query.asciidoc | 1 + .../common/geo/GeoBoundingBox.java | 25 +++++ .../elasticsearch/common/geo/GeoUtils.java | 1 + .../GeoTileGridValuesSourceBuilder.java | 27 ++++- .../bucket/geogrid/CellIdSource.java | 22 +++-- .../geogrid/GeoGridAggregationBuilder.java | 40 +++++++- .../bucket/geogrid/GeoGridAggregator.java | 4 +- .../GeoHashGridAggregationBuilder.java | 12 ++- .../bucket/geogrid/GeoHashGridAggregator.java | 8 +- .../geogrid/GeoHashGridAggregatorFactory.java | 19 ++-- .../GeoTileGridAggregationBuilder.java | 12 +-- .../bucket/geogrid/GeoTileGridAggregator.java | 8 +- .../geogrid/GeoTileGridAggregatorFactory.java | 19 ++-- .../bucket/geogrid/GeoTileUtils.java | 2 +- .../common/geo/GeoBoundingBoxTests.java | 31 +++++- .../aggregations/bucket/GeoHashGridTests.java | 4 + .../aggregations/bucket/GeoTileGridTests.java | 49 +++++++++ .../CompositeAggregationBuilderTests.java | 9 +- .../geogrid/GeoGridAggregatorTestCase.java | 99 ++++++++++++++++--- .../geogrid/GeoHashGridParserTests.java | 17 ++++ .../geogrid/GeoTileGridAggregatorTests.java | 1 - .../geogrid/GeoTileGridParserTests.java | 17 ++++ 24 files changed, 476 insertions(+), 67 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java diff --git a/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc b/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc index 59033843078b1..8d4652aebe2c5 100644 --- a/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc +++ b/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc @@ -191,6 +191,62 @@ var bbox = geohash.decode_bbox('u17'); -------------------------------------------------- // NOTCONSOLE +==== Requests with additional bounding box filtering + +The `geohash_grid` aggregation supports an optional `bounds` parameter +that restricts the points considered to those that fall within the +bounds provided. The `bounds` parameter accepts the bounding box in +all the same <> of the +bounds specified in the Geo Bounding Box Query. This bounding box can be used with or +without an additional `geo_bounding_box` query filtering the points prior to aggregating. +It is an independent bounding box that can intersect with, be equal to, or be disjoint +to any additional `geo_bounding_box` queries defined in the context of the aggregation. + +[source,console,id=geohashgrid-aggregation-with-bounds] +-------------------------------------------------- +POST /museums/_search?size=0 +{ + "aggregations" : { + "tiles-in-bounds" : { + "geohash_grid" : { + "field" : "location", + "precision" : 8, + "bounds": { + "top_left" : "53.4375, 4.21875", + "bottom_right" : "52.03125, 5.625" + } + } + } + } +} +-------------------------------------------------- +// TEST[continued] + +[source,console-result] +-------------------------------------------------- +{ + ... + "aggregations" : { + "tiles-in-bounds" : { + "buckets" : [ + { + "key" : "u173zy3j", + "doc_count" : 1 + }, + { + "key" : "u173zvfz", + "doc_count" : 1 + }, + { + "key" : "u173zt90", + "doc_count" : 1 + } + ] + } + } +} +-------------------------------------------------- +// TESTRESPONSE[s/\.\.\./"took": $body.took,"_shards": $body._shards,"hits":$body.hits,"timed_out":false,/] ==== Cell dimensions at the equator The table below shows the metric dimensions for cells covered by various string lengths of geohash. @@ -230,6 +286,8 @@ precision:: Optional. The string length of the geohashes used to define to precision levels higher than the supported 12 levels, (e.g. for distances <5.6cm) the value is rejected. +bounds: Optional. The bounding box to filter the points in the bucket. + size:: Optional. The maximum number of geohash buckets to return (defaults to 10,000). When results are trimmed, buckets are prioritised based on the volumes of documents they contain. diff --git a/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc b/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc index c60b14b0f2687..25cc1a977c86b 100644 --- a/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc +++ b/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc @@ -162,6 +162,62 @@ POST /museums/_search?size=0 -------------------------------------------------- // TESTRESPONSE[s/\.\.\./"took": $body.took,"_shards": $body._shards,"hits":$body.hits,"timed_out":false,/] +==== Requests with additional bounding box filtering + +The `geotile_grid` aggregation supports an optional `bounds` parameter +that restricts the points considered to those that fall within the +bounds provided. The `bounds` parameter accepts the bounding box in +all the same <> of the +bounds specified in the Geo Bounding Box Query. This bounding box can be used with or +without an additional `geo_bounding_box` query filtering the points prior to aggregating. +It is an independent bounding box that can intersect with, be equal to, or be disjoint +to any additional `geo_bounding_box` queries defined in the context of the aggregation. + +[source,console,id=geotilegrid-aggregation-with-bounds] +-------------------------------------------------- +POST /museums/_search?size=0 +{ + "aggregations" : { + "tiles-in-bounds" : { + "geotile_grid" : { + "field" : "location", + "precision" : 22, + "bounds": { + "top_left" : "52.4, 4.9", + "bottom_right" : "52.3, 5.0" + } + } + } + } +} +-------------------------------------------------- +// TEST[continued] + +[source,console-result] +-------------------------------------------------- +{ + ... + "aggregations" : { + "tiles-in-bounds" : { + "buckets" : [ + { + "key" : "22/2154412/1378379", + "doc_count" : 1 + }, + { + "key" : "22/2154385/1378332", + "doc_count" : 1 + }, + { + "key" : "22/2154259/1378425", + "doc_count" : 1 + } + ] + } + } +} +-------------------------------------------------- +// TESTRESPONSE[s/\.\.\./"took": $body.took,"_shards": $body._shards,"hits":$body.hits,"timed_out":false,/] ==== Options @@ -172,6 +228,8 @@ precision:: Optional. The integer zoom of the key used to define cells/buckets in the results. Defaults to 7. Values outside of [0,29] will be rejected. +bounds: Optional. The bounding box to filter the points in the bucket. + size:: Optional. The maximum number of geohash buckets to return (defaults to 10,000). When results are trimmed, buckets are prioritised based on the volumes of documents they contain. diff --git a/docs/reference/query-dsl/geo-bounding-box-query.asciidoc b/docs/reference/query-dsl/geo-bounding-box-query.asciidoc index a51283ceeb341..27f37423fa28f 100644 --- a/docs/reference/query-dsl/geo-bounding-box-query.asciidoc +++ b/docs/reference/query-dsl/geo-bounding-box-query.asciidoc @@ -84,6 +84,7 @@ be executed in memory or indexed. See <> below for further d Default is `memory`. |======================================================================= +[[query-dsl-geo-bounding-box-query-accepted-formats]] [float] ==== Accepted Formats diff --git a/server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java b/server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java index 78e81925275d7..01315d3e9848c 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoBoundingBox.java @@ -68,6 +68,11 @@ public GeoBoundingBox(StreamInput input) throws IOException { this.bottomRight = input.readGeoPoint(); } + public boolean isUnbounded() { + return Double.isNaN(topLeft.lon()) || Double.isNaN(topLeft.lat()) + || Double.isNaN(bottomRight.lon()) || Double.isNaN(bottomRight.lat()); + } + public GeoPoint topLeft() { return topLeft; } @@ -120,6 +125,26 @@ public XContentBuilder toXContentFragment(XContentBuilder builder, boolean build return builder; } + /** + * If the bounding box crosses the date-line (left greater-than right) then the + * longitude of the point need only to be higher than the left or lower + * than the right. Otherwise, it must be both. + * + * @param lon the longitude of the point + * @param lat the latitude of the point + * @return whether the point (lon, lat) is in the specified bounding box + */ + public boolean pointInBounds(double lon, double lat) { + if (lat >= bottom() && lat <= top()) { + if (left() <= right()) { + return lon >= left() && lon <= right(); + } else { + return lon >= left() || lon <= right(); + } + } + return false; + } + @Override public void writeTo(StreamOutput out) throws IOException { out.writeGeoPoint(topLeft); 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 359b7781b895f..d33f90043b1d5 100644 --- a/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java +++ b/server/src/main/java/org/elasticsearch/common/geo/GeoUtils.java @@ -93,6 +93,7 @@ public static boolean isValidLongitude(double longitude) { return true; } + /** * Calculate the width (in meters) of geohash cells at a specific level * @param level geohash level must be greater or equal to zero diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java index b6f2b2788cd25..c8aef28f871ef 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java @@ -19,7 +19,10 @@ package org.elasticsearch.search.aggregations.bucket.composite; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -45,6 +48,8 @@ public class GeoTileGridValuesSourceBuilder extends CompositeValuesSourceBuilder static { PARSER = new ObjectParser<>(GeoTileGridValuesSourceBuilder.TYPE); PARSER.declareInt(GeoTileGridValuesSourceBuilder::precision, new ParseField("precision")); + PARSER.declareField(((p, builder, context) -> builder.geoBoundingBox(GeoBoundingBox.parseBoundingBox(p))), + GeoBoundingBox.BOUNDS_FIELD, ObjectParser.ValueType.OBJECT); CompositeValuesSourceParserHelper.declareValuesSourceFields(PARSER, ValueType.NUMERIC); } @@ -53,6 +58,7 @@ static GeoTileGridValuesSourceBuilder parse(String name, XContentParser parser) } private int precision = GeoTileGridAggregationBuilder.DEFAULT_PRECISION; + private GeoBoundingBox geoBoundingBox = new GeoBoundingBox(new GeoPoint(Double.NaN, Double.NaN), new GeoPoint(Double.NaN, Double.NaN)); GeoTileGridValuesSourceBuilder(String name) { super(name); @@ -61,6 +67,9 @@ static GeoTileGridValuesSourceBuilder parse(String name, XContentParser parser) GeoTileGridValuesSourceBuilder(StreamInput in) throws IOException { super(in); this.precision = in.readInt(); + if (in.getVersion().onOrAfter(Version.V_8_0_0)) { + this.geoBoundingBox = new GeoBoundingBox(in); + } } public GeoTileGridValuesSourceBuilder precision(int precision) { @@ -68,6 +77,11 @@ public GeoTileGridValuesSourceBuilder precision(int precision) { return this; } + public GeoTileGridValuesSourceBuilder geoBoundingBox(GeoBoundingBox geoBoundingBox) { + this.geoBoundingBox = geoBoundingBox; + return this; + } + @Override public GeoTileGridValuesSourceBuilder format(String format) { throw new IllegalArgumentException("[format] is not supported for [" + TYPE + "]"); @@ -76,11 +90,17 @@ public GeoTileGridValuesSourceBuilder format(String format) { @Override protected void innerWriteTo(StreamOutput out) throws IOException { out.writeInt(precision); + if (out.getVersion().onOrAfter(Version.V_8_0_0)) { + geoBoundingBox.writeTo(out); + } } @Override protected void doXContentBody(XContentBuilder builder, Params params) throws IOException { builder.field("precision", precision); + if (geoBoundingBox.isUnbounded() == false) { + geoBoundingBox.toXContent(builder, params); + } } @Override @@ -90,7 +110,7 @@ String type() { @Override public int hashCode() { - return Objects.hash(super.hashCode(), precision); + return Objects.hash(super.hashCode(), precision, geoBoundingBox); } @Override @@ -99,7 +119,8 @@ public boolean equals(Object obj) { if (obj == null || getClass() != obj.getClass()) return false; if (super.equals(obj) == false) return false; GeoTileGridValuesSourceBuilder other = (GeoTileGridValuesSourceBuilder) obj; - return precision == other.precision; + return Objects.equals(precision,other.precision) + && Objects.equals(geoBoundingBox, other.geoBoundingBox); } @Override @@ -112,7 +133,7 @@ protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardCon ValuesSource.GeoPoint geoPoint = (ValuesSource.GeoPoint) orig; // is specified in the builder. final MappedFieldType fieldType = config.fieldContext() != null ? config.fieldContext().fieldType() : null; - CellIdSource cellIdSource = new CellIdSource(geoPoint, precision, GeoTileUtils::longEncode); + CellIdSource cellIdSource = new CellIdSource(geoPoint, precision, geoBoundingBox, GeoTileUtils::longEncode); return new CompositeValuesSourceConfig(name, fieldType, cellIdSource, DocValueFormat.GEOTILE, order(), missingBucket(), script() != null); } else { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java index 4ebb689c7c44f..3ef91e2d61120 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java @@ -20,6 +20,7 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.SortedNumericDocValues; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues; import org.elasticsearch.index.fielddata.MultiGeoPointValues; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; @@ -36,11 +37,13 @@ public class CellIdSource extends ValuesSource.Numeric { private final ValuesSource.GeoPoint valuesSource; private final int precision; private final GeoPointLongEncoder encoder; + private final GeoBoundingBox geoBoundingBox; - public CellIdSource(GeoPoint valuesSource, int precision, GeoPointLongEncoder encoder) { + public CellIdSource(GeoPoint valuesSource,int precision, GeoBoundingBox geoBoundingBox, GeoPointLongEncoder encoder) { this.valuesSource = valuesSource; //different GeoPoints could map to the same or different hashing cells. this.precision = precision; + this.geoBoundingBox = geoBoundingBox; this.encoder = encoder; } @@ -55,7 +58,7 @@ public boolean isFloatingPoint() { @Override public SortedNumericDocValues longValues(LeafReaderContext ctx) { - return new CellValues(valuesSource.geoPointValues(ctx), precision, encoder); + return new CellValues(valuesSource.geoPointValues(ctx), precision, geoBoundingBox, encoder); } @Override @@ -81,21 +84,28 @@ private static class CellValues extends AbstractSortingNumericDocValues { private MultiGeoPointValues geoValues; private int precision; private GeoPointLongEncoder encoder; + private GeoBoundingBox geoBoundingBox; - protected CellValues(MultiGeoPointValues geoValues, int precision, GeoPointLongEncoder encoder) { + protected CellValues(MultiGeoPointValues geoValues, int precision, GeoBoundingBox geoBoundingBox, GeoPointLongEncoder encoder) { this.geoValues = geoValues; this.precision = precision; this.encoder = encoder; + this.geoBoundingBox = geoBoundingBox; } @Override public boolean advanceExact(int docId) throws IOException { if (geoValues.advanceExact(docId)) { - resize(geoValues.docValueCount()); - for (int i = 0; i < docValueCount(); ++i) { + int docValueCount = geoValues.docValueCount(); + resize(docValueCount); + int j = 0; + for (int i = 0; i < docValueCount; i++) { org.elasticsearch.common.geo.GeoPoint target = geoValues.nextValue(); - values[i] = encoder.encode(target.getLon(), target.getLat(), precision); + if (geoBoundingBox.isUnbounded() || geoBoundingBox.pointInBounds(target.getLon(), target.getLat())) { + values[j++] = encoder.encode(target.getLon(), target.getLat(), precision); + } } + resize(j); sort(); return true; } else { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java index e1c4221139202..233fd4c6885e8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregationBuilder.java @@ -20,7 +20,10 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.Version; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -53,6 +56,8 @@ public abstract class GeoGridAggregationBuilder extends ValuesSourceAggregationB protected int precision; protected int requiredSize; protected int shardSize; + private GeoBoundingBox geoBoundingBox = new GeoBoundingBox(new GeoPoint(Double.NaN, Double.NaN), new GeoPoint(Double.NaN, Double.NaN)); + @FunctionalInterface protected interface PrecisionParser { @@ -66,6 +71,10 @@ public static ObjectParser createParser(String org.elasticsearch.common.xcontent.ObjectParser.ValueType.INT); parser.declareInt(GeoGridAggregationBuilder::size, FIELD_SIZE); parser.declareInt(GeoGridAggregationBuilder::shardSize, FIELD_SHARD_SIZE); + parser.declareField((p, builder, context) -> { + builder.setGeoBoundingBox(GeoBoundingBox.parseBoundingBox(p)); + }, + GeoBoundingBox.BOUNDS_FIELD, org.elasticsearch.common.xcontent.ObjectParser.ValueType.OBJECT); return parser; } @@ -78,7 +87,7 @@ protected GeoGridAggregationBuilder(GeoGridAggregationBuilder clone, Builder fac this.precision = clone.precision; this.requiredSize = clone.requiredSize; this.shardSize = clone.shardSize; - + this.geoBoundingBox = clone.geoBoundingBox; } /** @@ -89,6 +98,9 @@ public GeoGridAggregationBuilder(StreamInput in) throws IOException { precision = in.readVInt(); requiredSize = in.readVInt(); shardSize = in.readVInt(); + if (in.getVersion().onOrAfter(Version.V_8_0_0)) { + geoBoundingBox = new GeoBoundingBox(in); + } } @Override @@ -96,6 +108,9 @@ protected void innerWriteTo(StreamOutput out) throws IOException { out.writeVInt(precision); out.writeVInt(requiredSize); out.writeVInt(shardSize); + if (out.getVersion().onOrAfter(Version.V_8_0_0)) { + geoBoundingBox.writeTo(out); + } } /** @@ -110,7 +125,8 @@ protected void innerWriteTo(StreamOutput out) throws IOException { */ protected abstract ValuesSourceAggregatorFactory createFactory( String name, ValuesSourceConfig config, int precision, int requiredSize, int shardSize, - QueryShardContext queryShardContext, AggregatorFactory parent, Builder subFactoriesBuilder, Map metaData + GeoBoundingBox geoBoundingBox, QueryShardContext queryShardContext, AggregatorFactory parent, + Builder subFactoriesBuilder, Map metaData ) throws IOException; public int precision() { @@ -143,6 +159,16 @@ public int shardSize() { return shardSize; } + public GeoGridAggregationBuilder setGeoBoundingBox(GeoBoundingBox geoBoundingBox) { + this.geoBoundingBox = geoBoundingBox; + // no validation done here, similar to geo_bounding_box query behavior. + return this; + } + + public GeoBoundingBox geoBoundingBox() { + return geoBoundingBox; + } + @Override protected ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config, @@ -166,7 +192,7 @@ protected ValuesSourceAggregatorFactory innerBuild(QueryS if (shardSize < requiredSize) { shardSize = requiredSize; } - return createFactory(name, config, precision, requiredSize, shardSize, queryShardContext, parent, + return createFactory(name, config, precision, requiredSize, shardSize, geoBoundingBox, queryShardContext, parent, subFactoriesBuilder, metaData); } @@ -177,6 +203,9 @@ protected XContentBuilder doXContentBody(XContentBuilder builder, Params params) if (shardSize > -1) { builder.field(FIELD_SHARD_SIZE.getPreferredName(), shardSize); } + if (geoBoundingBox.isUnbounded() == false) { + geoBoundingBox.toXContent(builder, params); + } return builder; } @@ -188,11 +217,12 @@ public boolean equals(Object obj) { GeoGridAggregationBuilder other = (GeoGridAggregationBuilder) obj; return precision == other.precision && requiredSize == other.requiredSize - && shardSize == other.shardSize; + && shardSize == other.shardSize + && Objects.equals(geoBoundingBox, other.geoBoundingBox); } @Override public int hashCode() { - return Objects.hash(super.hashCode(), precision, requiredSize, shardSize); + return Objects.hash(super.hashCode(), precision, requiredSize, shardSize, geoBoundingBox); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregator.java index c91f763b603c0..c77578f2570c7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregator.java @@ -48,8 +48,8 @@ public abstract class GeoGridAggregator extends Bucke protected final LongHash bucketOrds; GeoGridAggregator(String name, AggregatorFactories factories, CellIdSource valuesSource, - int requiredSize, int shardSize, SearchContext aggregationContext, Aggregator parent, - List pipelineAggregators, Map metaData) throws IOException { + int requiredSize, int shardSize, SearchContext aggregationContext, + Aggregator parent, List pipelineAggregators, Map metaData) throws IOException { super(name, factories, aggregationContext, parent, pipelineAggregators, metaData); this.valuesSource = valuesSource; this.requiredSize = requiredSize; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java index d58beeb781c25..acc9cde113164 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ObjectParser; @@ -60,11 +61,12 @@ public GeoGridAggregationBuilder precision(int precision) { @Override protected ValuesSourceAggregatorFactory createFactory( - String name, ValuesSourceConfig config, int precision, int requiredSize, int shardSize, - QueryShardContext queryShardContext, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, - Map metaData) throws IOException { - return new GeoHashGridAggregatorFactory(name, config, precision, requiredSize, shardSize, queryShardContext, parent, - subFactoriesBuilder, metaData); + String name, ValuesSourceConfig config, int precision, int requiredSize, int shardSize, + GeoBoundingBox geoBoundingBox, QueryShardContext queryShardContext, + AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { + return new GeoHashGridAggregatorFactory(name, config, precision, requiredSize, shardSize, geoBoundingBox, + queryShardContext, parent, subFactoriesBuilder, metaData); } private GeoHashGridAggregationBuilder(GeoHashGridAggregationBuilder clone, AggregatorFactories.Builder factoriesBuilder, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java index 54d1e2e940649..1ad59ccc45100 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregator.java @@ -34,9 +34,11 @@ public class GeoHashGridAggregator extends GeoGridAggregator { GeoHashGridAggregator(String name, AggregatorFactories factories, CellIdSource valuesSource, - int requiredSize, int shardSize, SearchContext aggregationContext, Aggregator parent, - List pipelineAggregators, Map metaData) throws IOException { - super(name, factories, valuesSource, requiredSize, shardSize, aggregationContext, parent, pipelineAggregators, metaData); + int requiredSize, int shardSize, SearchContext aggregationContext, + Aggregator parent, List pipelineAggregators, + Map metaData) throws IOException { + super(name, factories, valuesSource, requiredSize, shardSize, aggregationContext, parent, + pipelineAggregators, metaData); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java index a049a07f13dbe..2d7087a693bfa 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.geometry.utils.Geohash; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.aggregations.Aggregator; @@ -28,7 +29,6 @@ import org.elasticsearch.search.aggregations.NonCollectingAggregator; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSource.GeoPoint; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.internal.SearchContext; @@ -43,14 +43,17 @@ public class GeoHashGridAggregatorFactory extends ValuesSourceAggregatorFactory< private final int precision; private final int requiredSize; private final int shardSize; + private final GeoBoundingBox geoBoundingBox; - GeoHashGridAggregatorFactory(String name, ValuesSourceConfig config, int precision, int requiredSize, - int shardSize, QueryShardContext queryShardContext, AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { + GeoHashGridAggregatorFactory(String name, ValuesSourceConfig config, int precision, int requiredSize, + int shardSize, GeoBoundingBox geoBoundingBox, QueryShardContext queryShardContext, + AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData); this.precision = precision; this.requiredSize = requiredSize; this.shardSize = shardSize; + this.geoBoundingBox = geoBoundingBox; } @Override @@ -69,7 +72,7 @@ public InternalAggregation buildEmptyAggregation() { } @Override - protected Aggregator doCreateInternal(final GeoPoint valuesSource, + protected Aggregator doCreateInternal(final ValuesSource.GeoPoint valuesSource, SearchContext searchContext, Aggregator parent, boolean collectsFromSingleBucket, @@ -78,8 +81,8 @@ protected Aggregator doCreateInternal(final GeoPoint valuesSource, if (collectsFromSingleBucket == false) { return asMultiBucketAggregator(this, searchContext, parent); } - CellIdSource cellIdSource = new CellIdSource(valuesSource, precision, Geohash::longEncode); - return new GeoHashGridAggregator(name, factories, cellIdSource, requiredSize, shardSize, searchContext, parent, - pipelineAggregators, metaData); + CellIdSource cellIdSource = new CellIdSource(valuesSource, precision, geoBoundingBox, Geohash::longEncode); + return new GeoHashGridAggregator(name, factories, cellIdSource, requiredSize, shardSize, + searchContext, parent, pipelineAggregators, metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java index b3d9888781362..595c6cab6e718 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentParser; @@ -59,12 +60,11 @@ public GeoGridAggregationBuilder precision(int precision) { @Override protected ValuesSourceAggregatorFactory createFactory( - String name, ValuesSourceConfig config, int precision, int requiredSize, int shardSize, - QueryShardContext queryShardContext, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, - Map metaData - ) throws IOException { - return new GeoTileGridAggregatorFactory(name, config, precision, requiredSize, shardSize, queryShardContext, parent, - subFactoriesBuilder, metaData); + String name, ValuesSourceConfig config, int precision, int requiredSize, int shardSize, + GeoBoundingBox geoBoundingBox, QueryShardContext queryShardContext, AggregatorFactory parent, + AggregatorFactories.Builder subFactoriesBuilder, Map metaData ) throws IOException { + return new GeoTileGridAggregatorFactory(name, config, precision, requiredSize, shardSize, geoBoundingBox, + queryShardContext, parent, subFactoriesBuilder, metaData); } private GeoTileGridAggregationBuilder(GeoTileGridAggregationBuilder clone, AggregatorFactories.Builder factoriesBuilder, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregator.java index d2ff5ed82513c..350761aa84050 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregator.java @@ -35,9 +35,11 @@ public class GeoTileGridAggregator extends GeoGridAggregator { GeoTileGridAggregator(String name, AggregatorFactories factories, CellIdSource valuesSource, - int requiredSize, int shardSize, SearchContext aggregationContext, Aggregator parent, - List pipelineAggregators, Map metaData) throws IOException { - super(name, factories, valuesSource, requiredSize, shardSize, aggregationContext, parent, pipelineAggregators, metaData); + int requiredSize, int shardSize, SearchContext aggregationContext, + Aggregator parent, List pipelineAggregators, + Map metaData) throws IOException { + super(name, factories, valuesSource, requiredSize, shardSize, aggregationContext, parent, + pipelineAggregators, metaData); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java index 8380a4172c9c5..0f59c9a71ea40 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorFactories; @@ -27,7 +28,6 @@ import org.elasticsearch.search.aggregations.NonCollectingAggregator; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; import org.elasticsearch.search.aggregations.support.ValuesSource; -import org.elasticsearch.search.aggregations.support.ValuesSource.GeoPoint; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.internal.SearchContext; @@ -42,14 +42,17 @@ public class GeoTileGridAggregatorFactory extends ValuesSourceAggregatorFactory< private final int precision; private final int requiredSize; private final int shardSize; + private final GeoBoundingBox geoBoundingBox; - GeoTileGridAggregatorFactory(String name, ValuesSourceConfig config, int precision, int requiredSize, - int shardSize, QueryShardContext queryShardContext, AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { + GeoTileGridAggregatorFactory(String name, ValuesSourceConfig config, int precision, int requiredSize, + int shardSize, GeoBoundingBox geoBoundingBox, QueryShardContext queryShardContext, + AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData); this.precision = precision; this.requiredSize = requiredSize; this.shardSize = shardSize; + this.geoBoundingBox = geoBoundingBox; } @Override @@ -68,7 +71,7 @@ public InternalAggregation buildEmptyAggregation() { } @Override - protected Aggregator doCreateInternal(final GeoPoint valuesSource, + protected Aggregator doCreateInternal(final ValuesSource.GeoPoint valuesSource, SearchContext searchContext, Aggregator parent, boolean collectsFromSingleBucket, @@ -77,8 +80,8 @@ protected Aggregator doCreateInternal(final GeoPoint valuesSource, if (collectsFromSingleBucket == false) { return asMultiBucketAggregator(this, searchContext, parent); } - CellIdSource cellIdSource = new CellIdSource(valuesSource, precision, GeoTileUtils::longEncode); - return new GeoTileGridAggregator(name, factories, cellIdSource, requiredSize, shardSize, searchContext, parent, - pipelineAggregators, metaData); + CellIdSource cellIdSource = new CellIdSource(valuesSource, precision, geoBoundingBox, GeoTileUtils::longEncode); + return new GeoTileGridAggregator(name, factories, cellIdSource, requiredSize, shardSize, + searchContext, parent, pipelineAggregators, metaData); } } 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 19381e87fedb6..03f821296f2a6 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 @@ -51,7 +51,7 @@ private GeoTileUtils() {} * Another consideration is that index optimizes lat/lng storage, loosing some precision. * E.g. hash lng=140.74779717298918D lat=45.61884022447444D == "18/233561/93659", but shown as "18/233561/93658" */ - static final int MAX_ZOOM = 29; + public static final int MAX_ZOOM = 29; /** * Bit position of the zoom value within hash - zoom is stored in the most significant 6 bits of a long number. diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java index da7758a720022..7105ef6d1e060 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java @@ -24,6 +24,7 @@ import org.elasticsearch.common.xcontent.XContentFactory; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.test.ESTestCase; import java.io.IOException; @@ -133,6 +134,34 @@ public void testNullTopBottomLeftRight() throws Exception { } } + + public void testPointInBounds() { + for (int iter = 0; iter < 100; iter++) { + Rectangle rectangle = GeometryTestUtils.randomRectangle(); + GeoBoundingBox geoBoundingBox = new GeoBoundingBox(new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), + new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())); + if (rectangle.getMinX() > rectangle.getMaxX()) { + double lonWithin = randomBoolean() ? + randomDoubleBetween(rectangle.getMinX(), 180.0, true) + : randomDoubleBetween(-180.0, rectangle.getMaxX(), true); + double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); + double lonOutside = (rectangle.getMinX() + rectangle.getMaxX()) / 2; + double latOutside = rectangle.getMinX() - randomIntBetween(1, 10); + + assertTrue(geoBoundingBox.pointInBounds(lonWithin, latWithin)); + assertFalse(geoBoundingBox.pointInBounds(lonOutside, latOutside)); + } else { + double lonWithin = randomDoubleBetween(rectangle.getMinX(), rectangle.getMaxX(), true); + double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); + double lonOutside = randomDoubleBetween(rectangle.getMaxX(), 180.0, false); + double latOutside = randomDoubleBetween(rectangle.getMaxY(), 90.0, false); + + assertTrue(geoBoundingBox.pointInBounds(lonWithin, latWithin)); + assertFalse(geoBoundingBox.pointInBounds(lonOutside, latOutside)); + } + } + } + private void assertBBox(GeoBoundingBox expected, XContentBuilder builder) throws IOException { try (XContentParser parser = createParser(builder)) { parser.nextToken(); @@ -140,7 +169,7 @@ private void assertBBox(GeoBoundingBox expected, XContentBuilder builder) throws } } - private GeoBoundingBox randomBBox() { + public static GeoBoundingBox randomBBox() { double topLat = GeometryTestUtils.randomLat(); double bottomLat = randomDoubleBetween(GeoUtils.MIN_LAT, topLat, true); return new GeoBoundingBox(new GeoPoint(topLat, GeometryTestUtils.randomLon()), diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index e414b86403a09..629ccfbce08f1 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.search.aggregations.bucket; +import org.elasticsearch.common.geo.GeoBoundingBoxTests; import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashGridAggregationBuilder; @@ -39,6 +40,9 @@ protected GeoHashGridAggregationBuilder createTestAggregatorBuilder() { if (randomBoolean()) { factory.shardSize(randomIntBetween(1, Integer.MAX_VALUE)); } + if (randomBoolean()) { + factory.setGeoBoundingBox(GeoBoundingBoxTests.randomBBox()); + } return factory; } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java new file mode 100644 index 0000000000000..825c201139442 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java @@ -0,0 +1,49 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.search.aggregations.bucket; + +import org.elasticsearch.common.geo.GeoBoundingBoxTests; +import org.elasticsearch.search.aggregations.BaseAggregationTestCase; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils; + +public class GeoTileGridTests extends BaseAggregationTestCase { + + @Override + protected GeoTileGridAggregationBuilder createTestAggregatorBuilder() { + String name = randomAlphaOfLengthBetween(3, 20); + GeoTileGridAggregationBuilder factory = new GeoTileGridAggregationBuilder(name); + if (randomBoolean()) { + factory.precision(randomIntBetween(0, GeoTileUtils.MAX_ZOOM)); + } + if (randomBoolean()) { + factory.size(randomIntBetween(1, Integer.MAX_VALUE)); + } + if (randomBoolean()) { + factory.shardSize(randomIntBetween(1, Integer.MAX_VALUE)); + } + if (randomBoolean()) { + factory.setGeoBoundingBox(GeoBoundingBoxTests.randomBBox()); + } + return factory; + } + +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java index c78a55b253297..c36ff50b7a36d 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregationBuilderTests.java @@ -19,8 +19,10 @@ package org.elasticsearch.search.aggregations.bucket.composite; +import org.elasticsearch.common.geo.GeoBoundingBoxTests; import org.elasticsearch.script.Script; import org.elasticsearch.search.aggregations.BaseAggregationTestCase; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils; import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval; import org.elasticsearch.search.sort.SortOrder; @@ -54,7 +56,10 @@ private DateHistogramValuesSourceBuilder randomDateHistogramSourceBuilder() { private GeoTileGridValuesSourceBuilder randomGeoTileGridValuesSourceBuilder() { GeoTileGridValuesSourceBuilder geoTile = new GeoTileGridValuesSourceBuilder(randomAlphaOfLengthBetween(5, 10)); if (randomBoolean()) { - geoTile.precision(randomIntBetween(1, 12)); + geoTile.precision(randomIntBetween(0, GeoTileUtils.MAX_ZOOM)); + } + if (randomBoolean()) { + geoTile.geoBoundingBox(GeoBoundingBoxTests.randomBBox()); } return geoTile; } @@ -90,9 +95,11 @@ private HistogramValuesSourceBuilder randomHistogramSourceBuilder() { @Override protected CompositeAggregationBuilder createTestAggregatorBuilder() { int numSources = randomIntBetween(1, 10); + numSources = 1; List> sources = new ArrayList<>(); for (int i = 0; i < numSources; i++) { int type = randomIntBetween(0, 3); + type = 3; switch (type) { case 0: sources.add(randomTermsSourceBuilder()); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java index 047903bc86100..ebd7a2b8ddc2a 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java @@ -28,6 +28,10 @@ import org.apache.lucene.search.Query; import org.apache.lucene.store.Directory; import org.elasticsearch.common.CheckedConsumer; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.index.mapper.GeoPointFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.search.aggregations.Aggregator; @@ -44,6 +48,8 @@ import java.util.Set; import java.util.function.Consumer; +import static org.hamcrest.Matchers.equalTo; + public abstract class GeoGridAggregatorTestCase extends AggregatorTestCase { private static final String FIELD_NAME = "location"; @@ -64,18 +70,18 @@ public abstract class GeoGridAggregatorTestCase protected abstract GeoGridAggregationBuilder createBuilder(String name); public void testNoDocs() throws IOException { - testCase(new MatchAllDocsQuery(), FIELD_NAME, randomPrecision(), iw -> { - // Intentionally not writing any docs - }, geoGrid -> { + testCase(new MatchAllDocsQuery(), FIELD_NAME, randomPrecision(), null, geoGrid -> { assertEquals(0, geoGrid.getBuckets().size()); + }, iw -> { + // Intentionally not writing any docs }); } public void testFieldMissing() throws IOException { - testCase(new MatchAllDocsQuery(), "wrong_field", randomPrecision(), iw -> { - iw.addDocument(Collections.singleton(new LatLonDocValuesField(FIELD_NAME, 10D, 10D))); - }, geoGrid -> { + testCase(new MatchAllDocsQuery(), "wrong_field", randomPrecision(), null, geoGrid -> { assertEquals(0, geoGrid.getBuckets().size()); + }, iw -> { + iw.addDocument(Collections.singleton(new LatLonDocValuesField(FIELD_NAME, 10D, 10D))); }); } @@ -83,7 +89,13 @@ public void testWithSeveralDocs() throws IOException { int precision = randomPrecision(); int numPoints = randomIntBetween(8, 128); Map expectedCountPerGeoHash = new HashMap<>(); - testCase(new MatchAllDocsQuery(), FIELD_NAME, precision, iw -> { + testCase(new MatchAllDocsQuery(), FIELD_NAME, precision, null, geoHashGrid -> { + assertEquals(expectedCountPerGeoHash.size(), geoHashGrid.getBuckets().size()); + for (GeoGrid.Bucket bucket : geoHashGrid.getBuckets()) { + assertEquals((long) expectedCountPerGeoHash.get(bucket.getKeyAsString()), bucket.getDocCount()); + } + assertTrue(AggregationInspectionHelper.hasValue(geoHashGrid)); + }, iw -> { List points = new ArrayList<>(); Set distinctHashesPerDoc = new HashSet<>(); for (int pointId = 0; pointId < numPoints; pointId++) { @@ -112,17 +124,69 @@ public void testWithSeveralDocs() throws IOException { if (points.size() != 0) { iw.addDocument(points); } - }, geoHashGrid -> { - assertEquals(expectedCountPerGeoHash.size(), geoHashGrid.getBuckets().size()); - for (GeoGrid.Bucket bucket : geoHashGrid.getBuckets()) { - assertEquals((long) expectedCountPerGeoHash.get(bucket.getKeyAsString()), bucket.getDocCount()); + }); + } + + public void testBounds() throws IOException { + final int numDocs = randomIntBetween(64, 256); + final GeoGridAggregationBuilder builder = createBuilder("_name"); + + expectThrows(IllegalArgumentException.class, () -> builder.precision(-1)); + expectThrows(IllegalArgumentException.class, () -> builder.precision(30)); + + Rectangle rectangle = GeometryTestUtils.randomRectangle(); + + int in = 0, out = 0; + List docs = new ArrayList<>(); + while (in + out < numDocs) { + if (rectangle.getMinX() > rectangle.getMaxX()) { + if (randomBoolean()) { + double lonWithin = randomBoolean() ? + randomDoubleBetween(rectangle.getMinX(), 180.0, true) + : randomDoubleBetween(-180.0, rectangle.getMaxX(), true); + double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); + in++; + docs.add(new LatLonDocValuesField(FIELD_NAME, latWithin, lonWithin)); + } else { + double lonOutside = (rectangle.getMinX() + rectangle.getMaxX()) / 2; + double latOutside = rectangle.getMinX() - randomIntBetween(1, 10); + out++; + docs.add(new LatLonDocValuesField(FIELD_NAME, latOutside, lonOutside)); + } + + } else { + if (randomBoolean()) { + double lonWithin = randomDoubleBetween(rectangle.getMinX(), rectangle.getMaxX(), true); + double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); + in++; + docs.add(new LatLonDocValuesField(FIELD_NAME, latWithin, lonWithin)); + } else { + double lonOutside = randomDoubleBetween(rectangle.getMaxX(), 180.0, false); + double latOutside = randomDoubleBetween(rectangle.getMaxY(), 90.0, false); + out++; + docs.add(new LatLonDocValuesField(FIELD_NAME, latOutside, lonOutside)); + } + } + + } + + final long numDocsInBucket = in; + + testCase(new MatchAllDocsQuery(), FIELD_NAME, 0, rectangle, geoGrid -> { + assertThat(geoGrid.getBuckets().size(), equalTo(1)); + long docCount = geoGrid.getBuckets().get(0).getDocCount(); + assertThat(docCount, equalTo(numDocsInBucket)); + assertTrue(AggregationInspectionHelper.hasValue(geoGrid)); + }, iw -> { + for (LatLonDocValuesField docField : docs) { + iw.addDocument(Collections.singletonList(docField)); } - assertTrue(AggregationInspectionHelper.hasValue(geoHashGrid)); }); } - private void testCase(Query query, String field, int precision, CheckedConsumer buildIndex, - Consumer> verify) throws IOException { + private void testCase(Query query, String field, int precision, Rectangle boundingBox, + Consumer> verify, + CheckedConsumer buildIndex) throws IOException { Directory directory = newDirectory(); RandomIndexWriter indexWriter = new RandomIndexWriter(random(), directory); buildIndex.accept(indexWriter); @@ -133,6 +197,13 @@ private void testCase(Query query, String field, int precision, CheckedConsumer< GeoGridAggregationBuilder aggregationBuilder = createBuilder("_name").field(field); aggregationBuilder.precision(precision); + if (boundingBox != null) { + GeoBoundingBox geoBoundingBox = new GeoBoundingBox(new GeoPoint(boundingBox.getMaxLat(), boundingBox.getMinLon()), + new GeoPoint(boundingBox.getMinLat(), boundingBox.getMaxLon())); + aggregationBuilder.setGeoBoundingBox(geoBoundingBox); + assertThat(aggregationBuilder.geoBoundingBox(), equalTo(geoBoundingBox)); + } + MappedFieldType fieldType = new GeoPointFieldMapper.GeoPointFieldType(); fieldType.setHasDocValues(true); fieldType.setName(FIELD_NAME); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java index 824a069203856..5075ed9f02aa2 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridParserTests.java @@ -22,6 +22,8 @@ import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.test.ESTestCase; import static org.hamcrest.Matchers.containsString; @@ -113,4 +115,19 @@ public void testParseErrorOnPrecisionOutOfRange() throws Exception { assertEquals("Invalid geohash aggregation precision of 13. Must be between 1 and 12.", ex.getCause().getMessage()); } } + + public void testParseValidBounds() throws Exception { + Rectangle bbox = GeometryTestUtils.randomRectangle(); + XContentParser stParser = createParser(JsonXContent.jsonXContent, + "{\"field\":\"my_loc\", \"precision\": 5, \"size\": 500, \"shard_size\": 550," + "\"bounds\": { " + + "\"top\": " + bbox.getMaxY() + "," + + "\"bottom\": " + bbox.getMinY() + "," + + "\"left\": " + bbox.getMinX() + "," + + "\"right\": " + bbox.getMaxX() + "}" + + "}"); + XContentParser.Token token = stParser.nextToken(); + assertSame(XContentParser.Token.START_OBJECT, token); + // can create a factory + assertNotNull(GeoHashGridAggregationBuilder.parse("geohash_grid", stParser)); + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java index 6544344543e34..85b2306403230 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridAggregatorTests.java @@ -46,5 +46,4 @@ public void testPrecision() { builder.precision(precision); assertEquals(precision, builder.precision()); } - } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridParserTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridParserTests.java index 6f15263a53b11..aa5253564fb49 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridParserTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileGridParserTests.java @@ -22,6 +22,8 @@ import org.elasticsearch.common.xcontent.XContentParseException; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.test.ESTestCase; import static org.hamcrest.Matchers.containsString; @@ -70,4 +72,19 @@ public void testParseErrorOnPrecisionOutOfRange() throws Exception { assertEquals("Invalid geotile_grid precision of 30. Must be between 0 and 29.", ex.getCause().getMessage()); } } + + public void testParseValidBounds() throws Exception { + Rectangle bbox = GeometryTestUtils.randomRectangle(); + XContentParser stParser = createParser(JsonXContent.jsonXContent, + "{\"field\":\"my_loc\", \"precision\": 5, \"size\": 500, \"shard_size\": 550," + "\"bounds\": { " + + "\"top\": " + bbox.getMaxY() + "," + + "\"bottom\": " + bbox.getMinY() + "," + + "\"left\": " + bbox.getMinX() + "," + + "\"right\": " + bbox.getMaxX() + "}" + + "}"); + XContentParser.Token token = stParser.nextToken(); + assertSame(XContentParser.Token.START_OBJECT, token); + // can create a factory + assertNotNull(GeoTileGridAggregationBuilder.parse("geotile_grid", stParser)); + } } From dc738519c8ab9f5e41f15f5e327825814868197c Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Mon, 23 Dec 2019 09:19:59 -0800 Subject: [PATCH 06/12] update to geo-bounding-box and modify tests --- .../common/geo/GeoBoundingBoxTests.java | 77 +++++++++++++------ .../geogrid/GeoGridAggregatorTestCase.java | 43 ++++++----- 2 files changed, 76 insertions(+), 44 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java index 7105ef6d1e060..91b886182b1d6 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java @@ -19,6 +19,7 @@ package org.elasticsearch.common.geo; +import org.apache.lucene.geo.GeoEncodingUtils; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentFactory; @@ -136,28 +137,43 @@ public void testNullTopBottomLeftRight() throws Exception { public void testPointInBounds() { - for (int iter = 0; iter < 100; iter++) { - Rectangle rectangle = GeometryTestUtils.randomRectangle(); - GeoBoundingBox geoBoundingBox = new GeoBoundingBox(new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), - new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())); - if (rectangle.getMinX() > rectangle.getMaxX()) { + for (int iter = 0; iter < 1000; iter++) { + GeoBoundingBox geoBoundingBox = randomBBox(); + GeoBoundingBox bbox = new GeoBoundingBox( + new GeoPoint(quantizeLat(geoBoundingBox.top()), quantizeLon(geoBoundingBox.left())), + new GeoPoint(quantizeLat(geoBoundingBox.bottom()), quantizeLon(geoBoundingBox.right()))); + if (bbox.left() > bbox.right()) { double lonWithin = randomBoolean() ? - randomDoubleBetween(rectangle.getMinX(), 180.0, true) - : randomDoubleBetween(-180.0, rectangle.getMaxX(), true); - double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); - double lonOutside = (rectangle.getMinX() + rectangle.getMaxX()) / 2; - double latOutside = rectangle.getMinX() - randomIntBetween(1, 10); - - assertTrue(geoBoundingBox.pointInBounds(lonWithin, latWithin)); - assertFalse(geoBoundingBox.pointInBounds(lonOutside, latOutside)); + randomDoubleBetween(bbox.left(), 180.0, true) + : randomDoubleBetween(-180.0, bbox.right(), true); + double latWithin = randomDoubleBetween(bbox.bottom(), bbox.top(), true); + double lonOutside = randomDoubleBetween(bbox.left(), bbox.right(), true); + double latOutside = randomDoubleBetween(bbox.top(), -90, false); + + latWithin = quantizeLat(latWithin); + lonWithin = quantizeLon(lonWithin); + latOutside = quantizeLat(latOutside); + lonOutside = quantizeLon(lonOutside); + + try { + assertTrue(geoBoundingBox.pointInBounds(lonWithin, latWithin)); + } catch (AssertionError e) { + System.out.println(geoBoundingBox); + } + //assertFalse(bbox.pointInBounds(lonOutside, latOutside)); } else { - double lonWithin = randomDoubleBetween(rectangle.getMinX(), rectangle.getMaxX(), true); - double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); - double lonOutside = randomDoubleBetween(rectangle.getMaxX(), 180.0, false); - double latOutside = randomDoubleBetween(rectangle.getMaxY(), 90.0, false); - - assertTrue(geoBoundingBox.pointInBounds(lonWithin, latWithin)); - assertFalse(geoBoundingBox.pointInBounds(lonOutside, latOutside)); + double lonWithin = randomDoubleBetween(bbox.left(), bbox.right(), true); + double latWithin = randomDoubleBetween(bbox.bottom(), bbox.top(), true); + double lonOutside = GeoUtils.normalizeLon(randomDoubleBetween(bbox.right(), 180.001, false)); + double latOutside = GeoUtils.normalizeLat(randomDoubleBetween(bbox.top(), 90.001, false)); + + latWithin = quantizeLat(latWithin); + lonWithin = quantizeLon(lonWithin); + latOutside = quantizeLat(latOutside); + lonOutside = quantizeLon(lonOutside); + + //assertTrue(bbox.pointInBounds(lonWithin, latWithin)); + //assertFalse(bbox.pointInBounds(lonOutside, latOutside)); } } } @@ -170,9 +186,22 @@ private void assertBBox(GeoBoundingBox expected, XContentBuilder builder) throws } public static GeoBoundingBox randomBBox() { - double topLat = GeometryTestUtils.randomLat(); - double bottomLat = randomDoubleBetween(GeoUtils.MIN_LAT, topLat, true); - return new GeoBoundingBox(new GeoPoint(topLat, GeometryTestUtils.randomLon()), - new GeoPoint(bottomLat, GeometryTestUtils.randomLon())); + // avoid bounding boxes with a pole as a corner + return randomValueOtherThanMany(bbox -> + (Math.abs(bbox.top()) == 90 && Math.abs(bbox.left()) == 180) + || (Math.abs(bbox.bottom()) == 90 && Math.abs(bbox.right()) == 180) + , () -> { + Rectangle rectangle = GeometryTestUtils.randomRectangle(); + return new GeoBoundingBox(new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), + new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())); + }); + } + + private static double quantizeLat(double lat) { + return GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(lat)); + } + + private static double quantizeLon(double lon) { + return GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(lon)); } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java index ebd7a2b8ddc2a..98e25c5db435e 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java @@ -29,7 +29,9 @@ import org.apache.lucene.store.Directory; import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoBoundingBoxTests; import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.geo.GeoUtils; import org.elasticsearch.geo.GeometryTestUtils; import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.index.mapper.GeoPointFieldMapper; @@ -49,6 +51,7 @@ import java.util.function.Consumer; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.greaterThanOrEqualTo; public abstract class GeoGridAggregatorTestCase extends AggregatorTestCase { @@ -134,35 +137,34 @@ public void testBounds() throws IOException { expectThrows(IllegalArgumentException.class, () -> builder.precision(-1)); expectThrows(IllegalArgumentException.class, () -> builder.precision(30)); - Rectangle rectangle = GeometryTestUtils.randomRectangle(); + GeoBoundingBox bbox = GeoBoundingBoxTests.randomBBox(); int in = 0, out = 0; List docs = new ArrayList<>(); while (in + out < numDocs) { - if (rectangle.getMinX() > rectangle.getMaxX()) { + if (bbox.left() > bbox.right()) { if (randomBoolean()) { double lonWithin = randomBoolean() ? - randomDoubleBetween(rectangle.getMinX(), 180.0, true) - : randomDoubleBetween(-180.0, rectangle.getMaxX(), true); - double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); + randomDoubleBetween(bbox.left(), 180.0, true) + : randomDoubleBetween(-180.0, bbox.right(), true); + double latWithin = randomDoubleBetween(bbox.bottom(), bbox.top(), true); in++; docs.add(new LatLonDocValuesField(FIELD_NAME, latWithin, lonWithin)); } else { - double lonOutside = (rectangle.getMinX() + rectangle.getMaxX()) / 2; - double latOutside = rectangle.getMinX() - randomIntBetween(1, 10); + double lonOutside = randomDoubleBetween(bbox.left(), bbox.right(), true); + double latOutside = randomDoubleBetween(bbox.top(), -90, false); out++; docs.add(new LatLonDocValuesField(FIELD_NAME, latOutside, lonOutside)); } - } else { if (randomBoolean()) { - double lonWithin = randomDoubleBetween(rectangle.getMinX(), rectangle.getMaxX(), true); - double latWithin = randomDoubleBetween(rectangle.getMinY(), rectangle.getMaxY(), true); + double lonWithin = randomDoubleBetween(bbox.left(), bbox.right(), true); + double latWithin = randomDoubleBetween(bbox.bottom(), bbox.top(), true); in++; docs.add(new LatLonDocValuesField(FIELD_NAME, latWithin, lonWithin)); } else { - double lonOutside = randomDoubleBetween(rectangle.getMaxX(), 180.0, false); - double latOutside = randomDoubleBetween(rectangle.getMaxY(), 90.0, false); + double lonOutside = GeoUtils.normalizeLon(randomDoubleBetween(bbox.right(), 180.001, false)); + double latOutside = GeoUtils.normalizeLat(randomDoubleBetween(bbox.top(), 90.001, false)); out++; docs.add(new LatLonDocValuesField(FIELD_NAME, latOutside, lonOutside)); } @@ -171,12 +173,15 @@ public void testBounds() throws IOException { } final long numDocsInBucket = in; + final int precision = randomPrecision(); - testCase(new MatchAllDocsQuery(), FIELD_NAME, 0, rectangle, geoGrid -> { - assertThat(geoGrid.getBuckets().size(), equalTo(1)); - long docCount = geoGrid.getBuckets().get(0).getDocCount(); - assertThat(docCount, equalTo(numDocsInBucket)); + testCase(new MatchAllDocsQuery(), FIELD_NAME, precision, bbox, geoGrid -> { assertTrue(AggregationInspectionHelper.hasValue(geoGrid)); + long docCount = 0; + for (int i = 0; i < geoGrid.getBuckets().size(); i++) { + docCount += geoGrid.getBuckets().get(i).getDocCount(); + } + assertThat(docCount, equalTo(numDocsInBucket)); }, iw -> { for (LatLonDocValuesField docField : docs) { iw.addDocument(Collections.singletonList(docField)); @@ -184,7 +189,7 @@ public void testBounds() throws IOException { }); } - private void testCase(Query query, String field, int precision, Rectangle boundingBox, + private void testCase(Query query, String field, int precision, GeoBoundingBox geoBoundingBox, Consumer> verify, CheckedConsumer buildIndex) throws IOException { Directory directory = newDirectory(); @@ -197,9 +202,7 @@ private void testCase(Query query, String field, int precision, Rectangle boundi GeoGridAggregationBuilder aggregationBuilder = createBuilder("_name").field(field); aggregationBuilder.precision(precision); - if (boundingBox != null) { - GeoBoundingBox geoBoundingBox = new GeoBoundingBox(new GeoPoint(boundingBox.getMaxLat(), boundingBox.getMinLon()), - new GeoPoint(boundingBox.getMinLat(), boundingBox.getMaxLon())); + if (geoBoundingBox != null) { aggregationBuilder.setGeoBoundingBox(geoBoundingBox); assertThat(aggregationBuilder.geoBoundingBox(), equalTo(geoBoundingBox)); } From c5478ceef5376de2daff7945815c5637938960dc Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Fri, 3 Jan 2020 11:37:12 -0800 Subject: [PATCH 07/12] fix unused --- .../bucket/geogrid/GeoGridAggregatorTestCase.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java index 98e25c5db435e..085be3855ac9f 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoGridAggregatorTestCase.java @@ -30,10 +30,7 @@ import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.geo.GeoBoundingBoxTests; -import org.elasticsearch.common.geo.GeoPoint; import org.elasticsearch.common.geo.GeoUtils; -import org.elasticsearch.geo.GeometryTestUtils; -import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.index.mapper.GeoPointFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.search.aggregations.Aggregator; @@ -51,7 +48,6 @@ import java.util.function.Consumer; import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.greaterThanOrEqualTo; public abstract class GeoGridAggregatorTestCase extends AggregatorTestCase { From 1d6451456eb8eaafef5ed84bf0d60059e149dd4c Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Fri, 3 Jan 2020 13:58:49 -0800 Subject: [PATCH 08/12] fix pointInBounds tests --- .../common/geo/GeoBoundingBoxTests.java | 43 ++++++------------- 1 file changed, 12 insertions(+), 31 deletions(-) diff --git a/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java index 1fadd2099a4cd..309302214f462 100644 --- a/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java +++ b/server/src/test/java/org/elasticsearch/common/geo/GeoBoundingBoxTests.java @@ -147,32 +147,19 @@ public void testPointInBounds() { : randomDoubleBetween(-180.0, bbox.right(), true); double latWithin = randomDoubleBetween(bbox.bottom(), bbox.top(), true); double lonOutside = randomDoubleBetween(bbox.left(), bbox.right(), true); - double latOutside = randomDoubleBetween(bbox.top(), -90, false); - - latWithin = quantizeLat(latWithin); - lonWithin = quantizeLon(lonWithin); - latOutside = quantizeLat(latOutside); - lonOutside = quantizeLon(lonOutside); - - try { - assertTrue(geoBoundingBox.pointInBounds(lonWithin, latWithin)); - } catch (AssertionError e) { - System.out.println(geoBoundingBox); - } - //assertFalse(bbox.pointInBounds(lonOutside, latOutside)); + double latOutside = randomBoolean() ? randomDoubleBetween(Math.max(bbox.top(), bbox.bottom()), 90, false) + : randomDoubleBetween(-90, Math.min(bbox.bottom(), bbox.top()), false); + + assertTrue(bbox.pointInBounds(lonWithin, latWithin)); + assertFalse(bbox.pointInBounds(lonOutside, latOutside)); } else { double lonWithin = randomDoubleBetween(bbox.left(), bbox.right(), true); double latWithin = randomDoubleBetween(bbox.bottom(), bbox.top(), true); - double lonOutside = GeoUtils.normalizeLon(randomDoubleBetween(bbox.right(), 180.001, false)); - double latOutside = GeoUtils.normalizeLat(randomDoubleBetween(bbox.top(), 90.001, false)); - - latWithin = quantizeLat(latWithin); - lonWithin = quantizeLon(lonWithin); - latOutside = quantizeLat(latOutside); - lonOutside = quantizeLon(lonOutside); + double lonOutside = GeoUtils.normalizeLon(randomDoubleBetween(bbox.right(), 180, false)); + double latOutside = GeoUtils.normalizeLat(randomDoubleBetween(bbox.top(), 90, false)); - //assertTrue(bbox.pointInBounds(lonWithin, latWithin)); - //assertFalse(bbox.pointInBounds(lonOutside, latOutside)); + assertTrue(bbox.pointInBounds(lonWithin, latWithin)); + assertFalse(bbox.pointInBounds(lonOutside, latOutside)); } } } @@ -185,15 +172,9 @@ private void assertBBox(GeoBoundingBox expected, XContentBuilder builder) throws } public static GeoBoundingBox randomBBox() { - // avoid bounding boxes with a pole as a corner - return randomValueOtherThanMany(bbox -> - (Math.abs(bbox.top()) == 90 && Math.abs(bbox.left()) == 180) - || (Math.abs(bbox.bottom()) == 90 && Math.abs(bbox.right()) == 180) - , () -> { - Rectangle rectangle = GeometryTestUtils.randomRectangle(); - return new GeoBoundingBox(new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), - new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())); - }); + Rectangle rectangle = GeometryTestUtils.randomRectangle(); + return new GeoBoundingBox(new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), + new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())); } private static double quantizeLat(double lat) { From d12fd752c282b55b5155b89150768fc3304cfefe Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Mon, 6 Jan 2020 13:40:58 -0800 Subject: [PATCH 09/12] add bwc serialization tests --- .../GeoTileGridValuesSourceBuilder.java | 4 +++ .../aggregations/bucket/GeoHashGridTests.java | 27 ++++++++++++++++ .../aggregations/bucket/GeoTileGridTests.java | 27 ++++++++++++++++ .../GeoTileGridValuesSourceBuilderTests.java | 32 +++++++++++++++++++ 4 files changed, 90 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java index c8aef28f871ef..30728af31b730 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java @@ -108,6 +108,10 @@ String type() { return TYPE; } + GeoBoundingBox geoBoundingBox() { + return geoBoundingBox; + } + @Override public int hashCode() { return Objects.hash(super.hashCode(), precision, geoBoundingBox); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java index 629ccfbce08f1..883810b25d76b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoHashGridTests.java @@ -19,10 +19,22 @@ package org.elasticsearch.search.aggregations.bucket; +import org.elasticsearch.Version; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.geo.GeoBoundingBoxTests; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoHashGridAggregationBuilder; +import org.elasticsearch.test.VersionUtils; + +import java.util.Collections; + +import static org.hamcrest.Matchers.equalTo; public class GeoHashGridTests extends BaseAggregationTestCase { @@ -46,4 +58,19 @@ protected GeoHashGridAggregationBuilder createTestAggregatorBuilder() { return factory; } + public void testSerializationPreBounds() throws Exception { + Version noBoundsSupportVersion = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0); + GeoHashGridAggregationBuilder builder = createTestAggregatorBuilder(); + try (BytesStreamOutput output = new BytesStreamOutput()) { + output.setVersion(Version.V_8_0_0); + builder.writeTo(output); + try (StreamInput in = new NamedWriteableAwareStreamInput(output.bytes().streamInput(), + new NamedWriteableRegistry(Collections.emptyList()))) { + in.setVersion(noBoundsSupportVersion); + GeoHashGridAggregationBuilder readBuilder = new GeoHashGridAggregationBuilder(in); + assertThat(readBuilder.geoBoundingBox(), equalTo(new GeoBoundingBox( + new GeoPoint(Double.NaN, Double.NaN), new GeoPoint(Double.NaN, Double.NaN)))); + } + } + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java index 825c201139442..33631d39f720d 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoTileGridTests.java @@ -19,11 +19,23 @@ package org.elasticsearch.search.aggregations.bucket; +import org.elasticsearch.Version; +import org.elasticsearch.common.geo.GeoBoundingBox; import org.elasticsearch.common.geo.GeoBoundingBoxTests; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.search.aggregations.BaseAggregationTestCase; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils; +import org.elasticsearch.test.VersionUtils; + +import java.util.Collections; + +import static org.hamcrest.Matchers.equalTo; public class GeoTileGridTests extends BaseAggregationTestCase { @@ -46,4 +58,19 @@ protected GeoTileGridAggregationBuilder createTestAggregatorBuilder() { return factory; } + public void testSerializationPreBounds() throws Exception { + Version noBoundsSupportVersion = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0); + GeoTileGridAggregationBuilder builder = createTestAggregatorBuilder(); + try (BytesStreamOutput output = new BytesStreamOutput()) { + output.setVersion(Version.V_8_0_0); + builder.writeTo(output); + try (StreamInput in = new NamedWriteableAwareStreamInput(output.bytes().streamInput(), + new NamedWriteableRegistry(Collections.emptyList()))) { + in.setVersion(noBoundsSupportVersion); + GeoTileGridAggregationBuilder readBuilder = new GeoTileGridAggregationBuilder(in); + assertThat(readBuilder.geoBoundingBox(), equalTo(new GeoBoundingBox( + new GeoPoint(Double.NaN, Double.NaN), new GeoPoint(Double.NaN, Double.NaN)))); + } + } + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilderTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilderTests.java index 6f9e0f697da25..7817a236d6680 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilderTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilderTests.java @@ -19,7 +19,21 @@ package org.elasticsearch.search.aggregations.bucket.composite; +import org.elasticsearch.Version; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoBoundingBoxTests; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.io.stream.BytesStreamOutput; +import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; +import org.elasticsearch.common.io.stream.NamedWriteableRegistry; +import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.VersionUtils; + +import java.io.IOException; +import java.util.Collections; + +import static org.hamcrest.Matchers.equalTo; public class GeoTileGridValuesSourceBuilderTests extends ESTestCase { @@ -28,4 +42,22 @@ public void testSetFormat() { expectThrows(IllegalArgumentException.class, () -> builder.format("format")); } + public void testBWCBounds() throws IOException { + Version noBoundsSupportVersion = VersionUtils.randomPreviousCompatibleVersion(random(), Version.V_8_0_0); + GeoTileGridValuesSourceBuilder builder = new GeoTileGridValuesSourceBuilder("name"); + if (randomBoolean()) { + builder.geoBoundingBox(GeoBoundingBoxTests.randomBBox()); + } + try (BytesStreamOutput output = new BytesStreamOutput()) { + output.setVersion(Version.V_8_0_0); + builder.writeTo(output); + try (StreamInput in = new NamedWriteableAwareStreamInput(output.bytes().streamInput(), + new NamedWriteableRegistry(Collections.emptyList()))) { + in.setVersion(noBoundsSupportVersion); + GeoTileGridValuesSourceBuilder readBuilder = new GeoTileGridValuesSourceBuilder(in); + assertThat(readBuilder.geoBoundingBox(), equalTo(new GeoBoundingBox( + new GeoPoint(Double.NaN, Double.NaN), new GeoPoint(Double.NaN, Double.NaN)))); + } + } + } } From 8016c70299049d423322fa45df8d0911b3d709e1 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Thu, 9 Jan 2020 12:16:26 -0800 Subject: [PATCH 10/12] split out bounded and unbounded cellvalues --- .../bucket/geogrid/BoundedCellValues.java | 48 +++++++++++++ .../bucket/geogrid/CellIdSource.java | 41 ++--------- .../bucket/geogrid/CellValues.java | 68 +++++++++++++++++++ .../bucket/geogrid/UnboundedCellValues.java | 39 +++++++++++ 4 files changed, 159 insertions(+), 37 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/UnboundedCellValues.java diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java new file mode 100644 index 0000000000000..f727f323ca57a --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java @@ -0,0 +1,48 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.index.fielddata.MultiGeoPointValues; + +/** + * Class representing {@link CellValues} whose values are filtered + * according to whether they are within the specified {@link GeoBoundingBox}. + * + * The specified bounding box is assumed to be bounded. + */ +class BoundedCellValues extends CellValues { + + private final GeoBoundingBox geoBoundingBox; + + protected BoundedCellValues(MultiGeoPointValues geoValues, int precision, CellIdSource.GeoPointLongEncoder encoder, + GeoBoundingBox geoBoundingBox) { + super(geoValues, precision, encoder); + this.geoBoundingBox = geoBoundingBox; + } + + + @Override + int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) { + if (geoBoundingBox.pointInBounds(target.getLon(), target.getLat())) { + values[valuesIdx] = encoder.encode(target.getLon(), target.getLat(), precision); + } + return valuesIdx + 1; + } +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java index 3ef91e2d61120..84f963bbbd9ce 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java @@ -21,14 +21,11 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.SortedNumericDocValues; import org.elasticsearch.common.geo.GeoBoundingBox; -import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues; import org.elasticsearch.index.fielddata.MultiGeoPointValues; import org.elasticsearch.index.fielddata.SortedBinaryDocValues; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; import org.elasticsearch.search.aggregations.support.ValuesSource; -import java.io.IOException; - /** * Wrapper class to help convert {@link MultiGeoPointValues} * to numeric long values for bucketing. @@ -58,7 +55,10 @@ public boolean isFloatingPoint() { @Override public SortedNumericDocValues longValues(LeafReaderContext ctx) { - return new CellValues(valuesSource.geoPointValues(ctx), precision, geoBoundingBox, encoder); + if (geoBoundingBox.isUnbounded()) { + return new UnboundedCellValues(valuesSource.geoPointValues(ctx), precision, encoder); + } + return new BoundedCellValues(valuesSource.geoPointValues(ctx), precision, encoder, geoBoundingBox); } @Override @@ -80,37 +80,4 @@ public interface GeoPointLongEncoder { long encode(double lon, double lat, int precision); } - private static class CellValues extends AbstractSortingNumericDocValues { - private MultiGeoPointValues geoValues; - private int precision; - private GeoPointLongEncoder encoder; - private GeoBoundingBox geoBoundingBox; - - protected CellValues(MultiGeoPointValues geoValues, int precision, GeoBoundingBox geoBoundingBox, GeoPointLongEncoder encoder) { - this.geoValues = geoValues; - this.precision = precision; - this.encoder = encoder; - this.geoBoundingBox = geoBoundingBox; - } - - @Override - public boolean advanceExact(int docId) throws IOException { - if (geoValues.advanceExact(docId)) { - int docValueCount = geoValues.docValueCount(); - resize(docValueCount); - int j = 0; - for (int i = 0; i < docValueCount; i++) { - org.elasticsearch.common.geo.GeoPoint target = geoValues.nextValue(); - if (geoBoundingBox.isUnbounded() || geoBoundingBox.pointInBounds(target.getLon(), target.getLat())) { - values[j++] = encoder.encode(target.getLon(), target.getLat(), precision); - } - } - resize(j); - sort(); - return true; - } else { - return false; - } - } - } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java new file mode 100644 index 0000000000000..5ca018d159f56 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java @@ -0,0 +1,68 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.elasticsearch.index.fielddata.AbstractSortingNumericDocValues; +import org.elasticsearch.index.fielddata.MultiGeoPointValues; + +import java.io.IOException; + +/** + * Class representing the long-encoded grid-cells belonging to + * the geo-doc-values. Class must encode the values and then + * sort them in order to account for the cells correctly. + */ +abstract class CellValues extends AbstractSortingNumericDocValues { + private MultiGeoPointValues geoValues; + protected int precision; + protected CellIdSource.GeoPointLongEncoder encoder; + + protected CellValues(MultiGeoPointValues geoValues, int precision, CellIdSource.GeoPointLongEncoder encoder) { + this.geoValues = geoValues; + this.precision = precision; + this.encoder = encoder; + } + + @Override + public boolean advanceExact(int docId) throws IOException { + if (geoValues.advanceExact(docId)) { + int docValueCount = geoValues.docValueCount(); + resize(docValueCount); + int j = 0; + for (int i = 0; i < docValueCount; i++) { + advanceValue(geoValues.nextValue(), j); + } + resize(j); + sort(); + return true; + } else { + return false; + } + } + + /** + * Sets the appropriate long-encoded value for target + * in values. + * + * @param target the geo-value to encode + * @param valuesIdx the index into values to set + * @return valuesIdx + 1 if value was set, valuesIdx otherwise. + */ + abstract int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx); +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/UnboundedCellValues.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/UnboundedCellValues.java new file mode 100644 index 0000000000000..e64061eae5ae5 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/UnboundedCellValues.java @@ -0,0 +1,39 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.index.fielddata.MultiGeoPointValues; + +/** + * Class representing {@link CellValues} that are unbounded by any + * {@link GeoBoundingBox}. + */ +class UnboundedCellValues extends CellValues { + + UnboundedCellValues(MultiGeoPointValues geoValues, int precision, CellIdSource.GeoPointLongEncoder encoder) { + super(geoValues, precision, encoder); + } + + @Override + int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) { + values[valuesIdx] = encoder.encode(target.getLon(), target.getLat(), precision); + return valuesIdx + 1; + } +} From 859cf9b7b4ef431508a7ae372ba873f5a3b1a719 Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Thu, 9 Jan 2020 12:20:16 -0800 Subject: [PATCH 11/12] fix forgotten bug --- .../search/aggregations/bucket/geogrid/CellValues.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java index 5ca018d159f56..5d428373ccd8f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellValues.java @@ -46,7 +46,7 @@ public boolean advanceExact(int docId) throws IOException { resize(docValueCount); int j = 0; for (int i = 0; i < docValueCount; i++) { - advanceValue(geoValues.nextValue(), j); + j = advanceValue(geoValues.nextValue(), j); } resize(j); sort(); From c54ede96c64ac514543920e5f21632fb4d9b000d Mon Sep 17 00:00:00 2001 From: Tal Levy Date: Thu, 9 Jan 2020 13:41:15 -0800 Subject: [PATCH 12/12] fix other bug --- .../search/aggregations/bucket/geogrid/BoundedCellValues.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java index f727f323ca57a..52fddb34b0576 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java @@ -42,7 +42,8 @@ protected BoundedCellValues(MultiGeoPointValues geoValues, int precision, CellId int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) { if (geoBoundingBox.pointInBounds(target.getLon(), target.getLat())) { values[valuesIdx] = encoder.encode(target.getLon(), target.getLat(), precision); + return valuesIdx + 1; } - return valuesIdx + 1; + return valuesIdx; } }