From 7537ff384ec3fc74924732ddd41f8255c254b754 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Fri, 6 Mar 2020 17:15:56 +0200 Subject: [PATCH 1/3] VS refactoring: Wire up median Extended Stats agg --- .../elasticsearch/search/SearchModule.java | 12 +++--- .../ExtendedStatsAggregationBuilder.java | 4 ++ .../ExtendedStatsAggregatorFactory.java | 30 +++++++++---- .../ExtendedStatsAggregatorProvider.java | 42 +++++++++++++++++++ 4 files changed, 74 insertions(+), 14 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorProvider.java diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index 37464dfac6c28..551f2e63dbe17 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -347,16 +347,18 @@ private void registerAggregations(List plugins) { .addResultReader(InternalSum::new) .setAggregatorRegistrar(SumAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(MinAggregationBuilder.NAME, MinAggregationBuilder::new, MinAggregationBuilder::parse) - .addResultReader(InternalMin::new) - .setAggregatorRegistrar(MinAggregationBuilder::registerAggregators)); + .addResultReader(InternalMin::new) + .setAggregatorRegistrar(MinAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(MaxAggregationBuilder.NAME, MaxAggregationBuilder::new, MaxAggregationBuilder::parse) - .addResultReader(InternalMax::new) - .setAggregatorRegistrar(MaxAggregationBuilder::registerAggregators)); + .addResultReader(InternalMax::new) + .setAggregatorRegistrar(MaxAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(StatsAggregationBuilder.NAME, StatsAggregationBuilder::new, StatsAggregationBuilder::parse) .addResultReader(InternalStats::new) .setAggregatorRegistrar(StatsAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(ExtendedStatsAggregationBuilder.NAME, ExtendedStatsAggregationBuilder::new, - ExtendedStatsAggregationBuilder::parse).addResultReader(InternalExtendedStats::new)); + ExtendedStatsAggregationBuilder::parse) + .addResultReader(InternalExtendedStats::new) + .setAggregatorRegistrar(ExtendedStatsAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(ValueCountAggregationBuilder.NAME, ValueCountAggregationBuilder::new, ValueCountAggregationBuilder::parse) .addResultReader(InternalValueCount::new) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java index 960c6f940e83d..7f3ad2b7cfbe9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java @@ -33,6 +33,7 @@ import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import org.elasticsearch.search.aggregations.support.ValuesSourceType; import java.io.IOException; @@ -54,6 +55,9 @@ public static AggregationBuilder parse(String aggregationName, XContentParser pa return PARSER.parse(parser, new ExtendedStatsAggregationBuilder(aggregationName), null); } + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + ExtendedStatsAggregatorFactory.registerAggregators(valuesSourceRegistry); + } private double sigma = 2.0; public ExtendedStatsAggregationBuilder(String name) { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorFactory.java index cba2e1cf72eb2..89fe71de2b62d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorFactory.java @@ -25,10 +25,13 @@ 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; @@ -50,6 +53,12 @@ class ExtendedStatsAggregatorFactory extends ValuesSourceAggregatorFactory { this.sigma = sigma; } + static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + valuesSourceRegistry.register(ExtendedStatsAggregationBuilder.NAME, + List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN), + (ExtendedStatsAggregatorProvider) ExtendedStatsAggregator::new); + } + @Override protected Aggregator createUnmapped(SearchContext searchContext, Aggregator parent, @@ -61,16 +70,19 @@ protected Aggregator createUnmapped(SearchContext searchContext, @Override protected Aggregator doCreateInternal(ValuesSource valuesSource, - SearchContext searchContext, - Aggregator parent, - boolean collectsFromSingleBucket, - List pipelineAggregators, - Map metaData) throws IOException { - if (valuesSource instanceof Numeric == false) { - throw new AggregationExecutionException("ValuesSource type " + valuesSource.toString() + "is not supported for aggregation " + - this.name()); + SearchContext searchContext, + Aggregator parent, + boolean collectsFromSingleBucket, + List pipelineAggregators, + Map metaData) throws IOException { + AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config.valueSourceType(), + ExtendedStatsAggregationBuilder.NAME); + + if (aggregatorSupplier instanceof ExtendedStatsAggregatorProvider == false) { + throw new AggregationExecutionException("Registry miss-match - expected ExtendedStatsAggregatorProvider, found [" + + aggregatorSupplier.getClass().toString() + "]"); } - return new ExtendedStatsAggregator(name, (Numeric) valuesSource, config.format(), searchContext, + return ((ExtendedStatsAggregatorProvider) aggregatorSupplier).build(name, (Numeric) valuesSource, config.format(), searchContext, parent, sigma, pipelineAggregators, metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorProvider.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorProvider.java new file mode 100644 index 0000000000000..0826f9a29e4a4 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregatorProvider.java @@ -0,0 +1,42 @@ +/* + * 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; +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 ExtendedStatsAggregatorProvider extends AggregatorSupplier { + + Aggregator build(String name, + ValuesSource.Numeric valuesSource, + DocValueFormat formatter, + SearchContext context, + Aggregator parent, + double sigma, + List pipelineAggregators, + Map metaData) throws IOException; +} From 54062137a5c5edec786d4e1ab344b07d57716026 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Mon, 9 Mar 2020 14:59:56 +0200 Subject: [PATCH 2/3] Removed ExtendedStatsAggregationBuilder::parse Replaced it with ExtendedStatsAggregationBuilder.PARSE --- .../src/main/java/org/elasticsearch/search/SearchModule.java | 2 +- .../aggregations/metrics/ExtendedStatsAggregationBuilder.java | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index 843ddc3115355..e74e8dedeaa21 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -356,7 +356,7 @@ private void registerAggregations(List plugins) { .addResultReader(InternalStats::new) .setAggregatorRegistrar(StatsAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(ExtendedStatsAggregationBuilder.NAME, ExtendedStatsAggregationBuilder::new, - ExtendedStatsAggregationBuilder::parse) + ExtendedStatsAggregationBuilder.PARSER) .addResultReader(InternalExtendedStats::new) .setAggregatorRegistrar(ExtendedStatsAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(ValueCountAggregationBuilder.NAME, ValueCountAggregationBuilder::new, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java index ecd88cdebb051..631c6845072d3 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java @@ -51,10 +51,6 @@ public class ExtendedStatsAggregationBuilder PARSER.declareDouble(ExtendedStatsAggregationBuilder::sigma, ExtendedStatsAggregator.SIGMA_FIELD); } - public static AggregationBuilder parse(String aggregationName, XContentParser parser) throws IOException { - return PARSER.parse(parser, new ExtendedStatsAggregationBuilder(aggregationName), null); - } - public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { ExtendedStatsAggregatorFactory.registerAggregators(valuesSourceRegistry); } From f14e2177dd2f3d88d42942f184f49c9e1eea3234 Mon Sep 17 00:00:00 2001 From: Christos Soulios Date: Mon, 9 Mar 2020 15:06:53 +0200 Subject: [PATCH 3/3] Checkstyle: removed unused import --- .../aggregations/metrics/ExtendedStatsAggregationBuilder.java | 1 - 1 file changed, 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java index 631c6845072d3..0d5991d1b2e92 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ExtendedStatsAggregationBuilder.java @@ -23,7 +23,6 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.ObjectParser; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder;