From b39f18b2de66bc79e4099020648b2cf01bdcb0c3 Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Thu, 26 Jan 2023 12:16:16 -0800 Subject: [PATCH 1/4] Add GeoTile and GeoHash Grid aggregations on GeoShapes. (#5589) Src files for GeoTile and GeoHash Aggregations on GeoShape with integration tests. Signed-off-by: Navneet Verma --- CHANGELOG.md | 2 +- modules/geo/build.gradle | 2 +- .../geo/GeoModulePluginIntegTestCase.java | 4 + ...AbstractGeoBucketAggregationIntegTest.java | 254 ++++++++++++++++++ .../aggregations/bucket/GeoTileGridIT.java | 164 +++++++++++ ...ractGeoAggregatorModulePluginTestCase.java | 4 - .../org/opensearch/geo/GeoModulePlugin.java | 26 +- .../GeoTileGridValuesSourceBuilder.java | 2 +- .../geogrid}/GeoGridAggregatorSupplier.java | 3 +- .../GeoHashGridAggregationBuilder.java | 1 - .../geogrid/GeoHashGridAggregatorFactory.java | 40 +++ .../GeoTileGridAggregationBuilder.java | 1 - .../geogrid/GeoTileGridAggregatorFactory.java | 39 +++ .../{ => cells}/BoundedCellValues.java | 2 +- .../geogrid/{ => cells}/CellIdSource.java | 2 +- .../geogrid/{ => cells}/CellValues.java | 2 +- .../geogrid/cells/GeoShapeCellIdSource.java | 107 ++++++++ .../geogrid/cells/GeoShapeCellValues.java | 136 ++++++++++ .../{ => cells}/UnboundedCellValues.java | 2 +- .../bucket/geogrid/cells/package-info.java | 12 + .../bucket/geogrid/util/GeoShapeHashUtil.java | 67 +++++ .../metrics/GeoBoundsAggregatorFactory.java | 1 + .../geo/tests/common/RandomGeoGenerator.java | 4 +- .../common/RandomGeoGeometryGenerator.java | 20 ++ .../common/geo/GeoShapeDocValue.java | 25 ++ .../aggregations/bucket/GeoTileUtils.java | 44 +++ 26 files changed, 925 insertions(+), 41 deletions(-) create mode 100644 modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java create mode 100644 modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java rename modules/geo/src/main/java/org/opensearch/geo/search/aggregations/{metrics => bucket/geogrid}/GeoGridAggregatorSupplier.java (93%) rename modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/{ => cells}/BoundedCellValues.java (96%) rename modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/{ => cells}/CellIdSource.java (98%) rename modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/{ => cells}/CellValues.java (97%) create mode 100644 modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellIdSource.java create mode 100644 modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java rename modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/{ => cells}/UnboundedCellValues.java (96%) create mode 100644 modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/package-info.java create mode 100644 modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/util/GeoShapeHashUtil.java diff --git a/CHANGELOG.md b/CHANGELOG.md index 47a26e09349f8..e6dae6a61202e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Addition of Doc values on the GeoShape Field - Addition of GeoShape ValueSource level code interfaces for accessing the DocValues. - Addition of Missing Value feature in the GeoShape Aggregations. - +- Add GeoTile and GeoHash Grid aggregations on GeoShapes. ([#5589](https://github.com/opensearch-project/OpenSearch/pull/5589)) ### Dependencies - Bump `com.azure:azure-storage-common` from 12.21.0 to 12.21.1 (#7566, #7814) - Bump `com.google.guava:guava` from 30.1.1-jre to 32.0.0-jre (#7565, #7811, #7807, #7808) diff --git a/modules/geo/build.gradle b/modules/geo/build.gradle index 6b00709f08bf9..7ab6f80b65ca2 100644 --- a/modules/geo/build.gradle +++ b/modules/geo/build.gradle @@ -31,7 +31,7 @@ apply plugin: 'opensearch.yaml-rest-test' apply plugin: 'opensearch.internal-cluster-test' opensearchplugin { - description 'Plugin for geospatial features in OpenSearch. Registering the geo_shape and aggregations GeoBounds on Geo_Shape and Geo_Point' + description 'Plugin for geospatial features in OpenSearch. Registering the geo_shape and aggregations on GeoShape and GeoPoint' classname 'org.opensearch.geo.GeoModulePlugin' } diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/GeoModulePluginIntegTestCase.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/GeoModulePluginIntegTestCase.java index 31ff2ef4689bd..b17f4804d4d50 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/GeoModulePluginIntegTestCase.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/GeoModulePluginIntegTestCase.java @@ -8,6 +8,8 @@ package org.opensearch.geo; +import org.opensearch.geometry.utils.StandardValidator; +import org.opensearch.geometry.utils.WellKnownText; import org.opensearch.index.mapper.GeoShapeFieldMapper; import org.opensearch.plugins.Plugin; import org.opensearch.test.OpenSearchIntegTestCase; @@ -24,6 +26,8 @@ public abstract class GeoModulePluginIntegTestCase extends OpenSearchIntegTestCa protected static final double GEOHASH_TOLERANCE = 1E-5D; + protected static final WellKnownText WKT = new WellKnownText(true, new StandardValidator(true)); + /** * Returns a collection of plugins that should be loaded on each node for doing the integration tests. As this * geo plugin is not getting packaged in a zip, we need to load it before the tests run. diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java new file mode 100644 index 0000000000000..70dd06fea23ba --- /dev/null +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java @@ -0,0 +1,254 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.geo.search.aggregations.bucket; + +import com.carrotsearch.hppc.ObjectIntHashMap; +import com.carrotsearch.hppc.ObjectIntMap; +import org.opensearch.Version; +import org.opensearch.action.index.IndexRequestBuilder; +import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.geo.GeoPoint; +import org.opensearch.common.geo.GeoShapeDocValue; +import org.opensearch.common.settings.Settings; +import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.geo.GeoModulePluginIntegTestCase; +import org.opensearch.geo.tests.common.RandomGeoGenerator; +import org.opensearch.geo.tests.common.RandomGeoGeometryGenerator; +import org.opensearch.geometry.Geometry; +import org.opensearch.geometry.Rectangle; +import org.opensearch.test.VersionUtils; + +import java.util.ArrayList; +import java.util.HashSet; +import java.util.List; +import java.util.Random; +import java.util.Set; + +import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; + +/** + * This is the base class for all the Bucket Aggregation related integration tests. Use this class to add common + * methods which can be used across different bucket aggregations. If there is any common code that can be used + * across other integration test too then this is not the class. Use {@link GeoModulePluginIntegTestCase} + */ +public abstract class AbstractGeoBucketAggregationIntegTest extends GeoModulePluginIntegTestCase { + + protected static final int MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING = 4; + + protected static final int NUM_DOCS = 100; + + protected static final String GEO_SHAPE_INDEX_NAME = "geoshape_index"; + + protected static Rectangle boundingRectangleForGeoShapesAgg; + + protected static ObjectIntMap expectedDocsCountForGeoShapes; + + protected static ObjectIntMap expectedDocCountsForSingleGeoPoint; + + protected static ObjectIntMap multiValuedExpectedDocCountsGeoPoint; + + protected static final String GEO_SHAPE_FIELD_NAME = "location_geo_shape"; + + protected static final String GEO_POINT_FIELD_NAME = "location"; + + protected static final String KEYWORD_FIELD_NAME = "city"; + + protected static String smallestGeoHash = null; + + protected final Version version = VersionUtils.randomIndexCompatibleVersion(random()); + + @Override + protected boolean forbidPrivateIndexSettings() { + return false; + } + + /** + * Prepares a GeoShape index for testing the GeoShape bucket aggregations. Different bucket aggregations can use + * different techniques for creating buckets. Override the method + * {@link AbstractGeoBucketAggregationIntegTest#generateBucketsForGeometry} in the test class for creating the + * buckets which will then be used for verifications. + * + * @param random {@link Random} + * @throws Exception thrown during index creation. + */ + protected void prepareGeoShapeIndexForAggregations(final Random random) throws Exception { + expectedDocsCountForGeoShapes = new ObjectIntHashMap<>(); + final Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, version).build(); + final List geoshapes = new ArrayList<>(); + assertAcked(prepareCreate(GEO_SHAPE_INDEX_NAME).setSettings(settings).setMapping(GEO_SHAPE_FIELD_NAME, "type" + "=geo_shape")); + boolean isShapeIntersectingBB = false; + for (int i = 0; i < NUM_DOCS;) { + final Geometry geometry = RandomGeoGeometryGenerator.randomGeometry(random); + final GeoShapeDocValue geometryDocValue = GeoShapeDocValue.createGeometryDocValue(geometry); + // make sure that there is 1 shape is intersecting with the bounding box + if (!isShapeIntersectingBB) { + isShapeIntersectingBB = geometryDocValue.isIntersectingRectangle(boundingRectangleForGeoShapesAgg); + if (!isShapeIntersectingBB && i == NUM_DOCS - 1) { + continue; + } + } + i++; + final Set values = generateBucketsForGeometry(geometry, geometryDocValue); + geoshapes.add(indexGeoShape(GEO_SHAPE_INDEX_NAME, geometry)); + for (final String hash : values) { + expectedDocsCountForGeoShapes.put(hash, expectedDocsCountForGeoShapes.getOrDefault(hash, 0) + 1); + } + } + indexRandom(true, geoshapes); + ensureGreen(GEO_SHAPE_INDEX_NAME); + } + + /** + * Returns a set of buckets for the shape at different precision level. Override this method for different bucket + * aggregations. + * + * @param geometry {@link Geometry} + * @param geoShapeDocValue {@link GeoShapeDocValue} + * @return A {@link Set} of {@link String} which represents the buckets. + */ + protected abstract Set generateBucketsForGeometry(final Geometry geometry, final GeoShapeDocValue geoShapeDocValue); + + /** + * Prepares a GeoPoint index for testing the GeoPoint bucket aggregations. Different bucket aggregations can use + * different techniques for creating buckets. Override the method + * {@link AbstractGeoBucketAggregationIntegTest#generateBucketsForGeoPoint} in the test class for creating the + * buckets which will then be used for verifications. + * + * @param random {@link Random} + * @throws Exception thrown during index creation. + */ + protected void prepareSingleValueGeoPointIndex(final Random random) throws Exception { + expectedDocCountsForSingleGeoPoint = new ObjectIntHashMap<>(); + createIndex("idx_unmapped"); + final Settings settings = Settings.builder() + .put(IndexMetadata.SETTING_VERSION_CREATED, version) + .put("index.number_of_shards", 4) + .put("index.number_of_replicas", 0) + .build(); + assertAcked( + prepareCreate("idx").setSettings(settings) + .setMapping(GEO_POINT_FIELD_NAME, "type=geo_point", KEYWORD_FIELD_NAME, "type=keyword") + ); + final List cities = new ArrayList<>(); + for (int i = 0; i < NUM_DOCS; i++) { + // generate random point + final GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random); + cities.add(indexGeoPoint("idx", geoPoint.toString(), geoPoint.getLat() + ", " + geoPoint.getLon())); + final Set buckets = generateBucketsForGeoPoint(geoPoint); + for (final String bucket : buckets) { + expectedDocCountsForSingleGeoPoint.put(bucket, expectedDocCountsForSingleGeoPoint.getOrDefault(bucket, 0) + 1); + } + } + indexRandom(true, cities); + ensureGreen("idx_unmapped", "idx"); + } + + protected void prepareMultiValuedGeoPointIndex(final Random random) throws Exception { + multiValuedExpectedDocCountsGeoPoint = new ObjectIntHashMap<>(); + final Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, version).build(); + final List cities = new ArrayList<>(); + assertAcked( + prepareCreate("multi_valued_idx").setSettings(settings) + .setMapping(GEO_POINT_FIELD_NAME, "type=geo_point", KEYWORD_FIELD_NAME, "type=keyword") + ); + for (int i = 0; i < NUM_DOCS; i++) { + final int numPoints = random.nextInt(4); + final List points = new ArrayList<>(); + final Set buckets = new HashSet<>(); + for (int j = 0; j < numPoints; ++j) { + // generate random point + final GeoPoint geoPoint = RandomGeoGenerator.randomPoint(random); + points.add(geoPoint.getLat() + "," + geoPoint.getLon()); + buckets.addAll(generateBucketsForGeoPoint(geoPoint)); + } + cities.add(indexGeoPoints("multi_valued_idx", Integer.toString(i), points)); + for (final String bucket : buckets) { + multiValuedExpectedDocCountsGeoPoint.put(bucket, multiValuedExpectedDocCountsGeoPoint.getOrDefault(bucket, 0) + 1); + } + } + indexRandom(true, cities); + ensureGreen("multi_valued_idx"); + } + + /** + * Returns a set of buckets for the GeoPoint at different precision level. Override this method for different bucket + * aggregations. + * + * @param geoPoint {@link GeoPoint} + * @return A {@link Set} of {@link String} which represents the buckets. + */ + protected abstract Set generateBucketsForGeoPoint(final GeoPoint geoPoint); + + /** + * Indexes a GeoShape in the provided index. + * @param index {@link String} index name + * @param geometry {@link Geometry} the Geometry to be indexed + * @return {@link IndexRequestBuilder} + * @throws Exception thrown during creation of {@link IndexRequestBuilder} + */ + protected IndexRequestBuilder indexGeoShape(final String index, final Geometry geometry) throws Exception { + XContentBuilder source = jsonBuilder().startObject(); + source = source.field(GEO_SHAPE_FIELD_NAME, WKT.toWKT(geometry)); + source = source.endObject(); + return client().prepareIndex(index).setSource(source); + } + + /** + * Indexes a {@link List} of {@link GeoPoint}s in the provided Index name. + * @param index {@link String} index name + * @param name {@link String} value for the string field in index + * @param latLon {@link List} of {@link String} representing the String representation of GeoPoint + * @return {@link IndexRequestBuilder} + * @throws Exception thrown during indexing. + */ + protected IndexRequestBuilder indexGeoPoints(final String index, final String name, final List latLon) throws Exception { + XContentBuilder source = jsonBuilder().startObject().field(KEYWORD_FIELD_NAME, name); + if (latLon != null) { + source = source.field(GEO_POINT_FIELD_NAME, latLon); + } + source = source.endObject(); + return client().prepareIndex(index).setSource(source); + } + + /** + * Indexes a {@link GeoPoint} in the provided Index name. + * @param index {@link String} index name + * @param name {@link String} value for the string field in index + * @param latLon {@link String} representing the String representation of GeoPoint + * @return {@link IndexRequestBuilder} + * @throws Exception thrown during indexing. + */ + protected IndexRequestBuilder indexGeoPoint(final String index, final String name, final String latLon) throws Exception { + return indexGeoPoints(index, name, List.of(latLon)); + } + + /** + * Generates a Bounding Box of a fixed radius that can be used for shapes aggregations to reduce the size of + * aggregation results. + * @param random {@link Random} + * @return {@link Rectangle} + */ + protected Rectangle getGridAggregationBoundingBox(final Random random) { + final double radius = getRadiusOfBoundingBox(); + assertTrue("The radius of Bounding Box is less than or equal to 0", radius > 0); + return RandomGeoGeometryGenerator.randomRectangle(random, radius); + } + + /** + * Returns a radius for the Bounding box. Test classes can override this method to change the radius of BBox for + * the test cases. If we increase this value, it will lead to creation of a lot of buckets that can lead of + * IndexOutOfBoundsExceptions. + * @return double + */ + protected double getRadiusOfBoundingBox() { + return 5.0; + } + +} diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java new file mode 100644 index 0000000000000..6198c4cef3a34 --- /dev/null +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java @@ -0,0 +1,164 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.geo.search.aggregations.bucket; + +import org.hamcrest.MatcherAssert; +import org.opensearch.action.search.SearchResponse; +import org.opensearch.common.geo.GeoBoundingBox; +import org.opensearch.common.geo.GeoPoint; +import org.opensearch.common.geo.GeoShapeDocValue; +import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoGrid; +import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; +import org.opensearch.geo.search.aggregations.common.GeoBoundsHelper; +import org.opensearch.geo.tests.common.AggregationBuilders; +import org.opensearch.geometry.Geometry; +import org.opensearch.search.aggregations.InternalAggregation; +import org.opensearch.search.aggregations.bucket.GeoTileUtils; +import org.opensearch.test.OpenSearchIntegTestCase; + +import java.util.HashSet; +import java.util.List; +import java.util.Random; +import java.util.Set; +import java.util.stream.Collectors; + +import static org.hamcrest.Matchers.equalTo; +import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse; + +@OpenSearchIntegTestCase.SuiteScopeTestCase +public class GeoTileGridIT extends AbstractGeoBucketAggregationIntegTest { + + private static final int GEOPOINT_MAX_PRECISION = 17; + + private static final String AGG_NAME = "geotilegrid"; + + @Override + public void setupSuiteScopeCluster() throws Exception { + final Random random = random(); + // Creating a BB for limiting the number buckets generated during aggregation + boundingRectangleForGeoShapesAgg = getGridAggregationBoundingBox(random); + prepareSingleValueGeoPointIndex(random); + prepareMultiValuedGeoPointIndex(random); + prepareGeoShapeIndexForAggregations(random); + ensureSearchable(); + } + + public void testGeoShapes() { + final GeoBoundingBox boundingBox = new GeoBoundingBox( + new GeoPoint(boundingRectangleForGeoShapesAgg.getMaxLat(), boundingRectangleForGeoShapesAgg.getMinLon()), + new GeoPoint(boundingRectangleForGeoShapesAgg.getMinLat(), boundingRectangleForGeoShapesAgg.getMaxLon()) + ); + for (int precision = 1; precision <= MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING; precision++) { + final GeoGridAggregationBuilder builder = AggregationBuilders.geotileGrid(AGG_NAME) + .field(GEO_SHAPE_FIELD_NAME) + .precision(precision); + // This makes sure that for only higher precision we are providing the GeoBounding Box. This also ensures + // that we are able to test both bounded and unbounded aggregations + if (precision > 2) { + builder.setGeoBoundingBox(boundingBox); + } + final SearchResponse response = client().prepareSearch(GEO_SHAPE_INDEX_NAME).addAggregation(builder).get(); + final GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); + final List buckets = geoGrid.getBuckets(); + final Object[] propertiesKeys = (Object[]) ((InternalAggregation) geoGrid).getProperty("_key"); + final Object[] propertiesDocCounts = (Object[]) ((InternalAggregation) geoGrid).getProperty("_count"); + for (int i = 0; i < buckets.size(); i++) { + final GeoGrid.Bucket cell = buckets.get(i); + final String geoTile = cell.getKeyAsString(); + + final long bucketCount = cell.getDocCount(); + final int expectedBucketCount = expectedDocsCountForGeoShapes.get(geoTile); + assertNotSame(bucketCount, 0); + assertEquals("Geotile " + geoTile + " has wrong doc count ", expectedBucketCount, bucketCount); + final GeoPoint geoPoint = (GeoPoint) propertiesKeys[i]; + MatcherAssert.assertThat(GeoTileUtils.stringEncode(geoPoint.lon(), geoPoint.lat(), precision), equalTo(geoTile)); + MatcherAssert.assertThat((long) propertiesDocCounts[i], equalTo(bucketCount)); + } + } + } + + public void testSimpleGeoPointsAggregation() { + for (int precision = 1; precision <= GEOPOINT_MAX_PRECISION; precision++) { + SearchResponse response = client().prepareSearch("idx") + .addAggregation(AggregationBuilders.geotileGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).precision(precision)) + .get(); + + assertSearchResponse(response); + + GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); + List buckets = geoGrid.getBuckets(); + Object[] propertiesKeys = (Object[]) ((InternalAggregation) geoGrid).getProperty("_key"); + Object[] propertiesDocCounts = (Object[]) ((InternalAggregation) geoGrid).getProperty("_count"); + for (int i = 0; i < buckets.size(); i++) { + GeoGrid.Bucket cell = buckets.get(i); + String geoTile = cell.getKeyAsString(); + + long bucketCount = cell.getDocCount(); + int expectedBucketCount = expectedDocCountsForSingleGeoPoint.get(geoTile); + assertNotSame(bucketCount, 0); + assertEquals("GeoTile " + geoTile + " has wrong doc count ", expectedBucketCount, bucketCount); + GeoPoint geoPoint = (GeoPoint) propertiesKeys[i]; + assertThat(GeoTileUtils.stringEncode(geoPoint.lon(), geoPoint.lat(), precision), equalTo(geoTile)); + assertThat((long) propertiesDocCounts[i], equalTo(bucketCount)); + } + } + } + + public void testMultivaluedGeoPointsAggregation() throws Exception { + for (int precision = 1; precision <= GEOPOINT_MAX_PRECISION; precision++) { + SearchResponse response = client().prepareSearch("multi_valued_idx") + .addAggregation(AggregationBuilders.geotileGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).precision(precision)) + .get(); + + assertSearchResponse(response); + + GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); + for (GeoGrid.Bucket cell : geoGrid.getBuckets()) { + String geohash = cell.getKeyAsString(); + + long bucketCount = cell.getDocCount(); + int expectedBucketCount = multiValuedExpectedDocCountsGeoPoint.get(geohash); + assertNotSame(bucketCount, 0); + assertEquals("Geohash " + geohash + " has wrong doc count ", expectedBucketCount, bucketCount); + } + } + } + + /** + * Returns a set of buckets for the shape at different precision level. Override this method for different bucket + * aggregations. + * + * @param geometry {@link Geometry} + * @param geoShapeDocValue {@link GeoShapeDocValue} + * @return A {@link Set} of {@link String} which represents the buckets. + */ + @Override + protected Set generateBucketsForGeometry(Geometry geometry, GeoShapeDocValue geoShapeDocValue) { + final GeoPoint topLeft = new GeoPoint(); + final GeoPoint bottomRight = new GeoPoint(); + assert geometry != null; + GeoBoundsHelper.updateBoundsForGeometry(geometry, topLeft, bottomRight); + final Set geoTiles = new HashSet<>(); + for (int precision = MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING; precision > 0; precision--) { + geoTiles.addAll( + GeoTileUtils.encodeShape(geoShapeDocValue, precision).stream().map(GeoTileUtils::stringEncode).collect(Collectors.toSet()) + ); + } + return geoTiles; + } + + protected Set generateBucketsForGeoPoint(final GeoPoint geoPoint) { + Set buckets = new HashSet<>(); + for (int precision = GEOPOINT_MAX_PRECISION; precision > 0; precision--) { + final String tile = GeoTileUtils.stringEncode(geoPoint.getLon(), geoPoint.getLat(), precision); + buckets.add(tile); + } + return buckets; + } +} diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/metrics/AbstractGeoAggregatorModulePluginTestCase.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/metrics/AbstractGeoAggregatorModulePluginTestCase.java index 3ccb62d40cbe3..03ed2ea6d1e3b 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/metrics/AbstractGeoAggregatorModulePluginTestCase.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/metrics/AbstractGeoAggregatorModulePluginTestCase.java @@ -27,8 +27,6 @@ import org.opensearch.geo.tests.common.RandomGeoGeometryGenerator; import org.opensearch.geometry.Geometry; import org.opensearch.geometry.utils.Geohash; -import org.opensearch.geometry.utils.StandardValidator; -import org.opensearch.geometry.utils.WellKnownText; import org.opensearch.search.SearchHit; import org.opensearch.search.sort.SortBuilders; import org.opensearch.search.sort.SortOrder; @@ -70,8 +68,6 @@ public abstract class AbstractGeoAggregatorModulePluginTestCase extends GeoModul protected static ObjectIntMap expectedDocCountsForGeoHash = null; protected static ObjectObjectMap expectedCentroidsForGeoHash = null; - protected static final WellKnownText WKT = new WellKnownText(true, new StandardValidator(true)); - @Override public void setupSuiteScopeCluster() throws Exception { createIndex(UNMAPPED_IDX_NAME); diff --git a/modules/geo/src/main/java/org/opensearch/geo/GeoModulePlugin.java b/modules/geo/src/main/java/org/opensearch/geo/GeoModulePlugin.java index 77abba7f54677..efee09d01d04e 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/GeoModulePlugin.java +++ b/modules/geo/src/main/java/org/opensearch/geo/GeoModulePlugin.java @@ -40,7 +40,6 @@ import org.opensearch.geo.search.aggregations.bucket.geogrid.InternalGeoTileGrid; import org.opensearch.geo.search.aggregations.metrics.GeoBounds; import org.opensearch.geo.search.aggregations.metrics.GeoBoundsAggregationBuilder; -import org.opensearch.geo.search.aggregations.metrics.GeoBoundsGeoShapeAggregator; import org.opensearch.geo.search.aggregations.metrics.InternalGeoBounds; import org.opensearch.index.mapper.GeoShapeFieldMapper; import org.opensearch.index.mapper.Mapper; @@ -48,13 +47,10 @@ import org.opensearch.plugins.Plugin; import org.opensearch.plugins.SearchPlugin; import org.opensearch.search.aggregations.bucket.composite.CompositeAggregation; -import org.opensearch.search.aggregations.support.CoreValuesSourceType; -import org.opensearch.search.aggregations.support.ValuesSourceRegistry; import java.util.Collections; import java.util.List; import java.util.Map; -import java.util.function.Consumer; public class GeoModulePlugin extends Plugin implements MapperPlugin, SearchPlugin { @@ -64,7 +60,8 @@ public Map getMappers() { } /** - * Registering {@link GeoBounds} aggregation on GeoPoint field. + * Registering {@link GeoBounds}, {@link InternalGeoHashGrid}, {@link InternalGeoTileGrid} aggregation on GeoPoint and GeoShape + * fields. */ @Override public List getAggregations() { @@ -106,23 +103,4 @@ public List getCompositeAggregations() { ) ); } - - /** - * Registering the GeoBounds Aggregation on the GeoShape Field. This function allows plugins to register new - * aggregations using aggregation names that are already defined in Core, as long as the new aggregations target - * different ValuesSourceTypes. - * - * @return A list of the new registrar functions - */ - @Override - public List> getAggregationExtentions() { - final Consumer geoShapeConsumer = builder -> builder.register( - GeoBoundsAggregationBuilder.REGISTRY_KEY, - CoreValuesSourceType.GEO_SHAPE, - GeoBoundsGeoShapeAggregator::new, - true - ); - return Collections.singletonList(geoShapeConsumer); - } - } diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java index d2bf3541b5cce..5ee4a18a4f325 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java @@ -43,7 +43,7 @@ import org.opensearch.core.xcontent.ObjectParser; import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.core.xcontent.XContentParser; -import org.opensearch.geo.search.aggregations.bucket.geogrid.CellIdSource; +import org.opensearch.geo.search.aggregations.bucket.geogrid.cells.CellIdSource; import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder; import org.opensearch.index.mapper.MappedFieldType; import org.opensearch.index.query.QueryShardContext; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/metrics/GeoGridAggregatorSupplier.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoGridAggregatorSupplier.java similarity index 93% rename from modules/geo/src/main/java/org/opensearch/geo/search/aggregations/metrics/GeoGridAggregatorSupplier.java rename to modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoGridAggregatorSupplier.java index 43ccb8b89545a..0ef1957f88ef6 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/metrics/GeoGridAggregatorSupplier.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoGridAggregatorSupplier.java @@ -30,10 +30,9 @@ * GitHub history for details. */ -package org.opensearch.geo.search.aggregations.metrics; +package org.opensearch.geo.search.aggregations.bucket.geogrid; import org.opensearch.common.geo.GeoBoundingBox; -import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoGridAggregator; import org.opensearch.search.aggregations.Aggregator; import org.opensearch.search.aggregations.AggregatorFactories; import org.opensearch.search.aggregations.CardinalityUpperBound; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java index 9631998649272..06fbaac8cdfed 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregationBuilder.java @@ -40,7 +40,6 @@ import org.opensearch.search.aggregations.AggregationBuilder; import org.opensearch.search.aggregations.AggregatorFactories; import org.opensearch.search.aggregations.AggregatorFactory; -import org.opensearch.geo.search.aggregations.metrics.GeoGridAggregatorSupplier; import org.opensearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.opensearch.search.aggregations.support.ValuesSourceConfig; import org.opensearch.search.aggregations.support.ValuesSourceRegistry; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java index 1914c07e831f7..5502e0c418cf4 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoHashGridAggregatorFactory.java @@ -33,6 +33,9 @@ package org.opensearch.geo.search.aggregations.bucket.geogrid; import org.opensearch.common.geo.GeoBoundingBox; +import org.opensearch.geo.search.aggregations.bucket.geogrid.cells.CellIdSource; +import org.opensearch.geo.search.aggregations.bucket.geogrid.cells.GeoShapeCellIdSource; +import org.opensearch.geo.search.aggregations.bucket.geogrid.util.GeoShapeHashUtil; import org.opensearch.geometry.utils.Geohash; import org.opensearch.index.query.QueryShardContext; import org.opensearch.search.aggregations.Aggregator; @@ -120,6 +123,7 @@ protected Aggregator doCreateInternal( } static void registerAggregators(ValuesSourceRegistry.Builder builder) { + // register GeoPoint Aggregation builder.register( GeoHashGridAggregationBuilder.REGISTRY_KEY, CoreValuesSourceType.GEOPOINT, @@ -155,5 +159,41 @@ static void registerAggregators(ValuesSourceRegistry.Builder builder) { }, true ); + // register GeoShape Aggregation + builder.register( + GeoHashGridAggregationBuilder.REGISTRY_KEY, + CoreValuesSourceType.GEO_SHAPE, + ( + name, + factories, + valuesSource, + precision, + geoBoundingBox, + requiredSize, + shardSize, + aggregationContext, + parent, + cardinality, + metadata) -> { + final GeoShapeCellIdSource cellIdSource = new GeoShapeCellIdSource( + (ValuesSource.GeoShape) valuesSource, + precision, + geoBoundingBox, + GeoShapeHashUtil::encodeShape + ); + return new GeoHashGridAggregator( + name, + factories, + cellIdSource, + requiredSize, + shardSize, + aggregationContext, + parent, + cardinality, + metadata + ); + }, + true + ); } } diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java index a07f3b438dc7a..10aa07a6712ee 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregationBuilder.java @@ -39,7 +39,6 @@ import org.opensearch.search.aggregations.AggregationBuilder; import org.opensearch.search.aggregations.AggregatorFactories; import org.opensearch.search.aggregations.AggregatorFactory; -import org.opensearch.geo.search.aggregations.metrics.GeoGridAggregatorSupplier; import org.opensearch.search.aggregations.bucket.GeoTileUtils; import org.opensearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.opensearch.search.aggregations.support.ValuesSourceConfig; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java index b830988a3d410..b8e3efbb891df 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/GeoTileGridAggregatorFactory.java @@ -33,6 +33,8 @@ package org.opensearch.geo.search.aggregations.bucket.geogrid; import org.opensearch.common.geo.GeoBoundingBox; +import org.opensearch.geo.search.aggregations.bucket.geogrid.cells.CellIdSource; +import org.opensearch.geo.search.aggregations.bucket.geogrid.cells.GeoShapeCellIdSource; import org.opensearch.index.query.QueryShardContext; import org.opensearch.search.aggregations.Aggregator; import org.opensearch.search.aggregations.AggregatorFactories; @@ -154,5 +156,42 @@ static void registerAggregators(ValuesSourceRegistry.Builder builder) { }, true ); + + // registers Aggregation on GeoShape + builder.register( + GeoTileGridAggregationBuilder.REGISTRY_KEY, + CoreValuesSourceType.GEO_SHAPE, + ( + name, + factories, + valuesSource, + precision, + geoBoundingBox, + requiredSize, + shardSize, + aggregationContext, + parent, + cardinality, + metadata) -> { + GeoShapeCellIdSource cellIdSource = new GeoShapeCellIdSource( + (ValuesSource.GeoShape) valuesSource, + precision, + geoBoundingBox, + GeoTileUtils::encodeShape + ); + return new GeoTileGridAggregator( + name, + factories, + cellIdSource, + requiredSize, + shardSize, + aggregationContext, + parent, + cardinality, + metadata + ); + }, + true + ); } } diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/BoundedCellValues.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/BoundedCellValues.java similarity index 96% rename from modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/BoundedCellValues.java rename to modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/BoundedCellValues.java index 06d2dcaee3932..588c8bc59c2e0 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/BoundedCellValues.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/BoundedCellValues.java @@ -29,7 +29,7 @@ * GitHub history for details. */ -package org.opensearch.geo.search.aggregations.bucket.geogrid; +package org.opensearch.geo.search.aggregations.bucket.geogrid.cells; import org.opensearch.common.geo.GeoBoundingBox; import org.opensearch.index.fielddata.MultiGeoPointValues; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/CellIdSource.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/CellIdSource.java similarity index 98% rename from modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/CellIdSource.java rename to modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/CellIdSource.java index cec49a867d660..42c4722e065af 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/CellIdSource.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/CellIdSource.java @@ -29,7 +29,7 @@ * GitHub history for details. */ -package org.opensearch.geo.search.aggregations.bucket.geogrid; +package org.opensearch.geo.search.aggregations.bucket.geogrid.cells; import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.SortedNumericDocValues; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/CellValues.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/CellValues.java similarity index 97% rename from modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/CellValues.java rename to modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/CellValues.java index d01896c8136fa..0b69040ec977a 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/CellValues.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/CellValues.java @@ -29,7 +29,7 @@ * GitHub history for details. */ -package org.opensearch.geo.search.aggregations.bucket.geogrid; +package org.opensearch.geo.search.aggregations.bucket.geogrid.cells; import org.opensearch.index.fielddata.AbstractSortingNumericDocValues; import org.opensearch.index.fielddata.MultiGeoPointValues; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellIdSource.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellIdSource.java new file mode 100644 index 0000000000000..0ea4d96c450ec --- /dev/null +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellIdSource.java @@ -0,0 +1,107 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.geo.search.aggregations.bucket.geogrid.cells; + +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.SortedNumericDocValues; +import org.opensearch.common.geo.GeoBoundingBox; +import org.opensearch.common.geo.GeoShapeDocValue; +import org.opensearch.index.fielddata.GeoShapeValue; +import org.opensearch.index.fielddata.SortedBinaryDocValues; +import org.opensearch.index.fielddata.SortedNumericDoubleValues; +import org.opensearch.search.aggregations.support.ValuesSource; + +import java.io.IOException; +import java.util.List; + +/** + * ValueSource class which converts the {@link GeoShapeValue} to numeric long values for bucketing. This class uses the + * {@link GeoShapeCellIdSource.GeoShapeLongEncoder} to encode the geo_shape to {@link Long} values which can be iterated + * to do the bucket aggregation. + * + * @opensearch.internal + */ +public class GeoShapeCellIdSource extends ValuesSource.Numeric { + + private final ValuesSource.GeoShape geoShape; + private final int precision; + private final GeoBoundingBox geoBoundingBox; + private final GeoShapeCellIdSource.GeoShapeLongEncoder encoder; + + public GeoShapeCellIdSource( + final ValuesSource.GeoShape geoShape, + final int precision, + final GeoBoundingBox geoBoundingBox, + final GeoShapeCellIdSource.GeoShapeLongEncoder encoder + ) { + this.geoShape = geoShape; + this.geoBoundingBox = geoBoundingBox; + this.precision = precision; + this.encoder = encoder; + } + + /** + * Get the current {@link SortedBinaryDocValues}. + * + * @param context {@link LeafReaderContext} + */ + @Override + public SortedBinaryDocValues bytesValues(LeafReaderContext context) throws IOException { + throw new UnsupportedOperationException("The bytesValues operation is not supported on GeoShapeCellIdSource"); + } + + /** + * Whether the underlying data is floating-point or not. + */ + @Override + public boolean isFloatingPoint() { + return false; + } + + /** + * Whether the underlying data is big integer or not. + */ + @Override + public boolean isBigInteger() { + return false; + } + + /** + * Get the current {@link SortedNumericDocValues}. + * + * @param context {@link LeafReaderContext} + */ + @Override + public SortedNumericDocValues longValues(final LeafReaderContext context) { + if (geoBoundingBox.isUnbounded()) { + return new GeoShapeCellValues.UnboundedCellValues(geoShape.getGeoShapeValues(context), precision, encoder); + } + return new GeoShapeCellValues.BoundedCellValues(geoShape.getGeoShapeValues(context), precision, encoder, geoBoundingBox); + } + + /** + * Get the current {@link SortedNumericDoubleValues}. + * + * @param context {@link LeafReaderContext} + */ + @Override + public SortedNumericDoubleValues doubleValues(LeafReaderContext context) { + throw new UnsupportedOperationException("The doubleValues operation is not supported on GeoShapeCellIdSource"); + } + + /** + * Encoder to encode the GeoShapes to the specific long values for the aggregation. + * + * @opensearch.internal + */ + @FunctionalInterface + public interface GeoShapeLongEncoder { + List encode(final GeoShapeDocValue geoShapeDocValue, final int precision); + } +} diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java new file mode 100644 index 0000000000000..4911818cd448f --- /dev/null +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java @@ -0,0 +1,136 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.geo.search.aggregations.bucket.geogrid.cells; + +import org.opensearch.common.geo.GeoBoundingBox; +import org.opensearch.common.geo.GeoShapeDocValue; +import org.opensearch.index.fielddata.AbstractSortingNumericDocValues; +import org.opensearch.index.fielddata.GeoShapeValue; + +import java.io.IOException; +import java.util.List; + +/** + * Class representing the long-encoded grid-cells belonging to the geoshape-doc-values. Class must encode the values + * as long and then sort them in order to account for the cells correctly. + * + * @opensearch.internal + */ +abstract class GeoShapeCellValues extends AbstractSortingNumericDocValues { + private final GeoShapeValue geoShapeValue; + protected int precision; + protected final GeoShapeCellIdSource.GeoShapeLongEncoder encoder; + + public GeoShapeCellValues(GeoShapeValue geoShapeValue, int precision, GeoShapeCellIdSource.GeoShapeLongEncoder encoder) { + this.geoShapeValue = geoShapeValue; + this.precision = precision; + this.encoder = encoder; + } + + @Override + public boolean advanceExact(int docId) throws IOException { + if (geoShapeValue.advanceExact(docId)) { + final GeoShapeDocValue geoShapeDocValue = geoShapeValue.nextValue(); + relateShape(geoShapeDocValue); + sort(); + return true; + } + return false; + } + + /** + * This function relates the shape's with the grid, and then put the intersecting grid's info as long, which + * can be iterated in the aggregation. It uses the encoder to find the relation. + * + * @param geoShapeDocValue {@link GeoShapeDocValue} + */ + abstract void relateShape(final GeoShapeDocValue geoShapeDocValue); + + /** + * Provides the {@link GeoShapeCellValues} for the input bounding box. + * @opensearch.internal + */ + static class BoundedCellValues extends GeoShapeCellValues { + + private final GeoBoundingBox geoBoundingBox; + + public BoundedCellValues( + final GeoShapeValue geoShapeValue, + final int precision, + final GeoShapeCellIdSource.GeoShapeLongEncoder encoder, + final GeoBoundingBox boundingBox + ) { + super(geoShapeValue, precision, encoder); + this.geoBoundingBox = boundingBox; + } + + /** + * This function relates the shape's with the grid, and then put the intersecting grid's info as long, which + * can be iterated in the aggregation. It uses the encoder to find the relation. + * + * @param geoShapeDocValue {@link GeoShapeDocValue} + */ + @Override + void relateShape(final GeoShapeDocValue geoShapeDocValue) { + if (intersect(geoShapeDocValue.getBoundingRectangle())) { + // now we know the shape is in the bounding rectangle, we need add them in longValues + // generate all grid that this shape intersects + final List encodedValues = encoder.encode(geoShapeDocValue, precision); + resize(encodedValues.size()); + for (int i = 0; i < encodedValues.size(); i++) { + values[i] = encodedValues.get(i); + } + } + } + + /** + * Validate that shape is intersecting the bounding box provided as input. + * + * @param rectangle {@link GeoShapeDocValue.BoundingRectangle} + * @return true or false + */ + private boolean intersect(final GeoShapeDocValue.BoundingRectangle rectangle) { + return geoBoundingBox.pointInBounds(rectangle.getMaxLongitude(), rectangle.getMaxLatitude()) + || geoBoundingBox.pointInBounds(rectangle.getMaxLongitude(), rectangle.getMinLatitude()) + || geoBoundingBox.pointInBounds(rectangle.getMinLongitude(), rectangle.getMaxLatitude()) + || geoBoundingBox.pointInBounds(rectangle.getMinLongitude(), rectangle.getMinLatitude()); + } + + } + + /** + * Provides the {@link GeoShapeCellValues} for unbounded cells + * @opensearch.internal + */ + static class UnboundedCellValues extends GeoShapeCellValues { + + public UnboundedCellValues( + final GeoShapeValue geoShapeValue, + final int precision, + final GeoShapeCellIdSource.GeoShapeLongEncoder encoder + ) { + super(geoShapeValue, precision, encoder); + } + + /** + * This function relates the shape's with the grid, and then put the intersecting grid's info as long, which + * can be iterated in the aggregation. It uses the encoder to find the relation. + * + * @param geoShapeDocValue {@link GeoShapeDocValue} + */ + @Override + void relateShape(final GeoShapeDocValue geoShapeDocValue) { + final List encodedValues = encoder.encode(geoShapeDocValue, precision); + resize(encodedValues.size()); + for (int i = 0; i < encodedValues.size(); i++) { + values[i] = encodedValues.get(i); + } + } + } +} diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/UnboundedCellValues.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/UnboundedCellValues.java similarity index 96% rename from modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/UnboundedCellValues.java rename to modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/UnboundedCellValues.java index c628c7bfdc8ec..0a520c7162002 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/UnboundedCellValues.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/UnboundedCellValues.java @@ -29,7 +29,7 @@ * GitHub history for details. */ -package org.opensearch.geo.search.aggregations.bucket.geogrid; +package org.opensearch.geo.search.aggregations.bucket.geogrid.cells; import org.opensearch.common.geo.GeoBoundingBox; import org.opensearch.index.fielddata.MultiGeoPointValues; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/package-info.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/package-info.java new file mode 100644 index 0000000000000..16a5dd11f6210 --- /dev/null +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/package-info.java @@ -0,0 +1,12 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +/** + * A Cells package which provide the different grid cells related functionalities for different aggregations + */ +package org.opensearch.geo.search.aggregations.bucket.geogrid.cells; diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/util/GeoShapeHashUtil.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/util/GeoShapeHashUtil.java new file mode 100644 index 0000000000000..aefb31e623bb5 --- /dev/null +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/util/GeoShapeHashUtil.java @@ -0,0 +1,67 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.geo.search.aggregations.bucket.geogrid.util; + +import org.opensearch.common.geo.GeoShapeDocValue; +import org.opensearch.geometry.Rectangle; +import org.opensearch.geometry.utils.Geohash; + +import java.util.ArrayList; +import java.util.List; + +/** + * We have a {@link Geohash} class present at the libs level, not using that because while encoding the shapes we need + * {@link GeoShapeDocValue}. This class provided the utilities encode the shape as GeoHashes + */ +public class GeoShapeHashUtil { + + /** + * The function encodes the shape provided as {@link GeoShapeDocValue} to a {@link List} of {@link Long} values + * (representing the GeoHashes) which are intersecting with the shapes at a given precision. + * + * @param geoShapeDocValue {@link GeoShapeDocValue} + * @param precision int + * @return {@link List} containing encoded {@link Long} values + */ + public static List encodeShape(final GeoShapeDocValue geoShapeDocValue, final int precision) { + final List encodedValues = new ArrayList<>(); + final GeoShapeDocValue.BoundingRectangle boundingRectangle = geoShapeDocValue.getBoundingRectangle(); + long topLeftGeoHash = Geohash.longEncode(boundingRectangle.getMinX(), boundingRectangle.getMaxY(), precision); + long topRightGeoHash = Geohash.longEncode(boundingRectangle.getMaxX(), boundingRectangle.getMaxY(), precision); + long bottomRightGeoHash = Geohash.longEncode(boundingRectangle.getMaxX(), boundingRectangle.getMinY(), precision); + + long currentValue = topLeftGeoHash; + long rightMax = topRightGeoHash; + long tempCurrent = currentValue; + while (true) { + // check if this currentValue intersect with shape. + final Rectangle geohashRectangle = Geohash.toBoundingBox(Geohash.stringEncode(tempCurrent)); + if (geoShapeDocValue.isIntersectingRectangle(geohashRectangle)) { + encodedValues.add(tempCurrent); + } + + // Breaking condition + if (tempCurrent == bottomRightGeoHash) { + break; + } + // now change the iterator => tempCurrent + if (tempCurrent == rightMax) { + // move to next row + tempCurrent = Geohash.longEncode(Geohash.getNeighbor(Geohash.stringEncode(currentValue), precision, 0, -1)); + currentValue = tempCurrent; + // update right max + rightMax = Geohash.longEncode(Geohash.getNeighbor(Geohash.stringEncode(rightMax), precision, 0, -1)); + } else { + // move to next column + tempCurrent = Geohash.longEncode(Geohash.getNeighbor(Geohash.stringEncode(tempCurrent), precision, 1, 0)); + } + } + return encodedValues; + } +} diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/metrics/GeoBoundsAggregatorFactory.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/metrics/GeoBoundsAggregatorFactory.java index 149e052b4db7d..780f25ba3d7fb 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/metrics/GeoBoundsAggregatorFactory.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/metrics/GeoBoundsAggregatorFactory.java @@ -87,5 +87,6 @@ protected Aggregator doCreateInternal( static void registerAggregators(ValuesSourceRegistry.Builder builder) { builder.register(GeoBoundsAggregationBuilder.REGISTRY_KEY, CoreValuesSourceType.GEOPOINT, GeoBoundsAggregator::new, true); + builder.register(GeoBoundsAggregationBuilder.REGISTRY_KEY, CoreValuesSourceType.GEO_SHAPE, GeoBoundsGeoShapeAggregator::new, true); } } diff --git a/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGenerator.java b/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGenerator.java index 2fb403155e2bc..a3def686b282d 100644 --- a/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGenerator.java +++ b/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGenerator.java @@ -64,7 +64,7 @@ public static GeoPoint randomPointIn(Random r, final double minLon, final double } /** Puts latitude in range of -90 to 90. */ - private static double normalizeLatitude(double latitude) { + public static double normalizeLatitude(double latitude) { if (latitude >= -90 && latitude <= 90) { return latitude; // common case, and avoids slight double precision shifting } @@ -73,7 +73,7 @@ private static double normalizeLatitude(double latitude) { } /** Puts longitude in range of -180 to +180. */ - private static double normalizeLongitude(double longitude) { + public static double normalizeLongitude(double longitude) { if (longitude >= -180 && longitude <= 180) { return longitude; // common case, and avoids slight double precision shifting } diff --git a/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGeometryGenerator.java b/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGeometryGenerator.java index caf15507e08c5..c6f78e846955d 100644 --- a/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGeometryGenerator.java +++ b/modules/geo/src/test/java/org/opensearch/geo/tests/common/RandomGeoGeometryGenerator.java @@ -199,6 +199,26 @@ public static Rectangle randomRectangle(final Random r) { return new Rectangle(minX, maxX, maxY, minY); } + /** + * Generates a {@link Rectangle} of a specific radius. The generated rectangle can cross the international date line. + * + * @param r {@link Random} + * @param radius double + * @return {@link Rectangle} + */ + public static Rectangle randomRectangle(final Random r, double radius) { + final double[] centre = new double[2]; + RandomGeoGenerator.randomPointIn(r, -180, -(90 - radius), 180, 90 - radius, centre); + final double centreX = centre[0]; + final double centreY = centre[1]; + return new Rectangle( + RandomGeoGenerator.normalizeLongitude(centreX - radius), + RandomGeoGenerator.normalizeLongitude(centreX + radius), + centreY + radius, + centreY - radius + ); + } + /** * Returns a double array where pt[0] : longitude and pt[1] : latitude * diff --git a/server/src/main/java/org/opensearch/common/geo/GeoShapeDocValue.java b/server/src/main/java/org/opensearch/common/geo/GeoShapeDocValue.java index 9bc28c1f67d47..0c6158598a423 100644 --- a/server/src/main/java/org/opensearch/common/geo/GeoShapeDocValue.java +++ b/server/src/main/java/org/opensearch/common/geo/GeoShapeDocValue.java @@ -10,12 +10,18 @@ import org.apache.lucene.document.Field; import org.apache.lucene.document.LatLonShape; +import org.apache.lucene.document.LatLonShapeDocValues; import org.apache.lucene.document.LatLonShapeDocValuesField; +import org.apache.lucene.geo.LatLonGeometry; import org.apache.lucene.index.IndexableField; +import org.apache.lucene.index.PointValues; import org.apache.lucene.util.BytesRef; import org.opensearch.geometry.Geometry; +import org.opensearch.geometry.GeometryVisitor; +import org.opensearch.geometry.Rectangle; import org.opensearch.index.mapper.GeoShapeIndexer; +import java.io.IOException; import java.util.List; /** @@ -25,6 +31,7 @@ */ public class GeoShapeDocValue extends ShapeDocValue { private static final String FIELD_NAME = "missingField"; + private final LatLonShapeDocValues shapeDocValues; public GeoShapeDocValue(final String fieldName, final BytesRef bytesRef) { this(LatLonShape.createDocValueField(fieldName, bytesRef)); @@ -39,6 +46,7 @@ public GeoShapeDocValue(final LatLonShapeDocValuesField shapeDocValuesField) { shapeDocValuesField.getBoundingBox().minLon, shapeDocValuesField.getBoundingBox().minLat ); + this.shapeDocValues = LatLonShape.createLatLonShapeDocValues(shapeDocValuesField.binaryValue()); } /** @@ -172,4 +180,21 @@ public String toString() { } } + + /** + * Checks if the input {@link Rectangle} is intersecting with the shape represented as {@link GeoShapeDocValue}. + * We could have used the {@link GeometryVisitor} here and added the functionality to check the intersection with + * other {@link Geometry} also, but that will be an overkill for now, if required we can easily create a + * {@link GeometryVisitor} to check the intersection with this Shape represented as {@link GeoShapeDocValue}. + * @return boolean + */ + public boolean isIntersectingRectangle(final Rectangle rectangle) { + final org.apache.lucene.geo.Rectangle luceneRectangle = GeoShapeUtils.toLuceneRectangle(rectangle); + try { + final PointValues.Relation relation = shapeDocValues.relate(LatLonGeometry.create(luceneRectangle)); + return relation != PointValues.Relation.CELL_OUTSIDE_QUERY; + } catch (IOException e) { + throw new RuntimeException(e); + } + } } diff --git a/server/src/main/java/org/opensearch/search/aggregations/bucket/GeoTileUtils.java b/server/src/main/java/org/opensearch/search/aggregations/bucket/GeoTileUtils.java index c0b91cd42928d..d37780f9808dc 100644 --- a/server/src/main/java/org/opensearch/search/aggregations/bucket/GeoTileUtils.java +++ b/server/src/main/java/org/opensearch/search/aggregations/bucket/GeoTileUtils.java @@ -35,6 +35,7 @@ import org.apache.lucene.util.SloppyMath; import org.opensearch.OpenSearchParseException; import org.opensearch.common.geo.GeoPoint; +import org.opensearch.common.geo.GeoShapeDocValue; import org.opensearch.common.util.OpenSearchSloppyMath; import org.opensearch.core.xcontent.ObjectParser.ValueType; import org.opensearch.core.xcontent.XContentParser; @@ -42,6 +43,8 @@ import org.opensearch.geometry.Rectangle; import java.io.IOException; +import java.util.ArrayList; +import java.util.List; import java.util.Locale; import static org.opensearch.common.geo.GeoUtils.normalizeLat; @@ -249,6 +252,13 @@ public static String stringEncode(long hash) { return "" + res[0] + "/" + res[1] + "/" + res[2]; } + /** + * Encode lon/lat to the geotile based string format which is "zoom/x/y" + */ + public static String stringEncode(double longitude, double latitude, int precision) { + return stringEncode(longEncode(longitude, latitude, precision)); + } + /** * Decode long hash as a GeoPoint (center of the tile) */ @@ -278,6 +288,40 @@ public static Rectangle toBoundingBox(String hash) { return toBoundingBox(hashAsInts[1], hashAsInts[2], hashAsInts[0]); } + /** + * The function encodes the shape provided as {@link GeoShapeDocValue} to a {@link List} of {@link Long} values + * (representing the GeoTiles) which are intersecting with the shapes at a given precision. + * + * @param geoShapeDocValue {@link GeoShapeDocValue} + * @param precision int + * @return {@link List} of {@link Long} + */ + public static List encodeShape(final GeoShapeDocValue geoShapeDocValue, final int precision) { + final GeoShapeDocValue.BoundingRectangle boundingRectangle = geoShapeDocValue.getBoundingRectangle(); + // generate all the grid long values that this shape intersects. + final long totalTilesAtPrecision = 1L << checkPrecisionRange(precision); + int maxXTile = getXTile(boundingRectangle.getMaxX(), totalTilesAtPrecision); + int minXTile = getXTile(boundingRectangle.getMinX(), totalTilesAtPrecision); + // as tuples in tiles are x,y and y(lat) increases from north to south in tiles, so for minYTile we need to + // take maxY and for maxYTile we need to take minY. + int minYTile = getYTile(boundingRectangle.getMaxY(), totalTilesAtPrecision); + int maxYTile = getYTile(boundingRectangle.getMinY(), totalTilesAtPrecision); + final List encodedValues = new ArrayList<>(); + for (int x = minXTile; x <= maxXTile; x++) { + for (int y = minYTile; y <= maxYTile; y++) { + // Convert the precision, x , y to encoded value. + long encodedValue = longEncodeTiles(precision, x, y); + // Convert encoded value to rectangle + final Rectangle tileRectangle = toBoundingBox(encodedValue); + // check to see if the GeoShape is intersecting with the rectangle. + if (geoShapeDocValue.isIntersectingRectangle(tileRectangle)) { + encodedValues.add(encodedValue); + } + } + } + return encodedValues; + } + public static Rectangle toBoundingBox(int xTile, int yTile, int precision) { final double tiles = validateZXY(precision, xTile, yTile); final double minN = Math.PI - (2.0 * Math.PI * (yTile + 1)) / tiles; From a66c3ba678f5a1c69858026271e981eee0ce0b3c Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Fri, 3 Feb 2023 07:43:23 -0800 Subject: [PATCH 2/4] =?UTF-8?q?[opensearch-project/geospatial#212]=20Fixin?= =?UTF-8?q?g=20the=20IT=20for=20GeoTilesAggrega=E2=80=A6=20(#6120)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixing the IT for GeoTilesAggregation. Signed-off-by: Navneet Verma --- ...AbstractGeoBucketAggregationIntegTest.java | 16 +- .../aggregations/bucket/GeoHashGridIT.java | 254 +++++++++--------- .../aggregations/bucket/GeoTileGridIT.java | 12 +- 3 files changed, 151 insertions(+), 131 deletions(-) diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java index 70dd06fea23ba..299ba0640ef8b 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java @@ -42,6 +42,8 @@ public abstract class AbstractGeoBucketAggregationIntegTest extends GeoModulePlu protected static final int MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING = 4; + protected static final int MIN_PRECISION_WITHOUT_BB_AGGS = 2; + protected static final int NUM_DOCS = 100; protected static final String GEO_SHAPE_INDEX_NAME = "geoshape_index"; @@ -94,8 +96,9 @@ protected void prepareGeoShapeIndexForAggregations(final Random random) throws E continue; } } + i++; - final Set values = generateBucketsForGeometry(geometry, geometryDocValue); + final Set values = generateBucketsForGeometry(geometry, geometryDocValue, isShapeIntersectingBB); geoshapes.add(indexGeoShape(GEO_SHAPE_INDEX_NAME, geometry)); for (final String hash : values) { expectedDocsCountForGeoShapes.put(hash, expectedDocsCountForGeoShapes.getOrDefault(hash, 0) + 1); @@ -109,11 +112,16 @@ protected void prepareGeoShapeIndexForAggregations(final Random random) throws E * Returns a set of buckets for the shape at different precision level. Override this method for different bucket * aggregations. * - * @param geometry {@link Geometry} - * @param geoShapeDocValue {@link GeoShapeDocValue} + * @param geometry {@link Geometry} + * @param geoShapeDocValue {@link GeoShapeDocValue} + * @param intersectingWithBB boolean * @return A {@link Set} of {@link String} which represents the buckets. */ - protected abstract Set generateBucketsForGeometry(final Geometry geometry, final GeoShapeDocValue geoShapeDocValue); + protected abstract Set generateBucketsForGeometry( + final Geometry geometry, + final GeoShapeDocValue geoShapeDocValue, + final boolean intersectingWithBB + ); /** * Prepares a GeoPoint index for testing the GeoPoint bucket aggregations. Different bucket aggregations can use diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java index 8ce0385b1ee9a..e24f69ea247f2 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java @@ -32,26 +32,23 @@ package org.opensearch.geo.search.aggregations.bucket; import com.carrotsearch.hppc.ObjectIntHashMap; -import com.carrotsearch.hppc.ObjectIntMap; import com.carrotsearch.hppc.cursors.ObjectIntCursor; -import org.opensearch.Version; -import org.opensearch.action.index.IndexRequestBuilder; import org.opensearch.action.search.SearchResponse; -import org.opensearch.cluster.metadata.IndexMetadata; +import org.opensearch.common.geo.GeoBoundingBox; import org.opensearch.common.geo.GeoPoint; -import org.opensearch.common.settings.Settings; -import org.opensearch.core.xcontent.XContentBuilder; -import org.opensearch.geo.GeoModulePluginIntegTestCase; +import org.opensearch.common.geo.GeoShapeDocValue; import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoGrid; +import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoGridAggregationBuilder; +import org.opensearch.geo.search.aggregations.common.GeoBoundsHelper; import org.opensearch.geo.tests.common.AggregationBuilders; +import org.opensearch.geometry.Geometry; +import org.opensearch.geometry.Rectangle; +import org.opensearch.geometry.utils.Geohash; import org.opensearch.index.query.GeoBoundingBoxQueryBuilder; import org.opensearch.search.aggregations.InternalAggregation; import org.opensearch.search.aggregations.bucket.filter.Filter; import org.opensearch.test.OpenSearchIntegTestCase; -import org.opensearch.test.VersionUtils; -import java.util.ArrayList; -import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Random; @@ -59,110 +56,35 @@ import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; -import static org.opensearch.common.xcontent.XContentFactory.jsonBuilder; import static org.opensearch.geometry.utils.Geohash.PRECISION; import static org.opensearch.geometry.utils.Geohash.stringEncode; -import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertAcked; import static org.opensearch.test.hamcrest.OpenSearchAssertions.assertSearchResponse; @OpenSearchIntegTestCase.SuiteScopeTestCase -public class GeoHashGridIT extends GeoModulePluginIntegTestCase { +public class GeoHashGridIT extends AbstractGeoBucketAggregationIntegTest { - @Override - protected boolean forbidPrivateIndexSettings() { - return false; - } - - private Version version = VersionUtils.randomIndexCompatibleVersion(random()); - - static ObjectIntMap expectedDocCountsForGeoHash = null; - static ObjectIntMap multiValuedExpectedDocCountsForGeoHash = null; - static int numDocs = 100; - - static String smallestGeoHash = null; - - private static IndexRequestBuilder indexCity(String index, String name, List latLon) throws Exception { - XContentBuilder source = jsonBuilder().startObject().field("city", name); - if (latLon != null) { - source = source.field("location", latLon); - } - source = source.endObject(); - return client().prepareIndex(index).setSource(source); - } - - private static IndexRequestBuilder indexCity(String index, String name, String latLon) throws Exception { - return indexCity(index, name, Arrays.asList(latLon)); - } + private static final String AGG_NAME = "geohashgrid"; @Override public void setupSuiteScopeCluster() throws Exception { - createIndex("idx_unmapped"); - - Settings settings = Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, version).build(); - - assertAcked(prepareCreate("idx").setSettings(settings).setMapping("location", "type=geo_point", "city", "type=keyword")); - - List cities = new ArrayList<>(); Random random = random(); - expectedDocCountsForGeoHash = new ObjectIntHashMap<>(numDocs * 2); - for (int i = 0; i < numDocs; i++) { - // generate random point - double lat = (180d * random.nextDouble()) - 90d; - double lng = (360d * random.nextDouble()) - 180d; - String randomGeoHash = stringEncode(lng, lat, PRECISION); - // Index at the highest resolution - cities.add(indexCity("idx", randomGeoHash, lat + ", " + lng)); - expectedDocCountsForGeoHash.put(randomGeoHash, expectedDocCountsForGeoHash.getOrDefault(randomGeoHash, 0) + 1); - // Update expected doc counts for all resolutions.. - for (int precision = PRECISION - 1; precision > 0; precision--) { - String hash = stringEncode(lng, lat, precision); - if ((smallestGeoHash == null) || (hash.length() < smallestGeoHash.length())) { - smallestGeoHash = hash; - } - expectedDocCountsForGeoHash.put(hash, expectedDocCountsForGeoHash.getOrDefault(hash, 0) + 1); - } - } - indexRandom(true, cities); - - assertAcked( - prepareCreate("multi_valued_idx").setSettings(settings).setMapping("location", "type=geo_point", "city", "type=keyword") - ); - - cities = new ArrayList<>(); - multiValuedExpectedDocCountsForGeoHash = new ObjectIntHashMap<>(numDocs * 2); - for (int i = 0; i < numDocs; i++) { - final int numPoints = random.nextInt(4); - List points = new ArrayList<>(); - Set geoHashes = new HashSet<>(); - for (int j = 0; j < numPoints; ++j) { - double lat = (180d * random.nextDouble()) - 90d; - double lng = (360d * random.nextDouble()) - 180d; - points.add(lat + "," + lng); - // Update expected doc counts for all resolutions.. - for (int precision = PRECISION; precision > 0; precision--) { - final String geoHash = stringEncode(lng, lat, precision); - geoHashes.add(geoHash); - } - } - cities.add(indexCity("multi_valued_idx", Integer.toString(i), points)); - for (String hash : geoHashes) { - multiValuedExpectedDocCountsForGeoHash.put(hash, multiValuedExpectedDocCountsForGeoHash.getOrDefault(hash, 0) + 1); - } - } - indexRandom(true, cities); - - ensureSearchable(); + // Creating a BB for limiting the number buckets generated during aggregation + boundingRectangleForGeoShapesAgg = getGridAggregationBoundingBox(random); + expectedDocCountsForSingleGeoPoint = new ObjectIntHashMap<>(); + prepareSingleValueGeoPointIndex(random); + prepareMultiValuedGeoPointIndex(random); + prepareGeoShapeIndexForAggregations(random); } - public void testSimple() throws Exception { + public void testSimple() { for (int precision = 1; precision <= PRECISION; precision++) { SearchResponse response = client().prepareSearch("idx") - .addAggregation(AggregationBuilders.geohashGrid("geohashgrid").field("location").precision(precision)) + .addAggregation(AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).precision(precision)) .get(); assertSearchResponse(response); - GeoGrid geoGrid = response.getAggregations().get("geohashgrid"); + GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); List buckets = geoGrid.getBuckets(); Object[] propertiesKeys = (Object[]) ((InternalAggregation) geoGrid).getProperty("_key"); Object[] propertiesDocCounts = (Object[]) ((InternalAggregation) geoGrid).getProperty("_count"); @@ -171,7 +93,7 @@ public void testSimple() throws Exception { String geohash = cell.getKeyAsString(); long bucketCount = cell.getDocCount(); - int expectedBucketCount = expectedDocCountsForGeoHash.get(geohash); + int expectedBucketCount = expectedDocCountsForSingleGeoPoint.get(geohash); assertNotSame(bucketCount, 0); assertEquals("Geohash " + geohash + " has wrong doc count ", expectedBucketCount, bucketCount); GeoPoint geoPoint = (GeoPoint) propertiesKeys[i]; @@ -181,34 +103,64 @@ public void testSimple() throws Exception { } } - public void testMultivalued() throws Exception { + public void testGeoShapes() { + final GeoBoundingBox boundingBox = new GeoBoundingBox( + new GeoPoint(boundingRectangleForGeoShapesAgg.getMaxLat(), boundingRectangleForGeoShapesAgg.getMinLon()), + new GeoPoint(boundingRectangleForGeoShapesAgg.getMinLat(), boundingRectangleForGeoShapesAgg.getMaxLon()) + ); + for (int precision = 1; precision <= MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING; precision++) { + GeoGridAggregationBuilder builder = AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_SHAPE_FIELD_NAME).precision(precision); + // This makes sure that for only higher precision we are providing the GeoBounding Box. This also ensures + // that we are able to test both bounded and unbounded aggregations + if (precision > MIN_PRECISION_WITHOUT_BB_AGGS) { + builder.setGeoBoundingBox(boundingBox); + } + final SearchResponse response = client().prepareSearch(GEO_SHAPE_INDEX_NAME).addAggregation(builder).get(); + final GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); + final List buckets = geoGrid.getBuckets(); + final Object[] propertiesKeys = (Object[]) ((InternalAggregation) geoGrid).getProperty("_key"); + final Object[] propertiesDocCounts = (Object[]) ((InternalAggregation) geoGrid).getProperty("_count"); + for (int i = 0; i < buckets.size(); i++) { + final GeoGrid.Bucket cell = buckets.get(i); + final String geohash = cell.getKeyAsString(); + + final long bucketCount = cell.getDocCount(); + final int expectedBucketCount = expectedDocsCountForGeoShapes.get(geohash); + assertNotSame(bucketCount, 0); + assertEquals("Geohash " + geohash + " has wrong doc count ", expectedBucketCount, bucketCount); + final GeoPoint geoPoint = (GeoPoint) propertiesKeys[i]; + assertThat(stringEncode(geoPoint.lon(), geoPoint.lat(), precision), equalTo(geohash)); + assertThat((long) propertiesDocCounts[i], equalTo(bucketCount)); + } + } + } + + public void testMultivalued() { for (int precision = 1; precision <= PRECISION; precision++) { SearchResponse response = client().prepareSearch("multi_valued_idx") - .addAggregation(AggregationBuilders.geohashGrid("geohashgrid").field("location").precision(precision)) + .addAggregation(AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).precision(precision)) .get(); - assertSearchResponse(response); - - GeoGrid geoGrid = response.getAggregations().get("geohashgrid"); + GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); for (GeoGrid.Bucket cell : geoGrid.getBuckets()) { String geohash = cell.getKeyAsString(); long bucketCount = cell.getDocCount(); - int expectedBucketCount = multiValuedExpectedDocCountsForGeoHash.get(geohash); + int expectedBucketCount = multiValuedExpectedDocCountsGeoPoint.get(geohash); assertNotSame(bucketCount, 0); assertEquals("Geohash " + geohash + " has wrong doc count ", expectedBucketCount, bucketCount); } } } - public void testFiltered() throws Exception { - GeoBoundingBoxQueryBuilder bbox = new GeoBoundingBoxQueryBuilder("location"); + public void testFiltered() { + GeoBoundingBoxQueryBuilder bbox = new GeoBoundingBoxQueryBuilder(GEO_POINT_FIELD_NAME); bbox.setCorners(smallestGeoHash).queryName("bbox"); for (int precision = 1; precision <= PRECISION; precision++) { SearchResponse response = client().prepareSearch("idx") .addAggregation( org.opensearch.search.aggregations.AggregationBuilders.filter("filtered", bbox) - .subAggregation(AggregationBuilders.geohashGrid("geohashgrid").field("location").precision(precision)) + .subAggregation(AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).precision(precision)) ) .get(); @@ -216,11 +168,11 @@ public void testFiltered() throws Exception { Filter filter = response.getAggregations().get("filtered"); - GeoGrid geoGrid = filter.getAggregations().get("geohashgrid"); + GeoGrid geoGrid = filter.getAggregations().get(AGG_NAME); for (GeoGrid.Bucket cell : geoGrid.getBuckets()) { String geohash = cell.getKeyAsString(); long bucketCount = cell.getDocCount(); - int expectedBucketCount = expectedDocCountsForGeoHash.get(geohash); + int expectedBucketCount = expectedDocCountsForSingleGeoPoint.get(geohash); assertNotSame(bucketCount, 0); assertTrue("Buckets must be filtered", geohash.startsWith(smallestGeoHash)); assertEquals("Geohash " + geohash + " has wrong doc count ", expectedBucketCount, bucketCount); @@ -229,58 +181,58 @@ public void testFiltered() throws Exception { } } - public void testUnmapped() throws Exception { + public void testUnmapped() { for (int precision = 1; precision <= PRECISION; precision++) { SearchResponse response = client().prepareSearch("idx_unmapped") - .addAggregation(AggregationBuilders.geohashGrid("geohashgrid").field("location").precision(precision)) + .addAggregation(AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).precision(precision)) .get(); assertSearchResponse(response); - GeoGrid geoGrid = response.getAggregations().get("geohashgrid"); + GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); assertThat(geoGrid.getBuckets().size(), equalTo(0)); } } - public void testPartiallyUnmapped() throws Exception { + public void testPartiallyUnmapped() { for (int precision = 1; precision <= PRECISION; precision++) { SearchResponse response = client().prepareSearch("idx", "idx_unmapped") - .addAggregation(AggregationBuilders.geohashGrid("geohashgrid").field("location").precision(precision)) + .addAggregation(AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).precision(precision)) .get(); assertSearchResponse(response); - GeoGrid geoGrid = response.getAggregations().get("geohashgrid"); + GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); for (GeoGrid.Bucket cell : geoGrid.getBuckets()) { String geohash = cell.getKeyAsString(); long bucketCount = cell.getDocCount(); - int expectedBucketCount = expectedDocCountsForGeoHash.get(geohash); + int expectedBucketCount = expectedDocCountsForSingleGeoPoint.get(geohash); assertNotSame(bucketCount, 0); assertEquals("Geohash " + geohash + " has wrong doc count ", expectedBucketCount, bucketCount); } } } - public void testTopMatch() throws Exception { + public void testTopMatch() { for (int precision = 1; precision <= PRECISION; precision++) { SearchResponse response = client().prepareSearch("idx") .addAggregation( - AggregationBuilders.geohashGrid("geohashgrid").field("location").size(1).shardSize(100).precision(precision) + AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).size(1).shardSize(100).precision(precision) ) .get(); assertSearchResponse(response); - GeoGrid geoGrid = response.getAggregations().get("geohashgrid"); + GeoGrid geoGrid = response.getAggregations().get(AGG_NAME); // Check we only have one bucket with the best match for that resolution assertThat(geoGrid.getBuckets().size(), equalTo(1)); for (GeoGrid.Bucket cell : geoGrid.getBuckets()) { String geohash = cell.getKeyAsString(); long bucketCount = cell.getDocCount(); int expectedBucketCount = 0; - for (ObjectIntCursor cursor : expectedDocCountsForGeoHash) { + for (ObjectIntCursor cursor : expectedDocCountsForSingleGeoPoint) { if (cursor.key.length() == precision) { expectedBucketCount = Math.max(expectedBucketCount, cursor.value); } @@ -297,10 +249,10 @@ public void testSizeIsZero() { IllegalArgumentException exception = expectThrows( IllegalArgumentException.class, () -> client().prepareSearch("idx") - .addAggregation(AggregationBuilders.geohashGrid("geohashgrid").field("location").size(size).shardSize(shardSize)) + .addAggregation(AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).size(size).shardSize(shardSize)) .get() ); - assertThat(exception.getMessage(), containsString("[size] must be greater than 0. Found [0] in [geohashgrid]")); + assertThat(exception.getMessage(), containsString("[size] must be greater than 0. Found [0] in [" + AGG_NAME + "]")); } public void testShardSizeIsZero() { @@ -309,10 +261,66 @@ public void testShardSizeIsZero() { IllegalArgumentException exception = expectThrows( IllegalArgumentException.class, () -> client().prepareSearch("idx") - .addAggregation(AggregationBuilders.geohashGrid("geohashgrid").field("location").size(size).shardSize(shardSize)) + .addAggregation(AggregationBuilders.geohashGrid(AGG_NAME).field(GEO_POINT_FIELD_NAME).size(size).shardSize(shardSize)) .get() ); - assertThat(exception.getMessage(), containsString("[shardSize] must be greater than 0. Found [0] in [geohashgrid]")); + assertThat(exception.getMessage(), containsString("[shardSize] must be greater than 0. Found [0] in [" + AGG_NAME + "]")); + } + + @Override + protected Set generateBucketsForGeometry( + final Geometry geometry, + final GeoShapeDocValue geometryDocValue, + boolean intersectingWithBB + ) { + final GeoPoint topLeft = new GeoPoint(); + final GeoPoint bottomRight = new GeoPoint(); + assert geometry != null; + GeoBoundsHelper.updateBoundsForGeometry(geometry, topLeft, bottomRight); + final Set geoHashes = new HashSet<>(); + for (int precision = MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING; precision > 0; precision--) { + if (precision > MIN_PRECISION_WITHOUT_BB_AGGS && intersectingWithBB == false) { + continue; + } + final GeoPoint topRight = new GeoPoint(topLeft.getLat(), bottomRight.getLon()); + String currentGeoHash = Geohash.stringEncode(topLeft.getLon(), topLeft.getLat(), precision); + String startingRowGeoHash = currentGeoHash; + String endGeoHashForCurrentRow = Geohash.stringEncode(topRight.getLon(), topRight.getLat(), precision); + String terminatingGeoHash = Geohash.stringEncode(bottomRight.getLon(), bottomRight.getLat(), precision); + while (true) { + final Rectangle currentRectangle = Geohash.toBoundingBox(currentGeoHash); + if (geometryDocValue.isIntersectingRectangle(currentRectangle)) { + geoHashes.add(currentGeoHash); + } + assert currentGeoHash != null; + if (currentGeoHash.equals(terminatingGeoHash)) { + break; + } + if (currentGeoHash.equals(endGeoHashForCurrentRow)) { + // move in south direction + currentGeoHash = Geohash.getNeighbor(startingRowGeoHash, precision, 0, -1); + startingRowGeoHash = currentGeoHash; + endGeoHashForCurrentRow = Geohash.getNeighbor(endGeoHashForCurrentRow, precision, 0, -1); + } else { + // move in East direction + currentGeoHash = Geohash.getNeighbor(currentGeoHash, precision, 1, 0); + } + } + } + return geoHashes; + } + + @Override + protected Set generateBucketsForGeoPoint(final GeoPoint geoPoint) { + Set buckets = new HashSet<>(); + for (int precision = PRECISION; precision > 0; precision--) { + final String hash = Geohash.stringEncode(geoPoint.getLon(), geoPoint.getLat(), precision); + if ((smallestGeoHash == null) || (hash.length() < smallestGeoHash.length())) { + smallestGeoHash = hash; + } + buckets.add(hash); + } + return buckets; } } diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java index 6198c4cef3a34..8a8f8572066e9 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java @@ -60,7 +60,7 @@ public void testGeoShapes() { .precision(precision); // This makes sure that for only higher precision we are providing the GeoBounding Box. This also ensures // that we are able to test both bounded and unbounded aggregations - if (precision > 2) { + if (precision > MIN_PRECISION_WITHOUT_BB_AGGS) { builder.setGeoBoundingBox(boundingBox); } final SearchResponse response = client().prepareSearch(GEO_SHAPE_INDEX_NAME).addAggregation(builder).get(); @@ -134,18 +134,22 @@ public void testMultivaluedGeoPointsAggregation() throws Exception { * Returns a set of buckets for the shape at different precision level. Override this method for different bucket * aggregations. * - * @param geometry {@link Geometry} - * @param geoShapeDocValue {@link GeoShapeDocValue} + * @param geometry {@link Geometry} + * @param geoShapeDocValue {@link GeoShapeDocValue} + * @param intersectingWithBB * @return A {@link Set} of {@link String} which represents the buckets. */ @Override - protected Set generateBucketsForGeometry(Geometry geometry, GeoShapeDocValue geoShapeDocValue) { + protected Set generateBucketsForGeometry(Geometry geometry, GeoShapeDocValue geoShapeDocValue, boolean intersectingWithBB) { final GeoPoint topLeft = new GeoPoint(); final GeoPoint bottomRight = new GeoPoint(); assert geometry != null; GeoBoundsHelper.updateBoundsForGeometry(geometry, topLeft, bottomRight); final Set geoTiles = new HashSet<>(); for (int precision = MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING; precision > 0; precision--) { + if (precision > MIN_PRECISION_WITHOUT_BB_AGGS && intersectingWithBB == false) { + continue; + } geoTiles.addAll( GeoTileUtils.encodeShape(geoShapeDocValue, precision).stream().map(GeoTileUtils::stringEncode).collect(Collectors.toSet()) ); From 9105f0c7cc462db397dbc806abe75e5a6e1e4007 Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Fri, 10 Feb 2023 09:24:25 -0800 Subject: [PATCH 3/4] [#6187, #6222] Fixing the GeoShapes GeoHash and GeoTile Aggregations Integration tests. (#6242) Changes done: * Fixed the ArrayIndexOutOfBoundsException. * Reduced the precision for GeoShapes Aggregation IT testing. Signed-off-by: Navneet Verma --- ...AbstractGeoBucketAggregationIntegTest.java | 15 ++++------- .../aggregations/bucket/GeoHashGridIT.java | 9 +++---- .../aggregations/bucket/GeoTileGridIT.java | 10 ++++---- .../geogrid/cells/GeoShapeCellValues.java | 25 ++++++------------- 4 files changed, 21 insertions(+), 38 deletions(-) diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java index 299ba0640ef8b..7bb4d6c889623 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java @@ -40,7 +40,7 @@ */ public abstract class AbstractGeoBucketAggregationIntegTest extends GeoModulePluginIntegTestCase { - protected static final int MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING = 4; + protected static final int MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING = 2; protected static final int MIN_PRECISION_WITHOUT_BB_AGGS = 2; @@ -98,7 +98,7 @@ protected void prepareGeoShapeIndexForAggregations(final Random random) throws E } i++; - final Set values = generateBucketsForGeometry(geometry, geometryDocValue, isShapeIntersectingBB); + final Set values = generateBucketsForGeometry(geometry, geometryDocValue); geoshapes.add(indexGeoShape(GEO_SHAPE_INDEX_NAME, geometry)); for (final String hash : values) { expectedDocsCountForGeoShapes.put(hash, expectedDocsCountForGeoShapes.getOrDefault(hash, 0) + 1); @@ -112,16 +112,11 @@ protected void prepareGeoShapeIndexForAggregations(final Random random) throws E * Returns a set of buckets for the shape at different precision level. Override this method for different bucket * aggregations. * - * @param geometry {@link Geometry} - * @param geoShapeDocValue {@link GeoShapeDocValue} - * @param intersectingWithBB boolean + * @param geometry {@link Geometry} + * @param geoShapeDocValue {@link GeoShapeDocValue} * @return A {@link Set} of {@link String} which represents the buckets. */ - protected abstract Set generateBucketsForGeometry( - final Geometry geometry, - final GeoShapeDocValue geoShapeDocValue, - final boolean intersectingWithBB - ); + protected abstract Set generateBucketsForGeometry(final Geometry geometry, final GeoShapeDocValue geoShapeDocValue); /** * Prepares a GeoPoint index for testing the GeoPoint bucket aggregations. Different bucket aggregations can use diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java index e24f69ea247f2..3d4cd430a77e2 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java @@ -268,18 +268,15 @@ public void testShardSizeIsZero() { } @Override - protected Set generateBucketsForGeometry( - final Geometry geometry, - final GeoShapeDocValue geometryDocValue, - boolean intersectingWithBB - ) { + protected Set generateBucketsForGeometry(final Geometry geometry, final GeoShapeDocValue geometryDocValue) { final GeoPoint topLeft = new GeoPoint(); final GeoPoint bottomRight = new GeoPoint(); assert geometry != null; GeoBoundsHelper.updateBoundsForGeometry(geometry, topLeft, bottomRight); final Set geoHashes = new HashSet<>(); + final boolean isIntersectingWithBoundingRectangle = geometryDocValue.isIntersectingRectangle(boundingRectangleForGeoShapesAgg); for (int precision = MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING; precision > 0; precision--) { - if (precision > MIN_PRECISION_WITHOUT_BB_AGGS && intersectingWithBB == false) { + if (precision > MIN_PRECISION_WITHOUT_BB_AGGS && isIntersectingWithBoundingRectangle == false) { continue; } final GeoPoint topRight = new GeoPoint(topLeft.getLat(), bottomRight.getLon()); diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java index 8a8f8572066e9..269dc52f29317 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java @@ -134,20 +134,20 @@ public void testMultivaluedGeoPointsAggregation() throws Exception { * Returns a set of buckets for the shape at different precision level. Override this method for different bucket * aggregations. * - * @param geometry {@link Geometry} - * @param geoShapeDocValue {@link GeoShapeDocValue} - * @param intersectingWithBB + * @param geometry {@link Geometry} + * @param geoShapeDocValue {@link GeoShapeDocValue} * @return A {@link Set} of {@link String} which represents the buckets. */ @Override - protected Set generateBucketsForGeometry(Geometry geometry, GeoShapeDocValue geoShapeDocValue, boolean intersectingWithBB) { + protected Set generateBucketsForGeometry(final Geometry geometry, final GeoShapeDocValue geoShapeDocValue) { final GeoPoint topLeft = new GeoPoint(); final GeoPoint bottomRight = new GeoPoint(); assert geometry != null; GeoBoundsHelper.updateBoundsForGeometry(geometry, topLeft, bottomRight); final Set geoTiles = new HashSet<>(); + final boolean isIntersectingWithBoundingRectangle = geoShapeDocValue.isIntersectingRectangle(boundingRectangleForGeoShapesAgg); for (int precision = MAX_PRECISION_FOR_GEO_SHAPES_AGG_TESTING; precision > 0; precision--) { - if (precision > MIN_PRECISION_WITHOUT_BB_AGGS && intersectingWithBB == false) { + if (precision > MIN_PRECISION_WITHOUT_BB_AGGS && isIntersectingWithBoundingRectangle == false) { continue; } geoTiles.addAll( diff --git a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java index 4911818cd448f..123a98fab3713 100644 --- a/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java +++ b/modules/geo/src/main/java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java @@ -10,6 +10,7 @@ import org.opensearch.common.geo.GeoBoundingBox; import org.opensearch.common.geo.GeoShapeDocValue; +import org.opensearch.geometry.Rectangle; import org.opensearch.index.fielddata.AbstractSortingNumericDocValues; import org.opensearch.index.fielddata.GeoShapeValue; @@ -57,8 +58,7 @@ public boolean advanceExact(int docId) throws IOException { * @opensearch.internal */ static class BoundedCellValues extends GeoShapeCellValues { - - private final GeoBoundingBox geoBoundingBox; + private final Rectangle geoBoundingBox; public BoundedCellValues( final GeoShapeValue geoShapeValue, @@ -67,7 +67,7 @@ public BoundedCellValues( final GeoBoundingBox boundingBox ) { super(geoShapeValue, precision, encoder); - this.geoBoundingBox = boundingBox; + this.geoBoundingBox = new Rectangle(boundingBox.left(), boundingBox.right(), boundingBox.top(), boundingBox.bottom()); } /** @@ -78,7 +78,7 @@ public BoundedCellValues( */ @Override void relateShape(final GeoShapeDocValue geoShapeDocValue) { - if (intersect(geoShapeDocValue.getBoundingRectangle())) { + if (geoShapeDocValue.isIntersectingRectangle(geoBoundingBox)) { // now we know the shape is in the bounding rectangle, we need add them in longValues // generate all grid that this shape intersects final List encodedValues = encoder.encode(geoShapeDocValue, precision); @@ -86,22 +86,13 @@ void relateShape(final GeoShapeDocValue geoShapeDocValue) { for (int i = 0; i < encodedValues.size(); i++) { values[i] = encodedValues.get(i); } + } else { + // As the shape is not intersecting with GeoBounding box, we need to reset the GeoShapeCellValues + // calling this function resets the CellValues for the current shape. + resize(0); } } - /** - * Validate that shape is intersecting the bounding box provided as input. - * - * @param rectangle {@link GeoShapeDocValue.BoundingRectangle} - * @return true or false - */ - private boolean intersect(final GeoShapeDocValue.BoundingRectangle rectangle) { - return geoBoundingBox.pointInBounds(rectangle.getMaxLongitude(), rectangle.getMaxLatitude()) - || geoBoundingBox.pointInBounds(rectangle.getMaxLongitude(), rectangle.getMinLatitude()) - || geoBoundingBox.pointInBounds(rectangle.getMinLongitude(), rectangle.getMaxLatitude()) - || geoBoundingBox.pointInBounds(rectangle.getMinLongitude(), rectangle.getMinLatitude()); - } - } /** From 9d1fa92c957460308b621d64bf7cf2050cdf5541 Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Mon, 17 Apr 2023 15:25:06 -0700 Subject: [PATCH 4/4] [#7101] Fixing the GeoTileIT#testMultivaluedGeoPointsAggregation test case. (#7166) The issue was happening because we encode the GeoPoint as long and error comes in the precision due to that encoding. The error was not taken care while generating the exepected tiles count for execpected output. Signed-off-by: Navneet Verma --- .../AbstractGeoBucketAggregationIntegTest.java | 16 +++++++++++++++- .../aggregations/bucket/GeoTileGridIT.java | 3 ++- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java index 7bb4d6c889623..30088c1acb136 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java @@ -10,13 +10,14 @@ import com.carrotsearch.hppc.ObjectIntHashMap; import com.carrotsearch.hppc.ObjectIntMap; +import org.apache.lucene.geo.GeoEncodingUtils; import org.opensearch.Version; import org.opensearch.action.index.IndexRequestBuilder; import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.common.geo.GeoPoint; import org.opensearch.common.geo.GeoShapeDocValue; import org.opensearch.common.settings.Settings; -import org.opensearch.common.xcontent.XContentBuilder; +import org.opensearch.core.xcontent.XContentBuilder; import org.opensearch.geo.GeoModulePluginIntegTestCase; import org.opensearch.geo.tests.common.RandomGeoGenerator; import org.opensearch.geo.tests.common.RandomGeoGeometryGenerator; @@ -254,4 +255,17 @@ protected double getRadiusOfBoundingBox() { return 5.0; } + /** + * Encode and Decode the {@link GeoPoint} to get a {@link GeoPoint} which has the exact precision which is being + * stored. + * @param geoPoint {@link GeoPoint} + * @return {@link GeoPoint} + */ + protected GeoPoint toStoragePrecision(final GeoPoint geoPoint) { + return new GeoPoint( + GeoEncodingUtils.decodeLatitude(GeoEncodingUtils.encodeLatitude(geoPoint.getLat())), + GeoEncodingUtils.decodeLongitude(GeoEncodingUtils.encodeLongitude(geoPoint.getLon())) + ); + } + } diff --git a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java index 269dc52f29317..4c2c13b66d926 100644 --- a/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java +++ b/modules/geo/src/internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java @@ -160,7 +160,8 @@ protected Set generateBucketsForGeometry(final Geometry geometry, final protected Set generateBucketsForGeoPoint(final GeoPoint geoPoint) { Set buckets = new HashSet<>(); for (int precision = GEOPOINT_MAX_PRECISION; precision > 0; precision--) { - final String tile = GeoTileUtils.stringEncode(geoPoint.getLon(), geoPoint.getLat(), precision); + final GeoPoint precisedGeoPoint = this.toStoragePrecision(geoPoint); + final String tile = GeoTileUtils.stringEncode(precisedGeoPoint.getLon(), precisedGeoPoint.getLat(), precision); buckets.add(tile); } return buckets;