Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Star tree] Changes to handle derived metrics such as avg as part of star tree mapping #15152

Merged
merged 9 commits into from
Aug 23, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -265,13 +265,9 @@ public void testValidCompositeIndex() {
assertEquals(expectedTimeUnits, dateDim.getIntervals());
assertEquals("numeric_dv", starTreeFieldType.getDimensions().get(1).getField());
assertEquals("numeric_dv", starTreeFieldType.getMetrics().get(0).getField());
List<MetricStat> expectedMetrics = Arrays.asList(
MetricStat.AVG,
MetricStat.VALUE_COUNT,
MetricStat.SUM,
MetricStat.MAX,
MetricStat.MIN
);

// Assert default metrics
List<MetricStat> expectedMetrics = Arrays.asList(MetricStat.VALUE_COUNT, MetricStat.SUM, MetricStat.AVG);
assertEquals(expectedMetrics, starTreeFieldType.getMetrics().get(0).getMetrics());
assertEquals(10000, starTreeFieldType.getStarTreeConfig().maxLeafDocs());
assertEquals(
Expand Down Expand Up @@ -349,13 +345,9 @@ public void testUpdateIndexWhenMappingIsSame() {
assertEquals(expectedTimeUnits, dateDim.getIntervals());
assertEquals("numeric_dv", starTreeFieldType.getDimensions().get(1).getField());
assertEquals("numeric_dv", starTreeFieldType.getMetrics().get(0).getField());
List<MetricStat> expectedMetrics = Arrays.asList(
MetricStat.AVG,
MetricStat.VALUE_COUNT,
MetricStat.SUM,
MetricStat.MAX,
MetricStat.MIN
);

// Assert default metrics
List<MetricStat> expectedMetrics = Arrays.asList(MetricStat.VALUE_COUNT, MetricStat.SUM, MetricStat.AVG);
assertEquals(expectedMetrics, starTreeFieldType.getMetrics().get(0).getMetrics());
assertEquals(10000, starTreeFieldType.getStarTreeConfig().maxLeafDocs());
assertEquals(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@

import org.opensearch.common.annotation.ExperimentalApi;

import java.util.Arrays;
import java.util.List;

/**
* Supported metric types for composite index
*
Expand All @@ -18,21 +21,39 @@
@ExperimentalApi
public enum MetricStat {
VALUE_COUNT("value_count"),
AVG("avg"),
SUM("sum"),
MIN("min"),
MAX("max");
MAX("max"),
AVG("avg", VALUE_COUNT, SUM);

private final String typeName;
private final MetricStat[] baseMetrics;

MetricStat(String typeName) {
MetricStat(String typeName, MetricStat... baseMetrics) {
this.typeName = typeName;
this.baseMetrics = baseMetrics;
bharath-techie marked this conversation as resolved.
Show resolved Hide resolved
}

public String getTypeName() {
return typeName;
}

/**
* Return the list of metrics that this metric is derived from
* For example, AVG is derived from COUNT and SUM
*/
public List<MetricStat> getBaseMetrics() {
return Arrays.asList(baseMetrics);
}

/**
* Return true if this metric is derived from other metrics
* For example, AVG is derived from COUNT and SUM
*/
public boolean isDerivedMetric() {
return baseMetrics != null && baseMetrics.length > 0;
}

public static MetricStat fromTypeName(String typeName) {
for (MetricStat metric : MetricStat.values()) {
if (metric.getTypeName().equalsIgnoreCase(typeName)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import java.util.Arrays;
import java.util.List;
import java.util.function.Function;

/**
* Index settings for star tree fields. The settings are final as right now
Expand Down Expand Up @@ -93,16 +94,10 @@ public class StarTreeIndexSettings {
/**
* Default metrics for metrics as part of star tree fields
*/
public static final Setting<List<MetricStat>> DEFAULT_METRICS_LIST = Setting.listSetting(
public static final Setting<List<String>> DEFAULT_METRICS_LIST = Setting.listSetting(
"index.composite_index.star_tree.field.default.metrics",
Arrays.asList(
MetricStat.AVG.toString(),
MetricStat.VALUE_COUNT.toString(),
MetricStat.SUM.toString(),
MetricStat.MAX.toString(),
MetricStat.MIN.toString()
),
MetricStat::fromTypeName,
Arrays.asList(MetricStat.VALUE_COUNT.toString(), MetricStat.SUM.toString()),
Function.identity(),
Setting.Property.IndexScope,
Setting.Property.Final
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ public List<MetricAggregatorInfo> generateMetricAggregatorInfos(MapperService ma
List<MetricAggregatorInfo> metricAggregatorInfos = new ArrayList<>();
for (Metric metric : this.starTreeField.getMetrics()) {
for (MetricStat metricStat : metric.getMetrics()) {
if (metricStat.isDerivedMetric()) {
continue;
}
IndexNumericFieldData.NumericType numericType;
Mapper fieldMapper = mapperService.documentMapper().mappers().getMapper(metric.getField());
if (fieldMapper instanceof NumberFieldMapper) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
import java.util.Queue;
import java.util.Set;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -262,17 +263,50 @@
.collect(Collectors.toList());
metric.remove(STATS);
if (metricStrings.isEmpty()) {
metricTypes = new ArrayList<>(StarTreeIndexSettings.DEFAULT_METRICS_LIST.get(context.getSettings()));
} else {
Set<MetricStat> metricSet = new LinkedHashSet<>();
for (String metricString : metricStrings) {
metricSet.add(MetricStat.fromTypeName(metricString));
}
metricTypes = new ArrayList<>(metricSet);
metricStrings = new ArrayList<>(StarTreeIndexSettings.DEFAULT_METRICS_LIST.get(context.getSettings()));
}
// Add all required metrics initially
Set<MetricStat> metricSet = new LinkedHashSet<>();
for (String metricString : metricStrings) {
bharath-techie marked this conversation as resolved.
Show resolved Hide resolved
MetricStat metricStat = MetricStat.fromTypeName(metricString);
metricSet.add(metricStat);
addBaseMetrics(metricStat, metricSet);
}
addEligibleDerivedMetrics(metricSet);
metricTypes = new ArrayList<>(metricSet);
return new Metric(name, metricTypes);
}

/**
* Add base metrics of derived metric to metric set
*/
private void addBaseMetrics(MetricStat metricStat, Set<MetricStat> metricSet) {
if (metricStat.isDerivedMetric()) {
Queue<MetricStat> metricQueue = new LinkedList<>(metricStat.getBaseMetrics());
while (metricQueue.isEmpty() == false) {
MetricStat metric = metricQueue.poll();
if (metric.isDerivedMetric() && !metricSet.contains(metric)) {
metricQueue.addAll(metric.getBaseMetrics());

Check warning on line 289 in server/src/main/java/org/opensearch/index/mapper/StarTreeMapper.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/org/opensearch/index/mapper/StarTreeMapper.java#L289

Added line #L289 was not covered by tests
bharath-techie marked this conversation as resolved.
Show resolved Hide resolved
}
metricSet.add(metric);
}
}
}

/**
* Add derived metrics if all associated base metrics are present
*/
private void addEligibleDerivedMetrics(Set<MetricStat> metricStats) {
bharath-techie marked this conversation as resolved.
Show resolved Hide resolved
for (MetricStat metric : MetricStat.values()) {
if (metric.isDerivedMetric() && !metricStats.contains(metric)) {
List<MetricStat> sourceMetrics = metric.getBaseMetrics();
if (metricStats.containsAll(sourceMetrics)) {
metricStats.add(metric);
}
}
}
}

@Override
protected List<Parameter<?>> getParameters() {
return List.of(config);
Expand Down
Loading
Loading