Skip to content

Speed up top_metrics on hot shards #70579

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Mar 22, 2021
Merged

Speed up top_metrics on hot shards #70579

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
eab894a
Speed up top_metrics on hot shards
nik9000 Mar 18, 2021
9d8ffc1
another test
nik9000 Mar 18, 2021
65f1fe5
unused import
nik9000 Mar 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .../main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregationBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ public static void registerAggregators(ValuesSourceRegistry.Builder registry) {
registry.register(
REGISTRY_KEY,
List.of(CoreValuesSourceType.KEYWORD, CoreValuesSourceType.IP),
TopMetricsAggregator.GlobalOrdsValues::new,
TopMetricsAggregator.SegmentOrdsValues::new,
false
);
}
Expand Down
72 changes: 45 additions & 27 deletions ...tics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.elasticsearch.common.util.BitArray;
import org.elasticsearch.common.util.DoubleArray;
import org.elasticsearch.common.util.LongArray;
import org.elasticsearch.common.util.ObjectArray;
import org.elasticsearch.index.fielddata.NumericDoubleValues;
import org.elasticsearch.search.DocValueFormat;
import org.elasticsearch.search.MultiValueMode;
Expand All @@ -33,8 +34,8 @@
import org.elasticsearch.search.sort.BucketedSort;
import org.elasticsearch.search.sort.SortBuilder;
import org.elasticsearch.search.sort.SortValue;
import org.elasticsearch.xpack.core.common.search.aggregations.MissingHelper;
import org.elasticsearch.xpack.analytics.topmetrics.InternalTopMetrics.MetricValue;
import org.elasticsearch.xpack.core.common.search.aggregations.MissingHelper;

import java.io.IOException;
import java.util.ArrayList;
Expand Down Expand Up @@ -398,20 +399,32 @@ public void close() {
}

/**
* Loads metrics for whole numbers.
* Loads metrics fields with segment ordinals.
*/
static class GlobalOrdsValues extends CollectingMetricValues {
static class SegmentOrdsValues extends CollectingMetricValues {
private final ValuesSource.Bytes.WithOrdinals valuesSource;
private SortedSetDocValues globalOrds;
private LongArray values;
/**
* Reference to the segment's doc values so we can convert the
* {@link #segmentOrds} into strings on the way out.
*/
private ObjectArray<SortedSetDocValues> segmentResolve;
/**
* Terms ordinal in the segment.
*/
private LongArray segmentOrds;

GlobalOrdsValues(int size, BigArrays bigArrays, String name, ValuesSourceConfig config) {
SegmentOrdsValues(int size, BigArrays bigArrays, String name, ValuesSourceConfig config) {
super(bigArrays, name, config);
if (false == config.hasGlobalOrdinals()) {
throw new IllegalArgumentException("top_metrics can only collect bytes that have global ordinals");
try {
valuesSource = (ValuesSource.Bytes.WithOrdinals) config.getValuesSource();
} catch (ClassCastException e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although this isn't literally an instanceof check, it's still relying on the class type to convey capabilities. I wonder if we should add a ValuesSource#hasSegmentOrdinals() method to parallel the hasGlobalOrdinals() method we were using before this change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably is, yeah.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll open up a follow up.

throw new IllegalArgumentException(
"top_metrics can only collect bytes that have segment ordinals but " + config.getDescription() + " does not",
e
);
}
this.valuesSource = (ValuesSource.Bytes.WithOrdinals) config.getValuesSource();
values = bigArrays.newLongArray(size, false);
segmentResolve = bigArrays.newObjectArray(size);
segmentOrds = bigArrays.newLongArray(size, false);
}

@Override
Expand All @@ -421,43 +434,48 @@ public double doubleValue(long index) {

@Override
public MetricValue metricValue(long index) throws IOException {
if (globalOrds == null) {
// We didn't collect a single segment.
return null;
}
long ord = values.get(index);
long ord = segmentOrds.get(index);
if (ord == -1) {
return null;
}
return new MetricValue(config.format(), SortValue.from(BytesRef.deepCopyOf(globalOrds.lookupOrd(ord))));
SortedSetDocValues resolve = segmentResolve.get(index);
return new MetricValue(config.format(), SortValue.from(BytesRef.deepCopyOf(resolve.lookupOrd(ord))));
}

@Override
public void swap(long lhs, long rhs) {
long tmp = values.get(lhs);
values.set(lhs, values.get(rhs));
values.set(rhs, tmp);
SortedSetDocValues tempSegmentResolve = segmentResolve.get(lhs);
segmentResolve.set(lhs, segmentResolve.get(rhs));
segmentResolve.set(rhs, tempSegmentResolve);
long tmpSegmentOrd = segmentOrds.get(lhs);
segmentOrds.set(lhs, segmentOrds.get(rhs));
segmentOrds.set(rhs, tmpSegmentOrd);
}

@Override
public Loader loader(LeafReaderContext ctx) throws IOException {
globalOrds = valuesSource.globalOrdinalsValues(ctx);
SortedSetDocValues segmentOrdValues = valuesSource.ordinalsValues(ctx);
// For now just return the value that sorts first.
return (index, doc) -> {
if (false == globalOrds.advanceExact(doc)) {
values.set(index, -1);
return;
if (index >= segmentResolve.size()) {
segmentResolve = bigArrays.grow(segmentResolve, index + 1);
}
if (index >= values.size()) {
values = bigArrays.grow(values, index + 1);
if (index >= segmentOrds.size()) {
segmentOrds = bigArrays.grow(segmentOrds, index + 1);
}
values.set(index, globalOrds.nextOrd());
if (false == segmentOrdValues.advanceExact(doc)) {
segmentResolve.set(index, null);
segmentOrds.set(index, -1);
return;
}
segmentResolve.set(index, segmentOrdValues);
segmentOrds.set(index, segmentOrdValues.nextOrd());
};
}

@Override
public void close() {
Releasables.close(values);
Releasables.close(segmentResolve, segmentOrds);
}
}

Expand Down
4 changes: 2 additions & 2 deletions ...t/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorMetricsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ private ValuesSourceConfig toConfig(SortedNumericDoubleValues values) throws IOE

private ValuesSourceConfig toConfig(SortedSetDocValues values) throws IOException {
ValuesSource.Bytes.WithOrdinals source = mock(ValuesSource.Bytes.WithOrdinals.class);
when(source.globalOrdinalsValues(null)).thenReturn(values);
when(source.ordinalsValues(null)).thenReturn(values);
ValuesSourceConfig config = toConfig(source, CoreValuesSourceType.KEYWORD, DocValueFormat.RAW, true);
when(config.hasGlobalOrdinals()).thenReturn(true);
when(config.hasGlobalOrdinals()).thenReturn(false); // We don't need global orindals. Only segment ordinals.
return config;
}

Expand Down
66 changes: 66 additions & 0 deletions x-pack/plugin/src/test/resources/rest-api-spec/test/analytics/top_metrics.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,3 +583,69 @@
- match: { aggregations.tm.top.1.sort: [2] }
- match: { aggregations.tm.top.2.metrics.animal\.keyword: chicken}
- match: { aggregations.tm.top.2.sort: [3] }

---
"fetch runtime keyword fails with nice error message":
- do:
bulk:
index: test
refresh: true
body:
- '{"index": {}}'
- '{"s": 1, "animal": "cat"}'
- '{"index": {}}'
- '{"s": 2, "animal": "dog"}'
- '{"index": {}}'
- '{"s": 3, "animal": "chicken"}'

- do:
catch: /top_metrics can only collect bytes that have segment ordinals but Field \[species\] of type \[keyword\] does not/
search:
size: 0
body:
runtime_mappings:
species:
type: keyword
script: |
emit(doc['animal.keyword'].value)
aggs:
tm:
top_metrics:
metrics:
field: species
sort:
s: asc
size: 3

---
"fetch keyword fields with some missing":
- do:
bulk:
index: test
refresh: true
body:
- '{"index": {}}'
- '{"s": 1, "animal": "cat"}'
- '{"index": {}}'
- '{"s": 2, "animal": "dog"}'
- '{"index": {}}'
- '{"s": 3}'

- do:
search:
size: 0
body:
aggs:
tm:
top_metrics:
metrics:
field: animal.keyword
sort:
s: asc
size: 3
- match: { aggregations.tm.top.0.metrics.animal\.keyword: cat}
- match: { aggregations.tm.top.0.sort: [1] }
- match: { aggregations.tm.top.1.metrics.animal\.keyword: dog}
- match: { aggregations.tm.top.1.sort: [2] }
- match: { aggregations.tm.top.2.metrics.animal\.keyword: null}
- match: { aggregations.tm.top.2.sort: [3] }