Add support for histogram fields to rate aggregation #63289
Conversation
The rate aggregation now supports histogram fields. At the moment only sum is supported. The value count mode will be added as a follow up. Closes elastic#60674
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
elasticmachine
added
the
Team:Analytics
|
Not sure if it will make it before the feature freeze for 7.10.0, but we can try. |
| for (int i = 0; i < valuesCount; i++) { | ||
| double value = values.nextValue(); | ||
| kahanSummation.add(value); | ||
| if (valuesSource instanceof HistogramValuesSource.Histogram) { |
There was a problem hiding this comment.
Rather than using an instanceof check and having one aggregator that supports both ValuesSourceTypes, we should create a new aggregator for histogram types, and add a second mapping in the register aggregators method on the factory.
There was a problem hiding this comment.
Oh, didn't think about that! Great idea.
…te-support-for-histogram-fields
| @@ -46,7 +40,7 @@ public RateAggregator( | |||
| Map<String, Object> metadata | |||
| ) throws IOException { | |||
| super(name, context, parent, metadata); | |||
| this.valuesSource = (ValuesSource.Numeric) valuesSourceConfig.getValuesSource(); | |||
| this.valuesSource = valuesSourceConfig.getValuesSource(); | |||
There was a problem hiding this comment.
I think this is fine, but just for the sake of mentioning options, we could push getting the values source into the concrete subclasses and do the cast in the constructor, rather than at access time in the leaf reader.
| @@ -44,15 +45,21 @@ | |||
| static void registerAggregators(ValuesSourceRegistry.Builder builder) { | |||
| builder.register( | |||
| RateAggregationBuilder.REGISTRY_KEY, | |||
| List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.BOOLEAN), | |||
| RateAggregator::new, | |||
| Collections.singletonList(CoreValuesSourceType.NUMERIC), | |||
There was a problem hiding this comment.
Is it a BWC break that we're dropping boolean here?
There was a problem hiding this comment.
Technically, yes. But this behavior was never documented, and it doesn't make any sense in this context. I am not even sure what would be an interpretation of this number in case of boolean field.
The rate aggregation now supports histogram fields. At the moment only sum is supported. Closes elastic#62939
The rate aggregation now supports histogram fields. At the moment only sum
is supported. The value count mode will be added as a follow up.
Closes #62939