redesign implementations

Signed-off-by: Sarthak Aggarwal <[email protected]>
sarthakaggarwal97 · Aug 5, 2024 · 6f1ae7a · 6f1ae7a
1 parent 243b48e
commit 6f1ae7a
Show file tree

Hide file tree

Showing 13 changed files with 168 additions and 308 deletions.
diff --git a/...g/opensearch/index/compositeindex/datacube/startree/aggregators/CountValueAggregator.java b/...g/opensearch/index/compositeindex/datacube/startree/aggregators/CountValueAggregator.java
@@ -7,7 +7,6 @@
  */
 package org.opensearch.index.compositeindex.datacube.startree.aggregators;
 
-import org.opensearch.index.compositeindex.datacube.MetricStat;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 
 /**
@@ -17,24 +16,13 @@
  */
 public class CountValueAggregator implements ValueAggregator<Long> {
 
-    private static final StarTreeNumericType VALUE_AGGREGATOR_TYPE = StarTreeNumericType.LONG;
     public static final long DEFAULT_INITIAL_VALUE = 1L;
     private final StarTreeNumericType starTreeNumericType;
 
     public CountValueAggregator(StarTreeNumericType starTreeNumericType) {
         this.starTreeNumericType = starTreeNumericType;
     }
 
-    @Override
-    public MetricStat getAggregationType() {
-        return MetricStat.COUNT;
-    }
-
-    @Override
-    public StarTreeNumericType getAggregatedValueType() {
-        return VALUE_AGGREGATOR_TYPE;
-    }
-
     @Override
     public Long getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue) {
 
@@ -64,24 +52,6 @@ public Long mergeAggregatedValues(Long value, Long aggregatedValue) {
         return value + aggregatedValue;
     }
 
-    @Override
-    public Long getInitialAggregatedValue(Long value) {
-        if (value == null) {
-            return getIdentityMetricValue();
-        }
-        return value;
-    }
-
-    @Override
-    public int getMaxAggregatedValueByteSize() {
-        return Long.BYTES;
-    }
-
-    @Override
-    public Long toLongValue(Long value) {
-        return value;
-    }
-
     @Override
     public Long toStarTreeNumericTypeValue(Long value) {
         return value;

diff --git a/...org/opensearch/index/compositeindex/datacube/startree/aggregators/MaxValueAggregator.java b/...org/opensearch/index/compositeindex/datacube/startree/aggregators/MaxValueAggregator.java
@@ -7,22 +7,21 @@
  */
 package org.opensearch.index.compositeindex.datacube.startree.aggregators;
 
-import org.opensearch.index.compositeindex.datacube.MetricStat;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 
 /**
  * Max value aggregator for star tree
  *
  * @opensearch.experimental
  */
-public class MaxValueAggregator extends MinMaxValueAggregator {
+public class MaxValueAggregator extends StatelessDoubleValueAggregator {
 
     public MaxValueAggregator(StarTreeNumericType starTreeNumericType) {
-        super(MetricStat.MAX, starTreeNumericType);
+        super(starTreeNumericType, null);
     }
 
     @Override
-    public Double performValueAggregation(Double aggregatedValue, Double segmentDocValue) {
+    protected Double performValueAggregation(Double aggregatedValue, Double segmentDocValue) {
         return Math.max(aggregatedValue, segmentDocValue);
     }
 }
diff --git a/.../opensearch/index/compositeindex/datacube/startree/aggregators/MinMaxValueAggregator.java b/.../opensearch/index/compositeindex/datacube/startree/aggregators/MinMaxValueAggregator.java
diff --git a/...org/opensearch/index/compositeindex/datacube/startree/aggregators/MinValueAggregator.java b/...org/opensearch/index/compositeindex/datacube/startree/aggregators/MinValueAggregator.java
@@ -7,22 +7,21 @@
  */
 package org.opensearch.index.compositeindex.datacube.startree.aggregators;
 
-import org.opensearch.index.compositeindex.datacube.MetricStat;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 
 /**
  * Min value aggregator for star tree
  *
  * @opensearch.experimental
  */
-public class MinValueAggregator extends MinMaxValueAggregator {
+public class MinValueAggregator extends StatelessDoubleValueAggregator {
 
     public MinValueAggregator(StarTreeNumericType starTreeNumericType) {
-        super(MetricStat.MIN, starTreeNumericType);
+        super(starTreeNumericType, null);
     }
 
     @Override
-    public Double performValueAggregation(Double aggregatedValue, Double segmentDocValue) {
+    protected Double performValueAggregation(Double aggregatedValue, Double segmentDocValue) {
         return Math.min(aggregatedValue, segmentDocValue);
     }
 }
diff --git a/...ch/index/compositeindex/datacube/startree/aggregators/StatelessDoubleValueAggregator.java b/...ch/index/compositeindex/datacube/startree/aggregators/StatelessDoubleValueAggregator.java
@@ -0,0 +1,75 @@
+/*
+ * 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.index.compositeindex.datacube.startree.aggregators;
+
+import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
+
+/**
+ * This is an abstract class that defines the common methods for all double value aggregators
+ * It is stateless.
+ *
+ * @opensearch.experimental
+ */
+public abstract class StatelessDoubleValueAggregator implements ValueAggregator<Double> {
+
+    protected final StarTreeNumericType starTreeNumericType;
+    protected final Double identityValue;
+
+    public StatelessDoubleValueAggregator(StarTreeNumericType starTreeNumericType, Double identityValue) {
+        this.starTreeNumericType = starTreeNumericType;
+        this.identityValue = identityValue;
+    }
+
+    @Override
+    public Double getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue) {
+        if (segmentDocValue == null) {
+            return getIdentityMetricValue();
+        }
+        return starTreeNumericType.getDoubleValue(segmentDocValue);
+    }
+
+    @Override
+    public Double mergeAggregatedValues(Double value, Double aggregatedValue) {
+        if (value == null && aggregatedValue != null) {
+            return aggregatedValue;
+        } else if (value != null && aggregatedValue == null) {
+            return value;
+        } else if (value == null) {
+            return getIdentityMetricValue();
+        }
+        return performValueAggregation(value, aggregatedValue);
+    }
+
+    @Override
+    public Double toStarTreeNumericTypeValue(Long value) {
+        try {
+            if (value == null) {
+                return getIdentityMetricValue();
+            }
+            return starTreeNumericType.getDoubleValue(value);
+        } catch (Exception e) {
+            throw new IllegalStateException("Cannot convert " + value + " to sortable aggregation type", e);
+        }
+    }
+
+    @Override
+    public Double getIdentityMetricValue() {
+        // the identity value that we return should be inline with the existing aggregations
+        return identityValue;
+    }
+
+    /**
+     * Performs min or max aggregation on the value and the segmentDocValue based on the implementation
+     *
+     * @param aggregatedValue aggregated value for the segment so far
+     * @param segmentDocValue current segment doc value
+     * @return aggregated value
+     */
+    protected abstract Double performValueAggregation(Double aggregatedValue, Double segmentDocValue);
+
+}
diff --git a/...org/opensearch/index/compositeindex/datacube/startree/aggregators/SumValueAggregator.java b/...org/opensearch/index/compositeindex/datacube/startree/aggregators/SumValueAggregator.java
@@ -7,39 +7,31 @@
  */
 package org.opensearch.index.compositeindex.datacube.startree.aggregators;
 
-import org.apache.lucene.util.NumericUtils;
-import org.opensearch.index.compositeindex.datacube.MetricStat;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 import org.opensearch.search.aggregations.metrics.CompensatedSum;
 
 /**
  * Sum value aggregator for star tree
  *
+ * <p>This implementation follows the Kahan summation algorithm to improve the accuracy
+ * of the sum by tracking and compensating for the accumulated error in each iteration.
+ *
+ * @see <a href="http://en.wikipedia.org/wiki/Kahan_summation_algorithm">Kahan Summation Algorithm</a>
+ *
  * @opensearch.experimental
  */
 public class SumValueAggregator implements ValueAggregator<Double> {
 
-    private static final StarTreeNumericType VALUE_AGGREGATOR_TYPE = StarTreeNumericType.DOUBLE;
+    private final StarTreeNumericType starTreeNumericType;
+
     private double sum = 0;
     private double compensation = 0;
     private CompensatedSum kahanSummation = new CompensatedSum(0, 0);
 
-    private final StarTreeNumericType starTreeNumericType;
-
     public SumValueAggregator(StarTreeNumericType starTreeNumericType) {
         this.starTreeNumericType = starTreeNumericType;
     }
 
-    @Override
-    public MetricStat getAggregationType() {
-        return MetricStat.SUM;
-    }
-
-    @Override
-    public StarTreeNumericType getAggregatedValueType() {
-        return VALUE_AGGREGATOR_TYPE;
-    }
-
     @Override
     public Double getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue) {
         kahanSummation.reset(0, 0);
@@ -53,6 +45,8 @@ public Double getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue)
         return kahanSummation.value();
     }
 
+    // we have overridden this method because the reset with sum and compensation helps us keep
+    // track of precision and avoids a potential loss in accuracy of sums.
     @Override
     public Double mergeAggregatedValueAndSegmentValue(Double value, Long segmentDocValue) {
         assert value == null || kahanSummation.value() == value;
@@ -94,23 +88,6 @@ public Double getInitialAggregatedValue(Double value) {
         return kahanSummation.value();
     }
 
-    @Override
-    public int getMaxAggregatedValueByteSize() {
-        return Double.BYTES;
-    }
-
-    @Override
-    public Long toLongValue(Double value) {
-        try {
-            if (value == null) {
-                return 0L;
-            }
-            return NumericUtils.doubleToSortableLong(value);
-        } catch (Exception e) {
-            throw new IllegalStateException("Cannot convert " + value + " to sortable long", e);
-        }
-    }
-
     @Override
     public Double toStarTreeNumericTypeValue(Long value) {
         try {