Skip to content

Commit 71a771e

Browse files
bharath-techiebharath-techie
authored
[Star tree][Bug] Fix for derived metrics (opensearch-project#15640)
* Fix for derived metrics Signed-off-by: Bharathwaj G <bharath78910@gmail.com> * fixes for byte Signed-off-by: Bharathwaj G <bharath78910@gmail.com> --------- Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
1 parent 4f57d6a commit 71a771e

File tree

12 files changed

+163
-44
lines changed

12 files changed

+163
-44
lines changed

‎server/src/main/java/org/opensearch/index/codec/composite/composite99/Composite99DocValuesReader.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,7 @@ public Composite99DocValuesReader(DocValuesProducer producer, SegmentReadState r
164164

165165
// adding metric fields
166166
for (Metric metric : starTreeMetadata.getMetrics()) {
167-
for (MetricStat metricStat : metric.getMetrics()) {
167+
for (MetricStat metricStat : metric.getBaseMetrics()) {
168168
fields.add(
169169
fullyQualifiedFieldNameForStarTreeMetricsDocValues(
170170
compositeFieldName,

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/Metric.java

+16
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
import org.opensearch.core.xcontent.XContentBuilder;
1414

1515
import java.io.IOException;
16+
import java.util.ArrayList;
1617
import java.util.List;
1718
import java.util.Objects;
1819

@@ -23,10 +24,18 @@
2324
public class Metric implements ToXContent {
2425
private final String field;
2526
private final List<MetricStat> metrics;
27+
private final List<MetricStat> baseMetrics;
2628

2729
public Metric(String field, List<MetricStat> metrics) {
2830
this.field = field;
2931
this.metrics = metrics;
32+
this.baseMetrics = new ArrayList<>();
33+
for (MetricStat metricStat : metrics) {
34+
if (metricStat.isDerivedMetric()) {
35+
continue;
36+
}
37+
baseMetrics.add(metricStat);
38+
}
3039
}
3140

3241
public String getField() {
@@ -37,6 +46,13 @@ public List<MetricStat> getMetrics() {
3746
return metrics;
3847
}
3948

49+
/**
50+
* Returns only the base metrics
51+
*/
52+
public List<MetricStat> getBaseMetrics() {
53+
return baseMetrics;
54+
}
55+
4056
@Override
4157
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
4258
builder.startObject();

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

+2-5
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,7 @@ public List<MetricAggregatorInfo> generateMetricAggregatorInfos(MapperService ma
150150
metricAggregatorInfos.add(metricAggregatorInfo);
151151
continue;
152152
}
153-
for (MetricStat metricStat : metric.getMetrics()) {
154-
if (metricStat.isDerivedMetric()) {
155-
continue;
156-
}
153+
for (MetricStat metricStat : metric.getBaseMetrics()) {
157154
FieldValueConverter fieldValueConverter;
158155
Mapper fieldMapper = mapperService.documentMapper().mappers().getMapper(metric.getField());
159156
if (fieldMapper instanceof FieldMapper && ((FieldMapper) fieldMapper).fieldType() instanceof FieldValueConverter) {
@@ -185,7 +182,7 @@ public List<SequentialDocValuesIterator> getMetricReaders(SegmentWriteState stat
185182

186183
List<SequentialDocValuesIterator> metricReaders = new ArrayList<>();
187184
for (Metric metric : this.starTreeField.getMetrics()) {
188-
for (MetricStat metricStat : metric.getMetrics()) {
185+
for (MetricStat metricStat : metric.getBaseMetrics()) {
189186
SequentialDocValuesIterator metricReader;
190187
FieldInfo metricFieldInfo = state.fieldInfos.fieldInfo(metric.getField());
191188
if (metricStat.equals(MetricStat.DOC_COUNT)) {

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -158,7 +158,7 @@ Iterator<StarTreeDocument> mergeStarTrees(List<StarTreeValues> starTreeValuesSub
158158
List<SequentialDocValuesIterator> metricReaders = new ArrayList<>();
159159
// get doc id set iterators for metrics
160160
for (Metric metric : starTreeValues.getStarTreeField().getMetrics()) {
161-
for (MetricStat metricStat : metric.getMetrics()) {
161+
for (MetricStat metricStat : metric.getBaseMetrics()) {
162162
String metricFullName = fullyQualifiedFieldNameForStarTreeMetricsDocValues(
163163
starTreeValues.getStarTreeField().getName(),
164164
metric.getField(),

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -144,7 +144,7 @@ StarTreeDocument[] getSegmentsStarTreeDocuments(List<StarTreeValues> starTreeVal
144144
List<SequentialDocValuesIterator> metricReaders = new ArrayList<>();
145145
// get doc id set iterators for metrics
146146
for (Metric metric : starTreeValues.getStarTreeField().getMetrics()) {
147-
for (MetricStat metricStat : metric.getMetrics()) {
147+
for (MetricStat metricStat : metric.getBaseMetrics()) {
148148
String metricFullName = fullyQualifiedFieldNameForStarTreeMetricsDocValues(
149149
starTreeValues.getStarTreeField().getName(),
150150
metric.getField(),

‎server/src/main/java/org/opensearch/index/compositeindex/datacube/startree/fileformats/meta/StarTreeMetadata.java

+25-4
Original file line numberDiff line numberDiff line change
@@ -220,16 +220,37 @@ private int readMetricsCount() throws IOException {
220220
private List<Metric> readMetricEntries() throws IOException {
221221
int metricCount = readMetricsCount();
222222

223-
Map<String, Metric> starTreeMetricMap = new LinkedHashMap<>();
223+
Map<String, List<MetricStat>> starTreeMetricStatMap = new LinkedHashMap<>();
224224
for (int i = 0; i < metricCount; i++) {
225225
String metricName = meta.readString();
226226
int metricStatOrdinal = meta.readVInt();
227227
MetricStat metricStat = MetricStat.fromMetricOrdinal(metricStatOrdinal);
228-
Metric metric = starTreeMetricMap.computeIfAbsent(metricName, field -> new Metric(field, new ArrayList<>()));
229-
metric.getMetrics().add(metricStat);
228+
List<MetricStat> metricStats = starTreeMetricStatMap.computeIfAbsent(metricName, field -> new ArrayList<>());
229+
metricStats.add(metricStat);
230230
}
231+
List<Metric> starTreeMetricMap = new ArrayList<>();
232+
for (Map.Entry<String, List<MetricStat>> metricStatsEntry : starTreeMetricStatMap.entrySet()) {
233+
addEligibleDerivedMetrics(metricStatsEntry.getValue());
234+
starTreeMetricMap.add(new Metric(metricStatsEntry.getKey(), metricStatsEntry.getValue()));
231235

232-
return new ArrayList<>(starTreeMetricMap.values());
236+
}
237+
return starTreeMetricMap;
238+
}
239+
240+
/**
241+
* Add derived metrics if all associated base metrics are present
242+
*/
243+
private void addEligibleDerivedMetrics(List<MetricStat> metricStatsList) {
244+
Set<MetricStat> metricStatsSet = new HashSet<>(metricStatsList);
245+
for (MetricStat metric : MetricStat.values()) {
246+
if (metric.isDerivedMetric() && !metricStatsSet.contains(metric)) {
247+
List<MetricStat> sourceMetrics = metric.getBaseMetrics();
248+
if (metricStatsSet.containsAll(sourceMetrics)) {
249+
metricStatsList.add(metric);
250+
metricStatsSet.add(metric);
251+
}
252+
}
253+
}
233254
}
234255

235256
private int readSegmentAggregatedDocCount() throws IOException {

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ public StarTreeValues(
171171

172172
// get doc id set iterators for metrics
173173
for (Metric metric : starTreeMetadata.getMetrics()) {
174-
for (MetricStat metricStat : metric.getMetrics()) {
174+
for (MetricStat metricStat : metric.getBaseMetrics()) {
175175
String metricFullName = fullyQualifiedFieldNameForStarTreeMetricsDocValues(
176176
starTreeField.getName(),
177177
metric.getField(),

‎server/src/main/java/org/opensearch/index/mapper/NumberFieldMapper.java

+1-3
Original file line numberDiff line numberDiff line change
@@ -653,9 +653,7 @@ public byte[] encodePoint(Number value) {
653653

654654
@Override
655655
public double toDoubleValue(long value) {
656-
byte[] bytes = new byte[8];
657-
NumericUtils.longToSortableBytes(value, bytes, 0);
658-
return NumericUtils.sortableLongToDouble(NumericUtils.sortableBytesToLong(bytes, 0));
656+
return objectToDouble(value);
659657
}
660658

661659
@Override

‎server/src/main/java/org/opensearch/index/mapper/StarTreeMapper.java

+1-5
Original file line numberDiff line numberDiff line change
@@ -287,11 +287,7 @@ private List<Metric> buildMetrics(String fieldName, Map<String, Object> map, Map
287287
}
288288
int numBaseMetrics = 0;
289289
for (Metric metric : metrics) {
290-
for (MetricStat metricStat : metric.getMetrics()) {
291-
if (metricStat.isDerivedMetric() == false) {
292-
numBaseMetrics++;
293-
}
294-
}
290+
numBaseMetrics += metric.getBaseMetrics().size();
295291
}
296292
if (numBaseMetrics > context.getSettings()
297293
.getAsInt(

‎server/src/test/java/org/opensearch/index/codec/composite99/datacube/startree/StarTreeDocValuesFormatTests.java

+60-9
Original file line numberDiff line numberDiff line change
@@ -119,23 +119,23 @@ public void testStarTreeDocValues() throws IOException {
119119
Document doc = new Document();
120120
doc.add(new SortedNumericDocValuesField("sndv", 1));
121121
doc.add(new SortedNumericDocValuesField("dv", 1));
122-
doc.add(new SortedNumericDocValuesField("field", 1));
122+
doc.add(new SortedNumericDocValuesField("field", -1));
123123
iw.addDocument(doc);
124124
doc = new Document();
125125
doc.add(new SortedNumericDocValuesField("sndv", 1));
126126
doc.add(new SortedNumericDocValuesField("dv", 1));
127-
doc.add(new SortedNumericDocValuesField("field", 1));
127+
doc.add(new SortedNumericDocValuesField("field", -1));
128128
iw.addDocument(doc);
129129
doc = new Document();
130130
iw.forceMerge(1);
131131
doc.add(new SortedNumericDocValuesField("sndv", 2));
132132
doc.add(new SortedNumericDocValuesField("dv", 2));
133-
doc.add(new SortedNumericDocValuesField("field", 2));
133+
doc.add(new SortedNumericDocValuesField("field", -2));
134134
iw.addDocument(doc);
135135
doc = new Document();
136136
doc.add(new SortedNumericDocValuesField("sndv", 2));
137137
doc.add(new SortedNumericDocValuesField("dv", 2));
138-
doc.add(new SortedNumericDocValuesField("field", 2));
138+
doc.add(new SortedNumericDocValuesField("field", -2));
139139
iw.addDocument(doc);
140140
iw.forceMerge(1);
141141
iw.close();
@@ -144,11 +144,39 @@ public void testStarTreeDocValues() throws IOException {
144144
TestUtil.checkReader(ir);
145145
assertEquals(1, ir.leaves().size());
146146

147+
// Segment documents
148+
/**
149+
* sndv dv field
150+
* [1, 1, -1]
151+
* [1, 1, -1]
152+
* [2, 2, -2]
153+
* [2, 2, -2]
154+
*/
155+
// Star tree docuements
156+
/**
157+
* sndv dv | [ sum, value_count, min, max[field]] , [ sum, value_count, min, max[sndv]], doc_count
158+
* [1, 1] | [-2.0, 2.0, -1.0, -1.0, 2.0, 2.0, 1.0, 1.0, 2.0]
159+
* [2, 2] | [-4.0, 2.0, -2.0, -2.0, 4.0, 2.0, 2.0, 2.0, 2.0]
160+
* [null, 1] | [-2.0, 2.0, -1.0, -1.0, 2.0, 2.0, 1.0, 1.0, 2.0]
161+
* [null, 2] | [-4.0, 2.0, -2.0, -2.0, 4.0, 2.0, 2.0, 2.0, 2.0]
162+
*/
147163
StarTreeDocument[] expectedStarTreeDocuments = new StarTreeDocument[4];
148-
expectedStarTreeDocuments[0] = new StarTreeDocument(new Long[] { 1L, 1L }, new Double[] { 2.0, 2.0, 2.0 });
149-
expectedStarTreeDocuments[1] = new StarTreeDocument(new Long[] { 2L, 2L }, new Double[] { 4.0, 2.0, 4.0 });
150-
expectedStarTreeDocuments[2] = new StarTreeDocument(new Long[] { null, 1L }, new Double[] { 2.0, 2.0, 2.0 });
151-
expectedStarTreeDocuments[3] = new StarTreeDocument(new Long[] { null, 2L }, new Double[] { 4.0, 2.0, 4.0 });
164+
expectedStarTreeDocuments[0] = new StarTreeDocument(
165+
new Long[] { 1L, 1L },
166+
new Double[] { -2.0, 2.0, -1.0, -1.0, 2.0, 2.0, 1.0, 1.0, 2.0 }
167+
);
168+
expectedStarTreeDocuments[1] = new StarTreeDocument(
169+
new Long[] { 2L, 2L },
170+
new Double[] { -4.0, 2.0, -2.0, -2.0, 4.0, 2.0, 2.0, 2.0, 2.0 }
171+
);
172+
expectedStarTreeDocuments[2] = new StarTreeDocument(
173+
new Long[] { null, 1L },
174+
new Double[] { -2.0, 2.0, -1.0, -1.0, 2.0, 2.0, 1.0, 1.0, 2.0 }
175+
);
176+
expectedStarTreeDocuments[3] = new StarTreeDocument(
177+
new Long[] { null, 2L },
178+
new Double[] { -4.0, 2.0, -2.0, -2.0, 4.0, 2.0, 2.0, 2.0, 2.0 }
179+
);
152180

153181
for (LeafReaderContext context : ir.leaves()) {
154182
SegmentReader reader = Lucene.segmentReader(context.reader());
@@ -159,7 +187,17 @@ public void testStarTreeDocValues() throws IOException {
159187
StarTreeValues starTreeValues = (StarTreeValues) starTreeDocValuesReader.getCompositeIndexValues(compositeIndexFieldInfo);
160188
StarTreeDocument[] starTreeDocuments = StarTreeTestUtils.getSegmentsStarTreeDocuments(
161189
List.of(starTreeValues),
162-
List.of(NumberFieldMapper.NumberType.DOUBLE, NumberFieldMapper.NumberType.LONG, NumberFieldMapper.NumberType.LONG),
190+
List.of(
191+
NumberFieldMapper.NumberType.DOUBLE,
192+
NumberFieldMapper.NumberType.LONG,
193+
NumberFieldMapper.NumberType.DOUBLE,
194+
NumberFieldMapper.NumberType.DOUBLE,
195+
NumberFieldMapper.NumberType.DOUBLE,
196+
NumberFieldMapper.NumberType.LONG,
197+
NumberFieldMapper.NumberType.DOUBLE,
198+
NumberFieldMapper.NumberType.DOUBLE,
199+
NumberFieldMapper.NumberType.LONG
200+
),
163201
reader.maxDoc()
164202
);
165203
assertStarTreeDocuments(starTreeDocuments, expectedStarTreeDocuments);
@@ -190,6 +228,19 @@ private XContentBuilder getExpandedMapping() throws IOException {
190228
b.startArray("stats");
191229
b.value("sum");
192230
b.value("value_count");
231+
b.value("avg");
232+
b.value("min");
233+
b.value("max");
234+
b.endArray();
235+
b.endObject();
236+
b.startObject();
237+
b.field("name", "sndv");
238+
b.startArray("stats");
239+
b.value("sum");
240+
b.value("value_count");
241+
b.value("avg");
242+
b.value("min");
243+
b.value("max");
193244
b.endArray();
194245
b.endObject();
195246
b.endArray();

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

+34-6
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,9 @@ public static StarTreeDocument[] getSegmentsStarTreeDocuments(
6161
// get doc id set iterators for metrics
6262
for (Metric metric : starTreeValues.getStarTreeField().getMetrics()) {
6363
for (MetricStat metricStat : metric.getMetrics()) {
64+
if (metricStat.isDerivedMetric()) {
65+
continue;
66+
}
6467
String metricFullName = fullyQualifiedFieldNameForStarTreeMetricsDocValues(
6568
starTreeValues.getStarTreeField().getName(),
6669
metric.getField(),
@@ -125,18 +128,18 @@ public static void assertStarTreeDocuments(StarTreeDocument[] starTreeDocuments,
125128
assertNotNull(resultStarTreeDocument.dimensions);
126129
assertNotNull(resultStarTreeDocument.metrics);
127130

128-
assertEquals(resultStarTreeDocument.dimensions.length, expectedStarTreeDocument.dimensions.length);
129-
assertEquals(resultStarTreeDocument.metrics.length, expectedStarTreeDocument.metrics.length);
131+
assertEquals(expectedStarTreeDocument.dimensions.length, resultStarTreeDocument.dimensions.length);
132+
assertEquals(expectedStarTreeDocument.metrics.length, resultStarTreeDocument.metrics.length);
130133

131134
for (int di = 0; di < resultStarTreeDocument.dimensions.length; di++) {
132-
assertEquals(resultStarTreeDocument.dimensions[di], expectedStarTreeDocument.dimensions[di]);
135+
assertEquals(expectedStarTreeDocument.dimensions[di], resultStarTreeDocument.dimensions[di]);
133136
}
134137

135138
for (int mi = 0; mi < resultStarTreeDocument.metrics.length; mi++) {
136139
if (expectedStarTreeDocument.metrics[mi] instanceof Long) {
137-
assertEquals(resultStarTreeDocument.metrics[mi], ((Long) expectedStarTreeDocument.metrics[mi]).doubleValue());
140+
assertEquals(((Long) expectedStarTreeDocument.metrics[mi]).doubleValue(), resultStarTreeDocument.metrics[mi]);
138141
} else {
139-
assertEquals(resultStarTreeDocument.metrics[mi], expectedStarTreeDocument.metrics[mi]);
142+
assertEquals(expectedStarTreeDocument.metrics[mi], resultStarTreeDocument.metrics[mi]);
140143
}
141144
}
142145
}
@@ -267,9 +270,34 @@ public static void assertStarTreeMetadata(StarTreeMetadata expectedStarTreeMetad
267270
Metric expectedMetric = expectedStarTreeMetadata.getMetrics().get(i);
268271
Metric resultMetric = resultStarTreeMetadata.getMetrics().get(i);
269272
assertEquals(expectedMetric.getField(), resultMetric.getField());
273+
List<MetricStat> metricStats = new ArrayList<>();
274+
for (MetricStat metricStat : expectedMetric.getMetrics()) {
275+
if (metricStat.isDerivedMetric()) {
276+
continue;
277+
}
278+
metricStats.add(metricStat);
279+
}
280+
Metric expectedMetricWithoutDerivedMetrics = new Metric(expectedMetric.getField(), metricStats);
281+
metricStats = new ArrayList<>();
282+
for (MetricStat metricStat : resultMetric.getMetrics()) {
283+
if (metricStat.isDerivedMetric()) {
284+
continue;
285+
}
286+
metricStats.add(metricStat);
287+
}
288+
Metric resultantMetricWithoutDerivedMetrics = new Metric(resultMetric.getField(), metricStats);
289+
290+
// assert base metrics are in order in metadata
291+
for (int j = 0; j < expectedMetricWithoutDerivedMetrics.getMetrics().size(); j++) {
292+
assertEquals(
293+
expectedMetricWithoutDerivedMetrics.getMetrics().get(j),
294+
resultantMetricWithoutDerivedMetrics.getMetrics().get(j)
295+
);
296+
}
270297

298+
// assert all metrics ( including derived metrics are present )
271299
for (int j = 0; j < expectedMetric.getMetrics().size(); j++) {
272-
assertEquals(expectedMetric.getMetrics().get(j), resultMetric.getMetrics().get(j));
300+
assertTrue(resultMetric.getMetrics().contains(expectedMetric.getMetrics().get(j)));
273301
}
274302

275303
}

0 commit comments

Comments
 (0)