[#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 <[email protected]>
opensearch-project · Feb 10, 2023 · 9386cde · 9386cde
1 parent ca9c1ad
commit 9386cde
Show file tree

Hide file tree

Showing 4 changed files with 21 additions and 38 deletions.
diff --git a/.../org/opensearch/geo/search/aggregations/bucket/AbstractGeoBucketAggregationIntegTest.java b/.../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<String> values = generateBucketsForGeometry(geometry, geometryDocValue, isShapeIntersectingBB);
+            final Set<String> 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<String> generateBucketsForGeometry(
-        final Geometry geometry,
-        final GeoShapeDocValue geoShapeDocValue,
-        final boolean intersectingWithBB
-    );
+    protected abstract Set<String> 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/...internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java b/...internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoHashGridIT.java
@@ -268,18 +268,15 @@ public void testShardSizeIsZero() {
     }
 
     @Override
-    protected Set<String> generateBucketsForGeometry(
-        final Geometry geometry,
-        final GeoShapeDocValue geometryDocValue,
-        boolean intersectingWithBB
-    ) {
+    protected Set<String> 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<String> 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/...internalClusterTest/java/org/opensearch/geo/search/aggregations/bucket/GeoTileGridIT.java b/...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<String> generateBucketsForGeometry(Geometry geometry, GeoShapeDocValue geoShapeDocValue, boolean intersectingWithBB) {
+    protected Set<String> 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<String> 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/.../java/org/opensearch/geo/search/aggregations/bucket/geogrid/cells/GeoShapeCellValues.java b/.../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,30 +78,21 @@ 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<Long> encodedValues = encoder.encode(geoShapeDocValue, precision);
                 resize(encodedValues.size());
                 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());
-        }
-
     }
 
     /**