From facb25a840c061d243944727ba24faa462344286 Mon Sep 17 00:00:00 2001 From: Navneet Verma Date: Wed, 8 Feb 2023 12:07:12 -0800 Subject: [PATCH] [#6187, #6222] Fixing the GeoShapes GeoHash and GeoTile Aggregations Integration tests to make them more robust. Signed-off-by: Navneet Verma --- ...AbstractGeoBucketAggregationIntegTest.java | 13 +++------- .../aggregations/bucket/GeoHashGridIT.java | 9 +++---- .../aggregations/bucket/GeoTileGridIT.java | 10 ++++---- .../geogrid/cells/GeoShapeCellValues.java | 25 ++++++------------- 4 files changed, 20 insertions(+), 37 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..229a05ab501b1 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 @@ -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()); - } - } /**