Preserve metric types in top_metrics #53288
Conversation
This changes the `top_metrics` aggregation to return metrics in their original type. Since it only supports numerics, that means that dates, longs, and doubles will come back as stored, with their appropriate formatter applied.
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
| @@ -217,14 +218,14 @@ TopMetric topMetric() { | |||
| TopMetric(StreamInput in) throws IOException { | |||
| sortFormat = in.readNamedWriteable(DocValueFormat.class); | |||
| sortValue = in.readNamedWriteable(SortValue.class); | |||
| metricValues = in.readDoubleArray(); | |||
| metricValues = in.readList(s -> s.readOptionalWriteable(MetricValue::new)); | |||
There was a problem hiding this comment.
Again, we haven't released this so this break is ok.
|
|
||
| static class MetricValue implements Writeable, ToXContent { | ||
| private final DocValueFormat format; | ||
| private final SortValue value; |
There was a problem hiding this comment.
I'm aware it is odd to have a "SortValue" be part of a MetricValue. It is a very convenient way to send a type-aware thing across the wire though.
There was a problem hiding this comment.
I think this would be better as an inline comment than a PR comment.
|
|
||
| static class MetricValue implements Writeable, ToXContent { | ||
| private final DocValueFormat format; | ||
| private final SortValue value; |
There was a problem hiding this comment.
I think this would be better as an inline comment than a PR comment.
...analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/InternalTopMetrics.java
Show resolved
Hide resolved
| /** | ||
| * Tracks "missing" values in a {@link BitArray}. Unlike | ||
| * {@link DoubleMetricValues}, we there isn't a sentinel value | ||
| * that we can steel from the longs to represent missing that |
There was a problem hiding this comment.
| * that we can steel from the longs to represent missing that | |
| * that we can steal from the longs to represent missing that |
|
Thanks @not-napoleon! I think @polyfractal said he was looking at this so I'll probably give him some more time before merging. |
| @@ -234,12 +309,13 @@ public void swap(long lhs, long rhs) { | |||
| @Override | |||
| public Loader loader(LeafReaderContext ctx) throws IOException { | |||
| // TODO allow configuration of value mode | |||
There was a problem hiding this comment.
Should we put an info blurb somewhere in the docs that multi-valued fields will be averaged together?
There was a problem hiding this comment.
Probably! I'd planned to give the docs another pass in the next PR.
nik9000
force-pushed
the
top_metrics_preserve_type
branch
from
1448e7d
to
42edcc6
Compare
This changes the `top_metrics` aggregation to return metrics in their original type. Since it only supports numerics, that means that dates, longs, and doubles will come back as stored, with their appropriate formatter applied.
This changes the `top_metrics` aggregation to return metrics in their original type. Since it only supports numerics, that means that dates, longs, and doubles will come back as stored, with their appropriate formatter applied.
This changes the
top_metricsaggregation to return metrics in theiroriginal type. Since it only supports numerics, that means that dates,
longs, and doubles will come back as stored, with their appropriate
formatter applied.