From fb161623703537d5da87faf6d3108f94b3270494 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 20 Feb 2020 18:32:27 +0200 Subject: [PATCH 1/3] ValuesSource refactoring: Wire up Avg aggregation --- .../elasticsearch/search/SearchModule.java | 3 +- .../metrics/AvgAggregationBuilder.java | 5 +++ .../metrics/AvgAggregatorFactory.java | 31 ++++++++++++++++--- .../metrics/MetricAggregatorSupplier.java | 24 ++++++++++++++ 4 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index 36421b5cb9aac..8838b4a2c7886 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -339,7 +339,8 @@ public Map getHighlighters() { private void registerAggregations(List plugins) { registerAggregation(new AggregationSpec(AvgAggregationBuilder.NAME, AvgAggregationBuilder::new, AvgAggregationBuilder::parse) - .addResultReader(InternalAvg::new)); + .addResultReader(InternalAvg::new) + .setAggregatorRegistrar(AvgAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(WeightedAvgAggregationBuilder.NAME, WeightedAvgAggregationBuilder::new, WeightedAvgAggregationBuilder::parse).addResultReader(InternalWeightedAvg::new)); registerAggregation(new AggregationSpec(SumAggregationBuilder.NAME, SumAggregationBuilder::new, SumAggregationBuilder::parse) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java index ce7312385e8cf..ddb1eab0bab0c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregationBuilder.java @@ -33,6 +33,7 @@ import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import org.elasticsearch.search.aggregations.support.ValuesSourceType; import java.io.IOException; @@ -50,6 +51,10 @@ public static AggregationBuilder parse(String aggregationName, XContentParser pa return PARSER.parse(parser, aggregationName); } + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + AvgAggregatorFactory.registerAggregators(valuesSourceRegistry); + } + public AvgAggregationBuilder(String name) { super(name); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java index d13b200bf55a7..8d2014a18aab8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java @@ -20,15 +20,19 @@ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.index.query.QueryShardContext; +import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationExecutionException; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorFactories; import org.elasticsearch.search.aggregations.AggregatorFactory; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.aggregations.support.AggregatorSupplier; +import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSource.Numeric; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; @@ -43,6 +47,22 @@ class AvgAggregatorFactory extends ValuesSourceAggregatorFactory { super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData); } + static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + valuesSourceRegistry.register(SumAggregationBuilder.NAME, + List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN), + new MetricAggregatorSupplier() { + @Override + public Aggregator build(String name, + ValuesSource valuesSource, + DocValueFormat formatter, + SearchContext context, + Aggregator parent, + List pipelineAggregators, Map metaData) throws IOException { + return new AvgAggregator(name, (Numeric) valuesSource, formatter, context, parent, pipelineAggregators, metaData); + } + }); + } + @Override protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, @@ -58,11 +78,14 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, boolean collectsFromSingleBucket, List pipelineAggregators, Map metaData) throws IOException { + AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config.valueSourceType(), + AvgAggregationBuilder.NAME); - if (valuesSource instanceof Numeric == false) { - throw new AggregationExecutionException("ValuesSource type " + valuesSource.toString() + "is not supported for aggregation " + - this.name()); + if (aggregatorSupplier instanceof MetricAggregatorSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected MetricAggregatorSupplier, found [" + + aggregatorSupplier.getClass().toString() + "]"); } - return new AvgAggregator(name, (Numeric) valuesSource, config.format(), searchContext, parent, pipelineAggregators, metaData); + return ((MetricAggregatorSupplier) aggregatorSupplier).build(name, valuesSource, config.format(), searchContext, parent, + pipelineAggregators, metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java new file mode 100644 index 0000000000000..2463a04ac5e1a --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java @@ -0,0 +1,24 @@ +package org.elasticsearch.search.aggregations.metrics; + +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.Aggregator; +import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.aggregations.support.AggregatorSupplier; +import org.elasticsearch.search.aggregations.support.ValuesSource; +import org.elasticsearch.search.internal.SearchContext; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +public interface MetricAggregatorSupplier extends AggregatorSupplier { + + Aggregator build(String name, + ValuesSource valuesSource, + DocValueFormat formatter, + SearchContext context, + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException; + +} From f413b1470fa7076066485a68506925116353cd48 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Thu, 20 Feb 2020 18:47:14 +0200 Subject: [PATCH 2/3] ValuesSource refactoring: Wire up Avg aggregation --- .../metrics/MetricAggregatorSupplier.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java index 2463a04ac5e1a..fc7f2d54a8d93 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MetricAggregatorSupplier.java @@ -1,3 +1,21 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.search.DocValueFormat; From 234cd3c23312882d7d71f97fa589bb7b85ab6e12 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Fri, 21 Feb 2020 15:51:03 +0200 Subject: [PATCH 3/3] Fixed wrong registration --- .../search/aggregations/metrics/AvgAggregatorFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java index 8d2014a18aab8..a510665bf7ab5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/AvgAggregatorFactory.java @@ -48,7 +48,7 @@ class AvgAggregatorFactory extends ValuesSourceAggregatorFactory { } static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { - valuesSourceRegistry.register(SumAggregationBuilder.NAME, + valuesSourceRegistry.register(AvgAggregationBuilder.NAME, List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN), new MetricAggregatorSupplier() { @Override