Histogram field type support for ValueCount and Avg aggregations#55933
Histogram field type support for ValueCount and Avg aggregations#55933csoulios merged 7 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
nik9000
left a comment
There was a problem hiding this comment.
I left some small stuff but it mostly looks good to me. Its neat that this is comparatively easy to do now!
| AnalyticsValuesSourceType.HISTOGRAM, | ||
| (MetricAggregatorSupplier) HistoBackedSumAggregator::new); | ||
| (MetricAggregatorSupplier) (name, valuesSource, format, context, parent, metadata) -> | ||
| new HistoBackedSumAggregator(name, (HistogramValuesSource.Histogram) valuesSource, format, context, parent, metadata) |
There was a problem hiding this comment.
@not-napoleon and I've been favoring changing the ctor so you can just to (MetricAggregatorSupplier) HistoBackedSumAggregator::new. It is less "big".
There was a problem hiding this comment.
(MetricAggregatorSupplier) HistoBackedSumAggregator::new is definitely better looking and I would prefer it as well.
The problem is that MetricAggregatorSupplier passes a ValuesSource argument
Aggregator build(String name,
ValuesSource valuesSource,
DocValueFormat format,
SearchContext context,
Aggregator parent,
Map<String, Object> metadata) throws IOException;
while HistoBackedSumAggregator constructor expects a HistogramValuesSource.Histogram
HistoBackedSumAggregator(String name, HistogramValuesSource.Histogram valuesSource, DocValueFormat formatter, SearchContext context,
Aggregator parent, Map<String, Object> metadata) throws IOException
So I had 3 options:
- Keep the
ValuesSourceparameter in theHistoBackedSumAggregatorconstructor and do a type cast later when I would use theHistogramValuesSource.Histogramobject. - Create a separate
HistoBackedMetricAggregatorSupplierthat would match the arguments of theHistoBackedSumAggregator - Do the cast of
ValuesSourcetoHistogramValuesSource.Histogramin the registrar method changing the admittedly prettier(MetricAggregatorSupplier) HistoBackedSumAggregator::newto the longer lambda expression.
I chose the third option because it seemed cleaner to do the type cast at the registrar method where it is obvious that the VS is a histogram. On the other hand, I didn't want to create yet another AggregatorSupplier sub-class
There was a problem hiding this comment.
I've been favoring having generic ValuesSource in the supplier and doing the cast in the aggregator constructor instead. Really the suppliers shouldn't care about the ValuesSource subclass, since the whole idea is to be able to add new ones in, and having to subclass a supplier to do that is a lot of boiler plate. I know I haven't been perfectly consistent about that so far, but I'm working on cleaning it up as I can.
...va/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedAvgAggregatorTests.java
Outdated
Show resolved
Hide resolved
...va/org/elasticsearch/xpack/analytics/aggregations/metrics/HistoBackedSumAggregatorTests.java
Outdated
Show resolved
Hide resolved
#56099) Backports #55933 to 7.x Implements value_count and avg aggregations over Histogram fields as discussed in #53285 - value_count returns the sum of all counts array of the histograms - avg computes a weighted average of the values array of the histogram by multiplying each value with its associated element in the counts array
Implements
value_countandavgaggregations over Histogram fields as discussed in #53285value_countreturns the sum of allcountsarray of the histogramsavgcomputes a weighted average of thevaluesarray of the histogram by multiplying each value with its associated element in thecountsarray