From 793166fd1f8dcaa5a54d0a8bc17272e913611b71 Mon Sep 17 00:00:00 2001 From: Ignacio Vera Date: Fri, 30 Apr 2021 08:43:18 +0200 Subject: [PATCH] [GeoPoint] Grid aggregations with bounds should exclude touching tiles (#72493) --- .../bucket/geohashgrid-aggregation.asciidoc | 2 +- .../bucket/geotilegrid-aggregation.asciidoc | 4 +- .../GeoTileGridValuesSourceBuilder.java | 9 +- .../bucket/geogrid/BoundedCellValues.java | 38 ------ .../bucket/geogrid/CellIdSource.java | 72 ----------- .../bucket/geogrid/CellValues.java | 4 +- .../bucket/geogrid/GeoHashCellIdSource.java | 108 ++++++++++++++++ .../geogrid/GeoHashGridAggregatorFactory.java | 9 +- .../bucket/geogrid/GeoTileCellIdSource.java | 122 ++++++++++++++++++ .../geogrid/GeoTileGridAggregatorFactory.java | 5 +- .../bucket/geogrid/UnboundedCellValues.java | 28 ---- .../aggregations/support/ValuesSource.java | 5 +- .../geogrid/GeoGridAggregatorTestCase.java | 106 ++++++++------- .../geogrid/GeoHashGridAggregatorTests.java | 24 ++++ .../geogrid/GeoTileGridAggregatorTests.java | 46 +++++++ 15 files changed, 375 insertions(+), 207 deletions(-) delete mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java delete mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileCellIdSource.java delete mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/UnboundedCellValues.java diff --git a/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc b/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc index 99c4d70d7132a..b39d5c2cebd1e 100644 --- a/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc +++ b/docs/reference/aggregations/bucket/geohashgrid-aggregation.asciidoc @@ -200,7 +200,7 @@ var bbox = geohash.decode_bbox('u17'); ==== 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 +that restricts the cells considered to those that intersects 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 diff --git a/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc b/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc index 7b1c010f0f841..0f6f24e784962 100644 --- a/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc +++ b/docs/reference/aggregations/bucket/geotilegrid-aggregation.asciidoc @@ -169,11 +169,11 @@ POST /museums/_search?size=0 ==== 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 +that restricts the cells considered to those that intersects 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. +without an additional `geo_bounding_box` query for 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. 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 3c6623f509936..9238a2fc400e4 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 @@ -20,7 +20,7 @@ import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.search.DocValueFormat; -import org.elasticsearch.search.aggregations.bucket.geogrid.CellIdSource; +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileCellIdSource; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder; import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileUtils; import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; @@ -78,11 +78,10 @@ static void register(ValuesSourceRegistry.Builder builder) { ValuesSource.GeoPoint geoPoint = (ValuesSource.GeoPoint) valuesSourceConfig.getValuesSource(); // is specified in the builder. final MappedFieldType fieldType = valuesSourceConfig.fieldType(); - CellIdSource cellIdSource = new CellIdSource( + GeoTileCellIdSource cellIdSource = new GeoTileCellIdSource( geoPoint, precision, - boundingBox, - GeoTileUtils::longEncode + boundingBox ); return new CompositeValuesSourceConfig( name, @@ -100,7 +99,7 @@ static void register(ValuesSourceRegistry.Builder builder) { CompositeValuesSourceConfig compositeValuesSourceConfig ) -> { - final CellIdSource cis = (CellIdSource) compositeValuesSourceConfig.valuesSource(); + final ValuesSource.Numeric cis = (ValuesSource.Numeric) compositeValuesSourceConfig.valuesSource(); return new GeoTileValuesSource( bigArrays, compositeValuesSourceConfig.fieldType(), 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 deleted file mode 100644 index f3fb17d057bc8..0000000000000 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/BoundedCellValues.java +++ /dev/null @@ -1,38 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ -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; - } - return valuesIdx; - } -} 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 deleted file mode 100644 index 21a1e52b8196d..0000000000000 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/CellIdSource.java +++ /dev/null @@ -1,72 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ -package org.elasticsearch.search.aggregations.bucket.geogrid; - -import org.apache.lucene.index.LeafReaderContext; -import org.apache.lucene.index.SortedNumericDocValues; -import org.elasticsearch.common.geo.GeoBoundingBox; -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; - -/** - * Wrapper class to help convert {@link MultiGeoPointValues} - * to numeric long values for bucketing. - */ -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, 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; - } - - public int precision() { - return precision; - } - - @Override - public boolean isFloatingPoint() { - return false; - } - - @Override - public SortedNumericDocValues longValues(LeafReaderContext ctx) { - if (geoBoundingBox.isUnbounded()) { - return new UnboundedCellValues(valuesSource.geoPointValues(ctx), precision, encoder); - } - return new BoundedCellValues(valuesSource.geoPointValues(ctx), precision, encoder, geoBoundingBox); - } - - @Override - public SortedNumericDoubleValues doubleValues(LeafReaderContext ctx) { - throw new UnsupportedOperationException(); - } - - @Override - public SortedBinaryDocValues bytesValues(LeafReaderContext ctx) { - throw new UnsupportedOperationException(); - } - - /** - * The encoder to use to convert a geopoint's (lon, lat, precision) into - * a long-encoded bucket key for aggregating. - */ - @FunctionalInterface - public interface GeoPointLongEncoder { - long encode(double lon, double lat, int precision); - } - -} 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 8ea03408177f8..424fbc9c2dfda 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 @@ -20,12 +20,10 @@ abstract class CellValues extends AbstractSortingNumericDocValues { private MultiGeoPointValues geoValues; protected int precision; - protected CellIdSource.GeoPointLongEncoder encoder; - protected CellValues(MultiGeoPointValues geoValues, int precision, CellIdSource.GeoPointLongEncoder encoder) { + protected CellValues(MultiGeoPointValues geoValues, int precision) { this.geoValues = geoValues; this.precision = precision; - this.encoder = encoder; } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java new file mode 100644 index 0000000000000..ac7c08946a4d8 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashCellIdSource.java @@ -0,0 +1,108 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.SortedNumericDocValues; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.geometry.Rectangle; +import org.elasticsearch.geometry.utils.Geohash; +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; + +/** + * Class to help convert {@link MultiGeoPointValues} + * to GeoHash bucketing. + */ +public class GeoHashCellIdSource extends ValuesSource.Numeric { + private final GeoPoint valuesSource; + private final int precision; + private final GeoBoundingBox geoBoundingBox; + + public GeoHashCellIdSource(GeoPoint valuesSource, int precision, GeoBoundingBox geoBoundingBox) { + this.valuesSource = valuesSource; + this.precision = precision; + this.geoBoundingBox = geoBoundingBox; + } + + public int precision() { + return precision; + } + + @Override + public boolean isFloatingPoint() { + return false; + } + + @Override + public SortedNumericDocValues longValues(LeafReaderContext ctx) { + return geoBoundingBox.isUnbounded() ? + new UnboundedCellValues(valuesSource.geoPointValues(ctx), precision) : + new BoundedCellValues(valuesSource.geoPointValues(ctx), precision, geoBoundingBox); + } + + @Override + public SortedNumericDoubleValues doubleValues(LeafReaderContext ctx) { + throw new UnsupportedOperationException(); + } + + @Override + public SortedBinaryDocValues bytesValues(LeafReaderContext ctx) { + throw new UnsupportedOperationException(); + } + + private static class UnboundedCellValues extends CellValues { + + UnboundedCellValues(MultiGeoPointValues geoValues, int precision) { + super(geoValues, precision); + } + + @Override + int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) { + values[valuesIdx] = Geohash.longEncode(target.getLon(), target.getLat(), precision); + return valuesIdx + 1; + } + } + + private static class BoundedCellValues extends CellValues { + + private final GeoBoundingBox bbox; + private final boolean crossesDateline; + + BoundedCellValues(MultiGeoPointValues geoValues, int precision, GeoBoundingBox bbox) { + super(geoValues, precision); + this.bbox = bbox; + this.crossesDateline = bbox.right() < bbox.left(); + } + + @Override + int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) { + final String hash = Geohash.stringEncode(target.getLon(), target.getLat(), precision); + if (validHash(hash)) { + values[valuesIdx] = Geohash.longEncode(hash); + return valuesIdx + 1; + } + return valuesIdx; + } + + private boolean validHash(String hash) { + final Rectangle rectangle = Geohash.toBoundingBox(hash); + // touching hashes are excluded + if (bbox.top() > rectangle.getMinY() && bbox.bottom() < rectangle.getMaxY()) { + if (crossesDateline) { + return bbox.left() < rectangle.getMaxX() || bbox.right() > rectangle.getMinX(); + } else { + return bbox.left() < rectangle.getMaxX() && bbox.right() > rectangle.getMinX(); + } + } + return false; + } + } +} 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 3554f6cf2e086..b299e9217fde7 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 @@ -9,7 +9,6 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; import org.elasticsearch.common.geo.GeoBoundingBox; -import org.elasticsearch.geometry.utils.Geohash; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorFactories; import org.elasticsearch.search.aggregations.AggregatorFactory; @@ -98,11 +97,10 @@ static void registerAggregators(ValuesSourceRegistry.Builder builder) { parent, cardinality, metadata) -> { - CellIdSource cellIdSource = new CellIdSource( + GeoHashCellIdSource cellIdSource = new GeoHashCellIdSource( (ValuesSource.GeoPoint) valuesSource, precision, - geoBoundingBox, - Geohash::longEncode + geoBoundingBox ); return new GeoHashGridAggregator( name, @@ -115,7 +113,6 @@ static void registerAggregators(ValuesSourceRegistry.Builder builder) { cardinality, metadata ); - }, - true); + }, true); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileCellIdSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileCellIdSource.java new file mode 100644 index 0000000000000..2f597921834d9 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoTileCellIdSource.java @@ -0,0 +1,122 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ +package org.elasticsearch.search.aggregations.bucket.geogrid; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.SortedNumericDocValues; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.geometry.Rectangle; +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; + +/** + * Class to help convert {@link MultiGeoPointValues} + * to GeoTile bucketing. + */ +public class GeoTileCellIdSource extends ValuesSource.Numeric { + private final GeoPoint valuesSource; + private final int precision; + private final GeoBoundingBox geoBoundingBox; + + public GeoTileCellIdSource(GeoPoint valuesSource, int precision, GeoBoundingBox geoBoundingBox) { + this.valuesSource = valuesSource; + this.precision = precision; + this.geoBoundingBox = geoBoundingBox; + } + + public int precision() { + return precision; + } + + @Override + public boolean isFloatingPoint() { + return false; + } + + @Override + public SortedNumericDocValues longValues(LeafReaderContext ctx) { + return geoBoundingBox.isUnbounded() ? + new UnboundedCellValues(valuesSource.geoPointValues(ctx), precision) : + new BoundedCellValues(valuesSource.geoPointValues(ctx), precision, geoBoundingBox); + } + + @Override + public SortedNumericDoubleValues doubleValues(LeafReaderContext ctx) { + throw new UnsupportedOperationException(); + } + + @Override + public SortedBinaryDocValues bytesValues(LeafReaderContext ctx) { + throw new UnsupportedOperationException(); + } + + private static class UnboundedCellValues extends CellValues { + + + UnboundedCellValues(MultiGeoPointValues geoValues, int precision) { + super(geoValues, precision); + } + + @Override + int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) { + values[valuesIdx] = GeoTileUtils.longEncode(target.getLon(), target.getLat(), precision); + return valuesIdx + 1; + } + } + + private static class BoundedCellValues extends CellValues { + + private final boolean crossesDateline; + private final long tiles; + private final int minX, maxX, minY, maxY; + + protected BoundedCellValues(MultiGeoPointValues geoValues, int precision, GeoBoundingBox bbox) { + super(geoValues, precision); + this.crossesDateline = bbox.right() < bbox.left(); + this.tiles = 1L << precision; + // compute minX, minY + final int minX = GeoTileUtils.getXTile(bbox.left(), this.tiles); + final int minY = GeoTileUtils.getYTile(bbox.top(), this.tiles); + final Rectangle minTile = GeoTileUtils.toBoundingBox(minX, minY, precision); + // touching tiles are excluded, they need to share at least one interior point + this.minX = minTile.getMaxX() == bbox.left() ? minX + 1: minX; + this.minY = minTile.getMinY() == bbox.top() ? minY + 1 : minY; + // compute maxX, maxY + final int maxX = GeoTileUtils.getXTile(bbox.right(), this.tiles); + final int maxY = GeoTileUtils.getYTile(bbox.bottom(), this.tiles); + final Rectangle maxTile = GeoTileUtils.toBoundingBox(maxX, maxY, precision); + // touching tiles are excluded, they need to share at least one interior point + this.maxX = maxTile.getMinX() == bbox.right() ? maxX - 1 : maxX; + this.maxY = maxTile.getMaxY() == bbox.bottom() ? maxY - 1 : maxY; + } + + @Override + int advanceValue(org.elasticsearch.common.geo.GeoPoint target, int valuesIdx) { + final int x = GeoTileUtils.getXTile(target.getLon(), this.tiles); + final int y = GeoTileUtils.getYTile(target.getLat(), this.tiles); + if (validTile(x, y)) { + values[valuesIdx] = GeoTileUtils.longEncodeTiles(precision, x, y); + return valuesIdx + 1; + } + return valuesIdx; + } + + private boolean validTile(int x, int y) { + if (maxY >= y && minY <= y) { + if (crossesDateline) { + return maxX >= x || minX <= x; + } else { + return maxX >= x && minX <= x; + } + } + return false; + } + } +} 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 40728606fe0b3..3144b197e78d0 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 @@ -85,11 +85,10 @@ static void registerAggregators(ValuesSourceRegistry.Builder builder) { parent, cardinality, metadata) -> { - CellIdSource cellIdSource = new CellIdSource( + GeoTileCellIdSource cellIdSource = new GeoTileCellIdSource( (ValuesSource.GeoPoint) valuesSource, precision, - geoBoundingBox, - GeoTileUtils::longEncode + geoBoundingBox ); return new GeoTileGridAggregator( name, 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 deleted file mode 100644 index 7d3be94d4322e..0000000000000 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/geogrid/UnboundedCellValues.java +++ /dev/null @@ -1,28 +0,0 @@ -/* - * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one - * or more contributor license agreements. Licensed under the Elastic License - * 2.0 and the Server Side Public License, v 1; you may not use this file except - * in compliance with, at your election, the Elastic License 2.0 or the Server - * Side Public License, v 1. - */ -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; - } -} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java index ccdfb962e36b6..ab7dd5f7f9c5a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java @@ -38,13 +38,12 @@ import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationExecutionException; import org.elasticsearch.search.aggregations.Aggregator; -import org.elasticsearch.search.aggregations.bucket.geogrid.CellIdSource; import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator; import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregator; import org.elasticsearch.search.aggregations.support.values.ScriptBytesValues; import org.elasticsearch.search.aggregations.support.values.ScriptDoubleValues; import org.elasticsearch.search.aggregations.support.values.ScriptLongValues; - +import org.elasticsearch.search.aggregations.bucket.geogrid.GeoTileCellIdSource; import java.io.IOException; import java.util.function.Function; import java.util.function.LongUnaryOperator; @@ -55,7 +54,7 @@ * top level sub-classes define type-specific behavior, such as * {@link ValuesSource.Numeric#isFloatingPoint()}. Second level subclasses are * then specialized based on where they read values from, e.g. script or field - * cases. There are also adapter classes like {@link CellIdSource} which do + * cases. There are also adapter classes like {@link GeoTileCellIdSource} which do * run-time conversion from one type to another, often dependent on a user * specified parameter (precision in that case). */ 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 509b1749951f1..1fadb63faaf26 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 @@ -22,8 +22,8 @@ import org.apache.lucene.util.BytesRef; import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.geo.GeoBoundingBox; -import org.elasticsearch.common.geo.GeoBoundingBoxTests; -import org.elasticsearch.common.geo.GeoUtils; +import org.elasticsearch.geometry.Point; +import org.elasticsearch.geometry.Rectangle; import org.elasticsearch.index.mapper.GeoPointFieldMapper; import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; @@ -55,7 +55,6 @@ public abstract class GeoGridAggregatorTestCase extends AggregatorTestCase { private static final String FIELD_NAME = "location"; - protected static final double GEOHASH_TOLERANCE = 1E-5D; /** * Generate a random precision according to the rules of the given aggregation. @@ -71,6 +70,20 @@ public abstract class GeoGridAggregatorTestCase * Create a new named {@link GeoGridAggregationBuilder}-derived builder */ protected abstract GeoGridAggregationBuilder createBuilder(String name); + /** + * Return a point within the bounds of the tile grid + */ + protected abstract Point randomPoint(); + + /** + * Return a random {@link GeoBoundingBox} within the bounds of the tile grid. + */ + protected abstract GeoBoundingBox randomBBox(); + + /** + * Return the bounding tile as a {@link Rectangle} for a given point + */ + protected abstract Rectangle getTile(double lng, double lat, int precision); @Override protected AggregationBuilder createAggBuilderForTypeTest(MappedFieldType fieldType, String fieldName) { @@ -202,58 +215,59 @@ public void testBounds() throws IOException { expectThrows(IllegalArgumentException.class, () -> builder.precision(-1)); expectThrows(IllegalArgumentException.class, () -> builder.precision(30)); - // only consider bounding boxes that are at least GEOHASH_TOLERANCE wide and have quantized coordinates - GeoBoundingBox bbox = randomValueOtherThanMany( - (b) -> Math.abs(GeoUtils.normalizeLon(b.right()) - GeoUtils.normalizeLon(b.left())) < GEOHASH_TOLERANCE, - GeoBoundingBoxTests::randomBBox); + GeoBoundingBox bbox = randomBBox(); + final double boundsTop = bbox.top(); + final double boundsBottom = bbox.bottom(); + final double boundsWestLeft; + final double boundsWestRight; + final double boundsEastLeft; + final double boundsEastRight; + final boolean crossesDateline; + if (bbox.right() < bbox.left()) { + boundsWestLeft = -180; + boundsWestRight = bbox.right(); + boundsEastLeft = bbox.left(); + boundsEastRight = 180; + crossesDateline = true; + } else { // only set east bounds + boundsEastLeft = bbox.left(); + boundsEastRight = bbox.right(); + boundsWestLeft = 0; + boundsWestRight = 0; + crossesDateline = false; + } + Function encodeDecodeLat = (lat) -> GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(lat)); Function encodeDecodeLon = (lon) -> GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(lon)); - bbox.topLeft().reset(encodeDecodeLat.apply(bbox.top()), encodeDecodeLon.apply(bbox.left())); - bbox.bottomRight().reset(encodeDecodeLat.apply(bbox.bottom()), encodeDecodeLon.apply(bbox.right())); - - int in = 0, out = 0; + final int precision = randomPrecision(); + int in = 0; List docs = new ArrayList<>(); - while (in + out < numDocs) { - if (bbox.left() > bbox.right()) { - if (randomBoolean()) { - double lonWithin = randomBoolean() ? - 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 = 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(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 = 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)); - } + for (int i = 0; i < numDocs; i++) { + Point p = randomPoint(); + double x = encodeDecodeLon.apply(p.getLon()); + double y = encodeDecodeLat.apply(p.getLat()); + Rectangle pointTile = getTile(x, y, precision); + boolean intersectsBounds = boundsTop > pointTile.getMinY() && boundsBottom < pointTile.getMaxY() + && (boundsEastLeft < pointTile.getMaxX() && boundsEastRight > pointTile.getMinX() + || (crossesDateline && boundsWestLeft < pointTile.getMaxX() && boundsWestRight > pointTile.getMinX())); + if (intersectsBounds) { + in++; } - + docs.add(new LatLonDocValuesField(FIELD_NAME, p.getLat(), p.getLon())); } final long numDocsInBucket = in; - final int precision = randomPrecision(); - 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(); + if (numDocsInBucket > 0) { + 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)); + } else { + assertFalse(AggregationInspectionHelper.hasValue(geoGrid)); } - assertThat(docCount, equalTo(numDocsInBucket)); }, iw -> { for (LatLonDocValuesField docField : docs) { iw.addDocument(Collections.singletonList(docField)); diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java index a58a69cc4aae7..d2e99a43751c0 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/geogrid/GeoHashGridAggregatorTests.java @@ -8,6 +8,13 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.geometry.Point; +import org.elasticsearch.geometry.Rectangle; +import org.elasticsearch.geometry.utils.Geohash; + import static org.elasticsearch.geometry.utils.Geohash.stringEncode; public class GeoHashGridAggregatorTests extends GeoGridAggregatorTestCase { @@ -17,6 +24,23 @@ protected int randomPrecision() { return randomIntBetween(1, 12); } + @Override + protected Point randomPoint() { + return GeometryTestUtils.randomPoint(false); + } + + @Override + protected GeoBoundingBox randomBBox() { + Rectangle rectangle = GeometryTestUtils.randomRectangle(); + return new GeoBoundingBox(new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), + new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())); + } + + @Override + protected Rectangle getTile(double lng, double lat, int precision) { + return Geohash.toBoundingBox(stringEncode(lng, lat, precision)); + } + @Override protected String hashAsString(double lng, double lat, int precision) { return stringEncode(lng, lat, precision); 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 d1dfefbb2173c..4ceb0d38c9447 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 @@ -8,6 +8,14 @@ package org.elasticsearch.search.aggregations.bucket.geogrid; +import org.apache.lucene.geo.GeoEncodingUtils; +import org.elasticsearch.common.geo.GeoBoundingBox; +import org.elasticsearch.common.geo.GeoPoint; +import org.elasticsearch.common.geo.GeoUtils; +import org.elasticsearch.geo.GeometryTestUtils; +import org.elasticsearch.geometry.Point; +import org.elasticsearch.geometry.Rectangle; + public class GeoTileGridAggregatorTests extends GeoGridAggregatorTestCase { @Override @@ -20,6 +28,44 @@ protected String hashAsString(double lng, double lat, int precision) { return GeoTileUtils.stringEncode(GeoTileUtils.longEncode(lng, lat, precision)); } + @Override + protected Point randomPoint() { + return new Point(randomDoubleBetween(GeoUtils.MIN_LON, GeoUtils.MAX_LON, true), + randomDoubleBetween(-GeoTileUtils.LATITUDE_MASK, GeoTileUtils.LATITUDE_MASK, false)); + } + + @Override + protected GeoBoundingBox randomBBox() { + GeoBoundingBox bbox = randomValueOtherThanMany( + (b) -> b.top() > GeoTileUtils.LATITUDE_MASK || b.bottom() < -GeoTileUtils.LATITUDE_MASK, + () -> { + Rectangle rectangle = GeometryTestUtils.randomRectangle(); + return new GeoBoundingBox(new GeoPoint(rectangle.getMaxLat(), rectangle.getMinLon()), + new GeoPoint(rectangle.getMinLat(), rectangle.getMaxLon())); + }); + // Avoid numerical errors for sub-atomic values + double left = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(bbox.left())); + double right = GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(bbox.right())); + double top = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(bbox.top())); + double bottom = GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(bbox.bottom())); + bbox.topLeft().reset(top, left); + bbox.bottomRight().reset(bottom, right); + return bbox; + } + + @Override + protected Rectangle getTile(double lng, double lat, int precision) { + long tiles = 1 << precision; + int x = GeoTileUtils.getXTile(lng, tiles); + int y = GeoTileUtils.getYTile(lat, tiles); + Rectangle r1 = GeoTileUtils.toBoundingBox(x, y, precision); + Rectangle r2 = GeoTileUtils.toBoundingBox(GeoTileUtils.longEncode(lng, lat, precision)); + if (r1.equals(r2) == false) { + int a =0; + } + return GeoTileUtils.toBoundingBox(GeoTileUtils.longEncode(lng, lat, precision)); + } + @Override protected GeoGridAggregationBuilder createBuilder(String name) { return new GeoTileGridAggregationBuilder(name);