Skip to content

Commit ae84daa

Browse files
sarthakaggarwal97sarthakaggarwal97
committed
nit fixes
Signed-off-by: Sarthak Aggarwal <[email protected]>
1 parent 0344858 commit ae84daa

File tree

8 files changed

+122
-19
lines changed

8 files changed

+122
-19
lines changed

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/aggregators/CountValueAggregator.java

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,17 @@ class CountValueAggregator implements ValueAggregator<Long> {
1818

1919
public static final long DEFAULT_INITIAL_VALUE = 1L;
2020
private final StarTreeNumericType starTreeNumericType;
21+
private final StarTreeNumericType VALUE_AGGREGATOR_TYPE = StarTreeNumericType.LONG;
2122

2223
public CountValueAggregator(StarTreeNumericType starTreeNumericType) {
2324
this.starTreeNumericType = starTreeNumericType;
2425
}
2526

27+
@Override
28+
public StarTreeNumericType getAggregatedValueType() {
29+
return VALUE_AGGREGATOR_TYPE;
30+
}
31+
2632
@Override
2733
public Long getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue) {
2834

@@ -35,10 +41,11 @@ public Long getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue) {
3541

3642
@Override
3743
public Long mergeAggregatedValueAndSegmentValue(Long value, Long segmentDocValue) {
38-
if (value == null) {
39-
return getIdentityMetricValue();
44+
assert value != null;
45+
if (segmentDocValue != null) {
46+
return value + 1;
4047
}
41-
return value + 1;
48+
return value;
4249
}
4350

4451
@Override

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/aggregators/StatelessDoubleValueAggregator.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,18 @@ abstract class StatelessDoubleValueAggregator implements ValueAggregator<Double>
1919

2020
protected final StarTreeNumericType starTreeNumericType;
2121
protected final Double identityValue;
22+
private final StarTreeNumericType VALUE_AGGREGATOR_TYPE = StarTreeNumericType.DOUBLE;
2223

2324
public StatelessDoubleValueAggregator(StarTreeNumericType starTreeNumericType, Double identityValue) {
2425
this.starTreeNumericType = starTreeNumericType;
2526
this.identityValue = identityValue;
2627
}
2728

29+
@Override
30+
public StarTreeNumericType getAggregatedValueType() {
31+
return VALUE_AGGREGATOR_TYPE;
32+
}
33+
2834
@Override
2935
public Double getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue) {
3036
if (segmentDocValue == null) {

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/aggregators/SumValueAggregator.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,19 @@
2323
class SumValueAggregator implements ValueAggregator<Double> {
2424

2525
private final StarTreeNumericType starTreeNumericType;
26+
private final StarTreeNumericType VALUE_AGGREGATOR_TYPE = StarTreeNumericType.DOUBLE;
2627

2728
private CompensatedSum kahanSummation = new CompensatedSum(0, 0);
2829

2930
public SumValueAggregator(StarTreeNumericType starTreeNumericType) {
3031
this.starTreeNumericType = starTreeNumericType;
3132
}
3233

34+
@Override
35+
public StarTreeNumericType getAggregatedValueType() {
36+
return VALUE_AGGREGATOR_TYPE;
37+
}
38+
3339
@Override
3440
public Double getInitialAggregatedValueForSegmentDocValue(Long segmentDocValue) {
3541
kahanSummation.reset(0, 0);

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/aggregators/ValueAggregator.java

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,20 @@
77
*/
88
package org.opensearch.index.compositeindex.datacube.startree.aggregators;
99

10+
import org.opensearch.index.compositeindex.datacube.startree.aggregators.numerictype.StarTreeNumericType;
11+
1012
/**
1113
* A value aggregator that pre-aggregates on the input values for a specific type of aggregation.
1214
*
1315
* @opensearch.experimental
1416
*/
1517
public interface ValueAggregator<A> {
1618

19+
/**
20+
* Returns the data type of the aggregated value.
21+
*/
22+
StarTreeNumericType getAggregatedValueType();
23+
1724
/**
1825
* Returns the initial aggregated value.
1926
*/

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/AbstractDocumentsFileManager.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -175,12 +175,7 @@ protected long readMetrics(RandomAccessInput input, long offset, int numMetrics,
175175
throw new IllegalStateException("Unsupported metric type");
176176
}
177177
}
178-
offset += StarTreeDocumentBitSetUtil.readBitSet(
179-
input,
180-
offset,
181-
metrics,
182-
index -> metricAggregatorInfos.get(index).getValueAggregators().getIdentityMetricValue()
183-
);
178+
offset += StarTreeDocumentBitSetUtil.readBitSet(input, offset, metrics, index -> null);
184179
return offset;
185180
}
186181

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/builder/BaseStarTreeBuilder.java

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -474,14 +474,10 @@ public List<SequentialDocValuesIterator> getMetricReaders(SegmentWriteState stat
474474
if (metricFieldInfo == null) {
475475
metricFieldInfo = getFieldInfo(metric.getField());
476476
}
477-
// TODO
478-
// if (metricStat != MetricStat.COUNT) {
479-
// Need not initialize the metric reader for COUNT metric type
477+
480478
SequentialDocValuesIterator metricReader = new SequentialDocValuesIterator(
481479
fieldProducerMap.get(metricFieldInfo.name).getSortedNumeric(metricFieldInfo)
482480
);
483-
// }
484-
485481
metricReaders.add(metricReader);
486482
}
487483
}

‎server/src/test/java/org/opensearch/index/compositeindex/datacube/startree/aggregators/AbstractValueAggregatorTests.java

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,11 @@ public void testGetInitialAggregatedValueForSegmentDocNullValue() {
4848
}
4949

5050
public void testMergeAggregatedNullValueAndSegmentNullValue() {
51-
assertEquals(aggregator.getIdentityMetricValue(), aggregator.mergeAggregatedValueAndSegmentValue(null, null));
51+
if (aggregator instanceof CountValueAggregator) {
52+
assertThrows(AssertionError.class, () -> aggregator.mergeAggregatedValueAndSegmentValue(null, null));
53+
} else {
54+
assertEquals(aggregator.getIdentityMetricValue(), aggregator.mergeAggregatedValueAndSegmentValue(null, null));
55+
}
5256
}
5357

5458
public void testMergeAggregatedNullValues() {

‎server/src/test/java/org/opensearch/index/compositeindex/datacube/startree/builder/AbstractStarTreeBuilderTests.java

Lines changed: 86 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -392,8 +392,14 @@ public void test_sortAndAggregateStarTreeDocuments_nullAndMinusOneInDimensionFie
392392
new Long[] { 2L, null, 3L, 4L },
393393
new Double[] { 12.0, null, randomDouble(), 8.0, 20.0 }
394394
);
395-
starTreeDocuments[1] = new StarTreeDocument(new Long[] { null, 4L, 2L, 1L }, new Double[] { 10.0, null, randomDouble(), 12.0, 10.0 });
396-
starTreeDocuments[2] = new StarTreeDocument(new Long[] { null, 4L, 2L, 1L }, new Double[] { 14.0, null, randomDouble(), 6.0, 24.0 });
395+
starTreeDocuments[1] = new StarTreeDocument(
396+
new Long[] { null, 4L, 2L, 1L },
397+
new Double[] { 10.0, null, randomDouble(), 12.0, 10.0 }
398+
);
399+
starTreeDocuments[2] = new StarTreeDocument(
400+
new Long[] { null, 4L, 2L, 1L },
401+
new Double[] { 14.0, null, randomDouble(), 6.0, 24.0 }
402+
);
397403
starTreeDocuments[3] = new StarTreeDocument(new Long[] { 2L, null, 3L, 4L }, new Double[] { 9.0, null, randomDouble(), 9.0, 12.0 });
398404
starTreeDocuments[4] = new StarTreeDocument(new Long[] { -1L, 4L, 2L, 1L }, new Double[] { 11.0, null, randomDouble(), 8.0, 13.0 });
399405

@@ -453,7 +459,7 @@ public void test_sortAndAggregateStarTreeDocuments_nullDimensionsAndNullMetrics(
453459
starTreeDocuments[4] = new StarTreeDocument(new Long[] { null, null, null, null }, new Double[] { null, null, null, null, null });
454460

455461
List<StarTreeDocument> inorderStarTreeDocuments = List.of(
456-
new StarTreeDocument(new Long[] { null, null, null, null }, new Object[] { 0.0, 0.0, 5L })
462+
new StarTreeDocument(new Long[] { null, null, null, null }, new Object[] { 0.0, 0.0, 0L, null, null })
457463
);
458464
Iterator<StarTreeDocument> expectedStarTreeDocumentIterator = inorderStarTreeDocuments.iterator();
459465

@@ -469,11 +475,87 @@ public void test_sortAndAggregateStarTreeDocuments_nullDimensionsAndNullMetrics(
469475
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[2])
470476
: null;
471477
Long metric4 = starTreeDocuments[i].metrics[3] != null
472-
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[2])
478+
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[3])
473479
: null;
474480
Long metric5 = starTreeDocuments[i].metrics[4] != null
481+
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[4])
482+
: null;
483+
segmentStarTreeDocuments[i] = new StarTreeDocument(
484+
starTreeDocuments[i].dimensions,
485+
new Object[] { metric1, metric2, metric3, metric4, metric5 }
486+
);
487+
}
488+
SequentialDocValuesIterator[] dimsIterators = getDimensionIterators(segmentStarTreeDocuments);
489+
List<SequentialDocValuesIterator> metricsIterators = getMetricIterators(segmentStarTreeDocuments);
490+
builder = getStarTreeBuilder(compositeField, writeState, mapperService);
491+
Iterator<StarTreeDocument> segmentStarTreeDocumentIterator = builder.sortAndAggregateSegmentDocuments(
492+
dimsIterators,
493+
metricsIterators
494+
);
495+
496+
while (segmentStarTreeDocumentIterator.hasNext() && expectedStarTreeDocumentIterator.hasNext()) {
497+
StarTreeDocument resultStarTreeDocument = segmentStarTreeDocumentIterator.next();
498+
StarTreeDocument expectedStarTreeDocument = expectedStarTreeDocumentIterator.next();
499+
assertEquals(expectedStarTreeDocument.dimensions[0], resultStarTreeDocument.dimensions[0]);
500+
assertEquals(expectedStarTreeDocument.dimensions[1], resultStarTreeDocument.dimensions[1]);
501+
assertEquals(expectedStarTreeDocument.dimensions[2], resultStarTreeDocument.dimensions[2]);
502+
assertEquals(expectedStarTreeDocument.dimensions[3], resultStarTreeDocument.dimensions[3]);
503+
assertEquals(expectedStarTreeDocument.metrics[0], resultStarTreeDocument.metrics[0]);
504+
assertEquals(expectedStarTreeDocument.metrics[1], resultStarTreeDocument.metrics[1]);
505+
assertEquals(expectedStarTreeDocument.metrics[2], resultStarTreeDocument.metrics[2]);
506+
assertEquals(expectedStarTreeDocument.metrics[3], resultStarTreeDocument.metrics[3]);
507+
assertEquals(expectedStarTreeDocument.metrics[4], resultStarTreeDocument.metrics[4]);
508+
}
509+
builder.build(segmentStarTreeDocumentIterator);
510+
validateStarTree(builder.getRootNode(), 4, 1, builder.getStarTreeDocuments());
511+
}
512+
513+
public void test_sortAndAggregateStarTreeDocuments_nullDimensionsAndFewNullMetrics() throws IOException {
514+
int noOfStarTreeDocuments = 5;
515+
StarTreeDocument[] starTreeDocuments = new StarTreeDocument[noOfStarTreeDocuments];
516+
517+
double sumValue = randomDouble();
518+
double minValue = randomDouble();
519+
double maxValue = randomDouble();
520+
521+
// Setting second metric iterator as empty sorted numeric , indicating a metric field is null
522+
starTreeDocuments[0] = new StarTreeDocument(
523+
new Long[] { null, null, null, null },
524+
new Double[] { null, null, randomDouble(), null, maxValue }
525+
);
526+
starTreeDocuments[1] = new StarTreeDocument(new Long[] { null, null, null, null }, new Double[] { null, null, null, null, null });
527+
starTreeDocuments[2] = new StarTreeDocument(
528+
new Long[] { null, null, null, null },
529+
new Double[] { null, null, null, minValue, null }
530+
);
531+
starTreeDocuments[3] = new StarTreeDocument(new Long[] { null, null, null, null }, new Double[] { null, null, null, null, null });
532+
starTreeDocuments[4] = new StarTreeDocument(
533+
new Long[] { null, null, null, null },
534+
new Double[] { sumValue, null, randomDouble(), null, null }
535+
);
536+
537+
List<StarTreeDocument> inorderStarTreeDocuments = List.of(
538+
new StarTreeDocument(new Long[] { null, null, null, null }, new Object[] { sumValue, 0.0, 2L, minValue, maxValue })
539+
);
540+
Iterator<StarTreeDocument> expectedStarTreeDocumentIterator = inorderStarTreeDocuments.iterator();
541+
542+
StarTreeDocument[] segmentStarTreeDocuments = new StarTreeDocument[noOfStarTreeDocuments];
543+
for (int i = 0; i < noOfStarTreeDocuments; i++) {
544+
Long metric1 = starTreeDocuments[i].metrics[0] != null
545+
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[0])
546+
: null;
547+
Long metric2 = starTreeDocuments[i].metrics[1] != null
548+
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[1])
549+
: null;
550+
Long metric3 = starTreeDocuments[i].metrics[2] != null
475551
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[2])
476552
: null;
553+
Long metric4 = starTreeDocuments[i].metrics[3] != null
554+
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[3])
555+
: null;
556+
Long metric5 = starTreeDocuments[i].metrics[4] != null
557+
? NumericUtils.doubleToSortableLong((Double) starTreeDocuments[i].metrics[4])
558+
: null;
477559
segmentStarTreeDocuments[i] = new StarTreeDocument(
478560
starTreeDocuments[i].dimensions,
479561
new Object[] { metric1, metric2, metric3, metric4, metric5 }

0 commit comments

Comments
 (0)