addressing comments

Signed-off-by: Sarthak Aggarwal <[email protected]>
opensearch-project · Aug 16, 2024 · 8d27f50 · 8d27f50
1 parent ea7a0d4
commit 8d27f50
Show file tree

Hide file tree

Showing 6 changed files with 11 additions and 113 deletions.
diff --git a/...rc/test/java/org/opensearch/index/compositeindex/datacube/startree/StarTreeTestUtils.java b/...rc/test/java/org/opensearch/index/compositeindex/datacube/startree/StarTreeTestUtils.java
diff --git a/...arch/index/compositeindex/datacube/startree/aggregators/AbstractValueAggregatorTests.java b/...arch/index/compositeindex/datacube/startree/aggregators/AbstractValueAggregatorTests.java
@@ -10,7 +10,6 @@
 
 import com.carrotsearch.randomizedtesting.annotations.ParametersFactory;
 
-import org.opensearch.index.compositeindex.datacube.startree.StarTreeTestUtils;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 import org.opensearch.test.OpenSearchTestCase;
 import org.junit.Before;
@@ -70,7 +69,7 @@ public void testGetInitialAggregatedValueForSegmentDocValue() {
             assertEquals(CountValueAggregator.DEFAULT_INITIAL_VALUE, aggregator.getInitialAggregatedValueForSegmentDocValue(randomLong()));
         } else {
             assertEquals(
-                StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType),
+                starTreeNumericType.getDoubleValue(randomLong),
                 aggregator.getInitialAggregatedValueForSegmentDocValue(randomLong)
             );
         }

diff --git a/...pensearch/index/compositeindex/datacube/startree/aggregators/MaxValueAggregatorTests.java b/...pensearch/index/compositeindex/datacube/startree/aggregators/MaxValueAggregatorTests.java
@@ -9,7 +9,6 @@
 package org.opensearch.index.compositeindex.datacube.startree.aggregators;
 
 import org.apache.lucene.util.NumericUtils;
-import org.opensearch.index.compositeindex.datacube.startree.StarTreeTestUtils;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 
 public class MaxValueAggregatorTests extends AbstractValueAggregatorTests {
@@ -24,21 +23,13 @@ public void testMergeAggregatedValueAndSegmentValue() {
         Long randomLong = randomLong();
         double randomDouble = randomDouble();
         assertEquals(
-            Math.max(StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType), randomDouble),
+            Math.max(starTreeNumericType.getDoubleValue(randomLong), randomDouble),
             aggregator.mergeAggregatedValueAndSegmentValue(randomDouble, randomLong),
             0.0
         );
-        assertEquals(
-            StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType),
-            aggregator.mergeAggregatedValueAndSegmentValue(null, randomLong),
-            0.0
-        );
+        assertEquals(starTreeNumericType.getDoubleValue(randomLong), aggregator.mergeAggregatedValueAndSegmentValue(null, randomLong), 0.0);
         assertEquals(randomDouble, aggregator.mergeAggregatedValueAndSegmentValue(randomDouble, null), 0.0);
-        assertEquals(
-            Math.max(2.0, StarTreeTestUtils.toStarTreeNumericTypeValue(3L, starTreeNumericType)),
-            aggregator.mergeAggregatedValueAndSegmentValue(2.0, 3L),
-            0.0
-        );
+        assertEquals(Math.max(2.0, starTreeNumericType.getDoubleValue(3L)), aggregator.mergeAggregatedValueAndSegmentValue(2.0, 3L), 0.0);
     }
 
     public void testMergeAggregatedValues() {

diff --git a/...pensearch/index/compositeindex/datacube/startree/aggregators/MinValueAggregatorTests.java b/...pensearch/index/compositeindex/datacube/startree/aggregators/MinValueAggregatorTests.java
@@ -9,7 +9,6 @@
 package org.opensearch.index.compositeindex.datacube.startree.aggregators;
 
 import org.apache.lucene.util.NumericUtils;
-import org.opensearch.index.compositeindex.datacube.startree.StarTreeTestUtils;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 
 public class MinValueAggregatorTests extends AbstractValueAggregatorTests {
@@ -23,21 +22,13 @@ public void testMergeAggregatedValueAndSegmentValue() {
         Long randomLong = randomLong();
         double randomDouble = randomDouble();
         assertEquals(
-            Math.min(StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType), randomDouble),
+            Math.min(starTreeNumericType.getDoubleValue(randomLong), randomDouble),
             aggregator.mergeAggregatedValueAndSegmentValue(randomDouble, randomLong),
             0.0
         );
-        assertEquals(
-            StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType),
-            aggregator.mergeAggregatedValueAndSegmentValue(null, randomLong),
-            0.0
-        );
+        assertEquals(starTreeNumericType.getDoubleValue(randomLong), aggregator.mergeAggregatedValueAndSegmentValue(null, randomLong), 0.0);
         assertEquals(randomDouble, aggregator.mergeAggregatedValueAndSegmentValue(randomDouble, null), 0.0);
-        assertEquals(
-            Math.min(2.0, StarTreeTestUtils.toStarTreeNumericTypeValue(3L, starTreeNumericType)),
-            aggregator.mergeAggregatedValueAndSegmentValue(2.0, 3L),
-            0.0
-        );
+        assertEquals(Math.min(2.0, starTreeNumericType.getDoubleValue(3L)), aggregator.mergeAggregatedValueAndSegmentValue(2.0, 3L), 0.0);
     }
 
     public void testMergeAggregatedValues() {

diff --git a/...pensearch/index/compositeindex/datacube/startree/aggregators/SumValueAggregatorTests.java b/...pensearch/index/compositeindex/datacube/startree/aggregators/SumValueAggregatorTests.java
@@ -8,7 +8,6 @@
 
 package org.opensearch.index.compositeindex.datacube.startree.aggregators;
 
-import org.opensearch.index.compositeindex.datacube.startree.StarTreeTestUtils;
 import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
 
 public class SumValueAggregatorTests extends AbstractValueAggregatorTests {
@@ -30,7 +29,7 @@ public void testMergeAggregatedValueAndSegmentValue() {
         Long randomLong = randomLong();
         aggregator.getInitialAggregatedValue(randomDouble);
         assertEquals(
-            randomDouble + StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType),
+            randomDouble + starTreeNumericType.getDoubleValue(randomLong),
             aggregator.mergeAggregatedValueAndSegmentValue(randomDouble, randomLong),
             0.0
         );
@@ -42,7 +41,7 @@ public void testMergeAggregatedValueAndSegmentValue_nullSegmentDocValue() {
         aggregator.getInitialAggregatedValue(randomDouble1);
         assertEquals(randomDouble1, aggregator.mergeAggregatedValueAndSegmentValue(randomDouble1, null), 0.0);
         assertEquals(
-            randomDouble1 + StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType),
+            randomDouble1 + starTreeNumericType.getDoubleValue(randomLong),
             aggregator.mergeAggregatedValueAndSegmentValue(randomDouble1, randomLong),
             0.0
         );
@@ -51,11 +50,7 @@ public void testMergeAggregatedValueAndSegmentValue_nullSegmentDocValue() {
     public void testMergeAggregatedValueAndSegmentValue_nullInitialDocValue() {
         Long randomLong = randomLong();
         aggregator.getInitialAggregatedValue(null);
-        assertEquals(
-            StarTreeTestUtils.toStarTreeNumericTypeValue(randomLong, starTreeNumericType),
-            aggregator.mergeAggregatedValueAndSegmentValue(null, randomLong),
-            0.0
-        );
+        assertEquals(starTreeNumericType.getDoubleValue(randomLong), aggregator.mergeAggregatedValueAndSegmentValue(null, randomLong), 0.0);
     }
 
     public void testMergeAggregatedValues() {

diff --git a/...ensearch/index/compositeindex/datacube/startree/builder/AbstractStarTreeBuilderTests.java b/...ensearch/index/compositeindex/datacube/startree/builder/AbstractStarTreeBuilderTests.java
@@ -1509,22 +1509,6 @@ private static StarTreeField getStarTreeFieldWithMultipleMetrics() {
     }
 
     public void testMergeFlowWithSum_randomNumberTypes() throws IOException {
-        List<Long> dimList = List.of(0L, 1L, 3L, 4L, 5L, 6L);
-        List<Integer> docsWithField = List.of(0, 1, 3, 4, 5, 6);
-        List<Long> dimList2 = List.of(0L, 1L, 2L, 3L, 4L, 5L, -1L);
-        List<Integer> docsWithField2 = List.of(0, 1, 2, 3, 4, 5, 6);
-
-        List<Long> metricsList = List.of(
-            getLongFromDouble(0.0),
-            getLongFromDouble(10.0),
-            getLongFromDouble(20.0),
-            getLongFromDouble(30.0),
-            getLongFromDouble(40.0),
-            getLongFromDouble(50.0),
-            getLongFromDouble(60.0)
-
-        );
-        List<Integer> metricsWithField = List.of(0, 1, 2, 3, 4, 5, 6);
 
         DocumentMapper documentMapper = mock(DocumentMapper.class);
         when(mapperService.documentMapper()).thenReturn(documentMapper);
@@ -1555,48 +1539,8 @@ public void testMergeFlowWithSum_randomNumberTypes() throws IOException {
             null
         );
         when(documentMapper.mappers()).thenReturn(fieldMappers);
+        testMergeFlowWithSum();
 
-        StarTreeField sf = getStarTreeField(MetricStat.SUM);
-        StarTreeValues starTreeValues = getStarTreeValues(
-            getSortedNumericMock(dimList, docsWithField),
-            getSortedNumericMock(dimList2, docsWithField2),
-            getSortedNumericMock(metricsList, metricsWithField),
-            sf,
-            "6"
-        );
-
-        StarTreeValues starTreeValues2 = getStarTreeValues(
-            getSortedNumericMock(dimList, docsWithField),
-            getSortedNumericMock(dimList2, docsWithField2),
-            getSortedNumericMock(metricsList, metricsWithField),
-            sf,
-            "6"
-        );
-        builder = getStarTreeBuilder(sf, getWriteState(6), mapperService);
-        Iterator<StarTreeDocument> starTreeDocumentIterator = builder.mergeStarTrees(List.of(starTreeValues, starTreeValues2));
-        /**
-         * Asserting following dim / metrics [ dim1, dim2 / Sum [ metric] ]
-         * [0, 0] | [0.0]
-         * [1, 1] | [20.0]
-         * [3, 3] | [60.0]
-         * [4, 4] | [80.0]
-         * [5, 5] | [100.0]
-         * [null, 2] | [40.0]
-         * ------------------ We only take non star docs
-         * [6,-1] | [120.0]
-         */
-        int count = 0;
-        while (starTreeDocumentIterator.hasNext()) {
-            count++;
-            StarTreeDocument starTreeDocument = starTreeDocumentIterator.next();
-            assertEquals(
-                starTreeDocument.dimensions[0] != null ? starTreeDocument.dimensions[0] * 2 * 10.0 : 40.0,
-                starTreeDocument.metrics[0]
-            );
-        }
-        assertEquals(6, count);
-        builder.build(starTreeDocumentIterator);
-        validateStarTree(builder.getRootNode(), 2, 1, builder.getStarTreeDocuments());
     }
 
     public void testMergeFlowWithSum() throws IOException {