Update GeoGrid base class access modifier to support extensibility (#…

…4572) * Update access modifier to support extensibility Change access modifier from default to protected. This will help to build new geo based aggregation outside OpenSearch, by keeping GeoGrid Classes as base class. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Updated CHANGELOG Added PR details to CHANGELOG.md Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Rename InternalGeoGridBucket to BaseGeoGridBucket Update class names, references and comments. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Rename InternalGeoGrid to BaseGeoGrid Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Make GridBucket classes package-private Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Remove Internal prefix from Geo Grid classes Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Update constructor and class access modifier Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> * Update access modifier based on usage Made classes package private if it is not used outside the package. Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com> Signed-off-by: Vijayan Balasubramanian <balasvij@amazon.com>
opensearch-project · Bukhtawar · Oct 8, 2022 · Aug 24, 2022 · Aug 24, 2022 · Aug 24, 2022
commit b1bc4d96d69e5f1278dfe4aea7e532e1e049c794
@@ -29,6 +29,8 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 - Added release notes for 1.3.6 ([#4681](https://github.com/opensearch-project/OpenSearch/pull/4681))
 - Added precommit support for MacOS ([#4682](https://github.com/opensearch-project/OpenSearch/pull/4682))
 - Recommission API changes for service layer ([#4320](https://github.com/opensearch-project/OpenSearch/pull/4320))
+- Update GeoGrid base class access modifier to support extensibility ([#4572](https://github.com/opensearch-project/OpenSearch/pull/4572))
+
 ### Dependencies
 - Bumps `log4j-core` from 2.18.0 to 2.19.0
 - Bumps `reactor-netty-http` from 1.0.18 to 1.0.23

@@ -35,9 +35,8 @@
 import org.opensearch.geo.search.aggregations.bucket.composite.GeoTileGridValuesSourceBuilder;
 import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoHashGridAggregationBuilder;
 import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoTileGridAggregationBuilder;
-import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoTileGridAggregator;
-import org.opensearch.geo.search.aggregations.bucket.geogrid.InternalGeoHashGrid;
-import org.opensearch.geo.search.aggregations.bucket.geogrid.InternalGeoTileGrid;
+import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoHashGrid;
+import org.opensearch.geo.search.aggregations.bucket.geogrid.GeoTileGrid;
 import org.opensearch.geo.search.aggregations.metrics.GeoBounds;
 import org.opensearch.geo.search.aggregations.metrics.GeoBoundsAggregationBuilder;
 import org.opensearch.geo.search.aggregations.metrics.GeoBoundsGeoShapeAggregator;
@@ -78,18 +77,18 @@ public List<AggregationSpec> getAggregations() {
             GeoHashGridAggregationBuilder.NAME,
             GeoHashGridAggregationBuilder::new,
             GeoHashGridAggregationBuilder.PARSER
-        ).addResultReader(InternalGeoHashGrid::new).setAggregatorRegistrar(GeoHashGridAggregationBuilder::registerAggregators);
+        ).addResultReader(GeoHashGrid::new).setAggregatorRegistrar(GeoHashGridAggregationBuilder::registerAggregators);
 
         final AggregationSpec geoTileGrid = new AggregationSpec(
             GeoTileGridAggregationBuilder.NAME,
             GeoTileGridAggregationBuilder::new,
             GeoTileGridAggregationBuilder.PARSER
-        ).addResultReader(InternalGeoTileGrid::new).setAggregatorRegistrar(GeoTileGridAggregationBuilder::registerAggregators);
+        ).addResultReader(GeoTileGrid::new).setAggregatorRegistrar(GeoTileGridAggregationBuilder::registerAggregators);
         return List.of(geoBounds, geoHashGrid, geoTileGrid);
     }
 
     /**
-     * Registering the {@link GeoTileGridAggregator} in the {@link CompositeAggregation}.
+     * Registering the geotile grid in the {@link CompositeAggregation}.
      *
      * @return a {@link List} of {@link CompositeAggregationSpec}
      */

@@ -54,30 +54,30 @@
  * All geo-grid hash-encoding in a grid are of the same precision and held internally as a single long
  * for efficiency's sake.
  *
- * @opensearch.internal
+ * @opensearch.api
  */
-public abstract class InternalGeoGrid<B extends InternalGeoGridBucket> extends InternalMultiBucketAggregation<
-    InternalGeoGrid,
-    InternalGeoGridBucket> implements GeoGrid {
+public abstract class BaseGeoGrid<B extends BaseGeoGridBucket> extends InternalMultiBucketAggregation<BaseGeoGrid, BaseGeoGridBucket>
+    implements
+        GeoGrid {
 
     protected final int requiredSize;
-    protected final List<InternalGeoGridBucket> buckets;
+    protected final List<BaseGeoGridBucket> buckets;
 
-    InternalGeoGrid(String name, int requiredSize, List<InternalGeoGridBucket> buckets, Map<String, Object> metadata) {
+    protected BaseGeoGrid(String name, int requiredSize, List<BaseGeoGridBucket> buckets, Map<String, Object> metadata) {
         super(name, metadata);
         this.requiredSize = requiredSize;
         this.buckets = buckets;
     }
 
-    abstract Writeable.Reader<B> getBucketReader();
+    protected abstract Writeable.Reader<B> getBucketReader();
 
     /**
      * Read from a stream.
      */
-    public InternalGeoGrid(StreamInput in) throws IOException {
+    public BaseGeoGrid(StreamInput in) throws IOException {
         super(in);
         requiredSize = readSize(in);
-        buckets = (List<InternalGeoGridBucket>) in.readList(getBucketReader());
+        buckets = (List<BaseGeoGridBucket>) in.readList(getBucketReader());
     }
 
     @Override
@@ -86,24 +86,24 @@ protected void doWriteTo(StreamOutput out) throws IOException {
         out.writeList(buckets);
     }
 
-    abstract InternalGeoGrid create(String name, int requiredSize, List<InternalGeoGridBucket> buckets, Map<String, Object> metadata);
+    protected abstract BaseGeoGrid create(String name, int requiredSize, List<BaseGeoGridBucket> buckets, Map<String, Object> metadata);
 
     @Override
-    public List<InternalGeoGridBucket> getBuckets() {
+    public List<BaseGeoGridBucket> getBuckets() {
         return unmodifiableList(buckets);
     }
 
     @Override
-    public InternalGeoGrid reduce(List<InternalAggregation> aggregations, ReduceContext reduceContext) {
-        LongObjectPagedHashMap<List<InternalGeoGridBucket>> buckets = null;
+    public BaseGeoGrid reduce(List<InternalAggregation> aggregations, ReduceContext reduceContext) {
+        LongObjectPagedHashMap<List<BaseGeoGridBucket>> buckets = null;
         for (InternalAggregation aggregation : aggregations) {
-            InternalGeoGrid grid = (InternalGeoGrid) aggregation;
+            BaseGeoGrid grid = (BaseGeoGrid) aggregation;
             if (buckets == null) {
                 buckets = new LongObjectPagedHashMap<>(grid.buckets.size(), reduceContext.bigArrays());
             }
             for (Object obj : grid.buckets) {
-                InternalGeoGridBucket bucket = (InternalGeoGridBucket) obj;
-                List<InternalGeoGridBucket> existingBuckets = buckets.get(bucket.hashAsLong());
+                BaseGeoGridBucket bucket = (BaseGeoGridBucket) obj;
+                List<BaseGeoGridBucket> existingBuckets = buckets.get(bucket.hashAsLong());
                 if (existingBuckets == null) {
                     existingBuckets = new ArrayList<>(aggregations.size());
                     buckets.put(bucket.hashAsLong(), existingBuckets);
@@ -113,13 +113,13 @@ public InternalGeoGrid reduce(List<InternalAggregation> aggregations, ReduceCont
         }
 
         final int size = Math.toIntExact(reduceContext.isFinalReduce() == false ? buckets.size() : Math.min(requiredSize, buckets.size()));
-        BucketPriorityQueue<InternalGeoGridBucket> ordered = new BucketPriorityQueue<>(size);
-        for (LongObjectPagedHashMap.Cursor<List<InternalGeoGridBucket>> cursor : buckets) {
-            List<InternalGeoGridBucket> sameCellBuckets = cursor.value;
+        BucketPriorityQueue<BaseGeoGridBucket> ordered = new BucketPriorityQueue<>(size);
+        for (LongObjectPagedHashMap.Cursor<List<BaseGeoGridBucket>> cursor : buckets) {
+            List<BaseGeoGridBucket> sameCellBuckets = cursor.value;
             ordered.insertWithOverflow(reduceBucket(sameCellBuckets, reduceContext));
         }
         buckets.close();
-        InternalGeoGridBucket[] list = new InternalGeoGridBucket[ordered.size()];
+        BaseGeoGridBucket[] list = new BaseGeoGridBucket[ordered.size()];
         for (int i = ordered.size() - 1; i >= 0; i--) {
             list[i] = ordered.pop();
         }
@@ -128,24 +128,24 @@ public InternalGeoGrid reduce(List<InternalAggregation> aggregations, ReduceCont
     }
 
     @Override
-    protected InternalGeoGridBucket reduceBucket(List<InternalGeoGridBucket> buckets, ReduceContext context) {
+    protected BaseGeoGridBucket reduceBucket(List<BaseGeoGridBucket> buckets, ReduceContext context) {
         assert buckets.size() > 0;
         List<InternalAggregations> aggregationsList = new ArrayList<>(buckets.size());
         long docCount = 0;
-        for (InternalGeoGridBucket bucket : buckets) {
+        for (BaseGeoGridBucket bucket : buckets) {
             docCount += bucket.docCount;
             aggregationsList.add(bucket.aggregations);
         }
         final InternalAggregations aggs = InternalAggregations.reduce(aggregationsList, context);
         return createBucket(buckets.get(0).hashAsLong, docCount, aggs);
     }
 
-    abstract B createBucket(long hashAsLong, long docCount, InternalAggregations aggregations);
+    protected abstract B createBucket(long hashAsLong, long docCount, InternalAggregations aggregations);
 
     @Override
     public XContentBuilder doXContentBody(XContentBuilder builder, Params params) throws IOException {
         builder.startArray(CommonFields.BUCKETS.getPreferredName());
-        for (InternalGeoGridBucket bucket : buckets) {
+        for (BaseGeoGridBucket bucket : buckets) {
             bucket.toXContent(builder, params);
         }
         builder.endArray();
@@ -168,7 +168,7 @@ public boolean equals(Object obj) {
         if (obj == null || getClass() != obj.getClass()) return false;
         if (super.equals(obj) == false) return false;
 
-        InternalGeoGrid other = (InternalGeoGrid) obj;
+        BaseGeoGrid other = (BaseGeoGrid) obj;
         return Objects.equals(requiredSize, other.requiredSize) && Objects.equals(buckets, other.buckets);
     }
 

@@ -45,20 +45,20 @@
 /**
  * Base implementation of geogrid aggs
  *
- * @opensearch.internal
+ * @opensearch.api
  */
-public abstract class InternalGeoGridBucket<B extends InternalGeoGridBucket> extends InternalMultiBucketAggregation.InternalBucket
+public abstract class BaseGeoGridBucket<B extends BaseGeoGridBucket> extends InternalMultiBucketAggregation.InternalBucket
     implements
         GeoGrid.Bucket,
-        Comparable<InternalGeoGridBucket> {
+        Comparable<BaseGeoGridBucket> {
 
     protected long hashAsLong;
     protected long docCount;
     protected InternalAggregations aggregations;
 
     long bucketOrd;
 
-    public InternalGeoGridBucket(long hashAsLong, long docCount, InternalAggregations aggregations) {
+    public BaseGeoGridBucket(long hashAsLong, long docCount, InternalAggregations aggregations) {
         this.docCount = docCount;
         this.aggregations = aggregations;
         this.hashAsLong = hashAsLong;
@@ -67,7 +67,7 @@ public InternalGeoGridBucket(long hashAsLong, long docCount, InternalAggregation
     /**
      * Read from a stream.
      */
-    public InternalGeoGridBucket(StreamInput in) throws IOException {
+    public BaseGeoGridBucket(StreamInput in) throws IOException {
         hashAsLong = in.readLong();
         docCount = in.readVLong();
         aggregations = InternalAggregations.readFrom(in);
@@ -80,7 +80,7 @@ public void writeTo(StreamOutput out) throws IOException {
         aggregations.writeTo(out);
     }
 
-    long hashAsLong() {
+    public long hashAsLong() {
         return hashAsLong;
     }
 
@@ -95,7 +95,7 @@ public Aggregations getAggregations() {
     }
 
     @Override
-    public int compareTo(InternalGeoGridBucket other) {
+    public int compareTo(BaseGeoGridBucket other) {
         if (this.hashAsLong > other.hashAsLong) {
             return 1;
         }
@@ -119,7 +119,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
     public boolean equals(Object o) {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
-        InternalGeoGridBucket bucket = (InternalGeoGridBucket) o;
+        BaseGeoGridBucket bucket = (BaseGeoGridBucket) o;
         return hashAsLong == bucket.hashAsLong && docCount == bucket.docCount && Objects.equals(aggregations, bucket.aggregations);
     }
 

@@ -38,14 +38,14 @@
  *
  * @opensearch.internal
  */
-class BucketPriorityQueue<B extends InternalGeoGridBucket> extends PriorityQueue<B> {
+class BucketPriorityQueue<B extends BaseGeoGridBucket> extends PriorityQueue<B> {
 
     BucketPriorityQueue(int size) {
         super(size);
     }
 
     @Override
-    protected boolean lessThan(InternalGeoGridBucket o1, InternalGeoGridBucket o2) {
+    protected boolean lessThan(BaseGeoGridBucket o1, BaseGeoGridBucket o2) {
         int cmp = Long.compare(o2.getDocCount(), o1.getDocCount());
         if (cmp == 0) {
             cmp = o2.compareTo(o1);

@@ -43,7 +43,7 @@
  * Wrapper class to help convert {@link MultiGeoPointValues}
  * to numeric long values for bucketing.
  *
- * @opensearch.internal
+ * @opensearch.api
  */
 public class CellIdSource extends ValuesSource.Numeric {
     private final ValuesSource.GeoPoint valuesSource;

@@ -39,13 +39,13 @@
  * A geo-grid aggregation. Defines multiple buckets, each representing a cell in a geo-grid of a specific
  * precision.
  *
- * @opensearch.internal
+ * @opensearch.api
  */
 public interface GeoGrid extends MultiBucketsAggregation {
 
     /**
      * A bucket that is associated with a geo-grid cell. The key of the bucket is
-     * the {@link InternalGeoGridBucket#getKeyAsString()} of the cell
+     * the {@link BaseGeoGridBucket#getKeyAsString()} of the cell
      */
     interface Bucket extends MultiBucketsAggregation.Bucket {}
 

@@ -58,9 +58,9 @@
 import java.util.function.Function;
 
 /**
- * Base Aggregation Builder for geohash_grid and geotile_grid aggs
+ * Base Aggregation Builder for geogrid aggs
  *
- * @opensearch.internal
+ * @opensearch.api
  */
 public abstract class GeoGridAggregationBuilder extends ValuesSourceAggregationBuilder<GeoGridAggregationBuilder> {
     /* recognized field names in JSON */

@@ -55,16 +55,16 @@
 /**
  * Aggregates data expressed as longs (for efficiency's sake) but formats results as aggregation-specific strings.
  *
- * @opensearch.internal
+ * @opensearch.api
  */
-public abstract class GeoGridAggregator<T extends InternalGeoGrid> extends BucketsAggregator {
+public abstract class GeoGridAggregator<T extends BaseGeoGrid> extends BucketsAggregator {
 
     protected final int requiredSize;
     protected final int shardSize;
     protected final ValuesSource.Numeric valuesSource;
     protected final LongKeyedBucketOrds bucketOrds;
 
-    GeoGridAggregator(
+    protected GeoGridAggregator(
         String name,
         AggregatorFactories factories,
         ValuesSource.Numeric valuesSource,
@@ -118,23 +118,23 @@ public void collect(int doc, long owningBucketOrd) throws IOException {
         };
     }
 
-    abstract T buildAggregation(String name, int requiredSize, List<InternalGeoGridBucket> buckets, Map<String, Object> metadata);
+    protected abstract T buildAggregation(String name, int requiredSize, List<BaseGeoGridBucket> buckets, Map<String, Object> metadata);
 
     /**
      * This method is used to return a re-usable instance of the bucket when building
      * the aggregation.
-     * @return a new {@link InternalGeoGridBucket} implementation with empty parameters
+     * @return a new {@link BaseGeoGridBucket} implementation with empty parameters
      */
-    abstract InternalGeoGridBucket newEmptyBucket();
+    protected abstract BaseGeoGridBucket newEmptyBucket();
 
     @Override
     public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws IOException {
-        InternalGeoGridBucket[][] topBucketsPerOrd = new InternalGeoGridBucket[owningBucketOrds.length][];
+        BaseGeoGridBucket[][] topBucketsPerOrd = new BaseGeoGridBucket[owningBucketOrds.length][];
         for (int ordIdx = 0; ordIdx < owningBucketOrds.length; ordIdx++) {
             int size = (int) Math.min(bucketOrds.bucketsInOrd(owningBucketOrds[ordIdx]), shardSize);
 
-            BucketPriorityQueue<InternalGeoGridBucket> ordered = new BucketPriorityQueue<>(size);
-            InternalGeoGridBucket spare = null;
+            BucketPriorityQueue<BaseGeoGridBucket> ordered = new BucketPriorityQueue<>(size);
+            BaseGeoGridBucket spare = null;
             LongKeyedBucketOrds.BucketOrdsEnum ordsEnum = bucketOrds.ordsEnum(owningBucketOrds[ordIdx]);
             while (ordsEnum.next()) {
                 if (spare == null) {
@@ -149,7 +149,7 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
                 spare = ordered.insertWithOverflow(spare);
             }
 
-            topBucketsPerOrd[ordIdx] = new InternalGeoGridBucket[ordered.size()];
+            topBucketsPerOrd[ordIdx] = new BaseGeoGridBucket[ordered.size()];
             for (int i = ordered.size() - 1; i >= 0; --i) {
                 topBucketsPerOrd[ordIdx][i] = ordered.pop();
             }
@@ -163,7 +163,7 @@ public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws I
     }
 
     @Override
-    public InternalGeoGrid buildEmptyAggregation() {
+    public BaseGeoGrid buildEmptyAggregation() {
         return buildAggregation(name, requiredSize, Collections.emptyList(), metadata());
     }