From 001cb820fcdbb0dc29d7705f723c00a113f6136c Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 11 Feb 2020 13:15:50 -0500 Subject: [PATCH 1/3] Wire up Value Count --- .../elasticsearch/search/SearchModule.java | 4 ++- .../metrics/CardinalityAggregatorFactory.java | 2 +- .../metrics/ValueCountAggregationBuilder.java | 5 ++++ .../ValueCountAggregationSupplier.java | 20 ++++++++++++++ .../metrics/ValueCountAggregatorFactory.java | 26 ++++++++++++++++++- .../support/ValuesSourceRegistry.java | 19 +++++++++++--- 6 files changed, 70 insertions(+), 6 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index cf49a8266afc3..b218bddd4244d 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -351,7 +351,9 @@ private void registerAggregations(List plugins) { registerAggregation(new AggregationSpec(ExtendedStatsAggregationBuilder.NAME, ExtendedStatsAggregationBuilder::new, ExtendedStatsAggregationBuilder::parse).addResultReader(InternalExtendedStats::new)); registerAggregation(new AggregationSpec(ValueCountAggregationBuilder.NAME, ValueCountAggregationBuilder::new, - ValueCountAggregationBuilder::parse).addResultReader(InternalValueCount::new)); + ValueCountAggregationBuilder::parse) + .addResultReader(InternalValueCount::new) + .setAggregatorRegistrar(ValueCountAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(PercentilesAggregationBuilder.NAME, PercentilesAggregationBuilder::new, PercentilesAggregationBuilder::parse) .addResultReader(InternalTDigestPercentiles.NAME, InternalTDigestPercentiles::new) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorFactory.java index f9e9a3fd8374b..63da48e944d86 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/CardinalityAggregatorFactory.java @@ -51,7 +51,7 @@ class CardinalityAggregatorFactory extends ValuesSourceAggregatorFactory { } static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { - valuesSourceRegistry.register(CardinalityAggregationBuilder.NAME, (ignored) -> true, cardinalityAggregatorSupplier()); + valuesSourceRegistry.registerAny(CardinalityAggregationBuilder.NAME, cardinalityAggregatorSupplier()); } private static CardinalityAggregatorSupplier cardinalityAggregatorSupplier(){ diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java index 25892b179829e..5998de1e7bf17 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationBuilder.java @@ -34,6 +34,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; @@ -52,6 +53,10 @@ public static AggregationBuilder parse(String aggregationName, XContentParser pa return PARSER.parse(parser, new ValueCountAggregationBuilder(aggregationName), null); } + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + ValueCountAggregatorFactory.registerAggregators(valuesSourceRegistry); + } + public ValueCountAggregationBuilder(String name) { super(name); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java new file mode 100644 index 0000000000000..e3ed1bb8d0d04 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java @@ -0,0 +1,20 @@ +package org.elasticsearch.search.aggregations.metrics; + +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 ValueCountAggregationSupplier extends AggregatorSupplier { + Aggregator build(String name, + ValuesSource valuesSource, + SearchContext aggregationContext, + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException; +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java index 0c8787856af8b..54881b2b2a036 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java @@ -20,13 +20,16 @@ package org.elasticsearch.search.aggregations.metrics; import org.elasticsearch.index.query.QueryShardContext; +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.ValuesSource; 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; @@ -35,6 +38,21 @@ class ValueCountAggregatorFactory extends ValuesSourceAggregatorFactory { + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + valuesSourceRegistry.registerAny(ValueCountAggregationBuilder.NAME, + new ValueCountAggregationSupplier() { + @Override + public Aggregator build(String name, + ValuesSource valuesSource, + SearchContext aggregationContext, + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException { + return new ValueCountAggregator(name, valuesSource, aggregationContext, parent, pipelineAggregators, metaData); + } + }); + } + ValueCountAggregatorFactory(String name, ValuesSourceConfig config, QueryShardContext queryShardContext, AggregatorFactory parent, AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { @@ -56,6 +74,12 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, boolean collectsFromSingleBucket, List pipelineAggregators, Map metaData) throws IOException { - return new ValueCountAggregator(name, valuesSource, searchContext, parent, pipelineAggregators, metaData); + AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config.valueSourceType(), + ValueCountAggregationBuilder.NAME); + if (aggregatorSupplier instanceof ValueCountAggregationSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected ValueCountAggregationSupplier, found [" + + aggregatorSupplier.getClass().toString() + "]"); + } + return ((ValueCountAggregationSupplier) aggregatorSupplier).build(name, valuesSource, searchContext, parent, pipelineAggregators, metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java index 77d838b7a86b4..e60ed6789c1e6 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java @@ -54,7 +54,7 @@ public class ValuesSourceRegistry { * different worker threads. Thus we want to optimize the read case to be thread safe and fast, which the immutable * collections do well. Using immutable collections requires a copy on write mechanic, thus the somewhat non-intuitive * implementation of this method. - * @param aggregationName The name of the family of aggregations, typically found via ValuesSourceAggregationBuilder.getType() + * @param aggregationName The name of the family of aggregations, typically found via {@link ValuesSourceAggregationBuilder#getType()} * @param appliesTo A predicate which accepts the resolved {@link ValuesSourceType} and decides if the given aggregator can be applied * to that type. * @param aggregatorSupplier An Aggregation-specific specialization of AggregatorSupplier which will construct the mapped aggregator @@ -75,7 +75,7 @@ public synchronized void register(String aggregationName, Predicate valuesSource }, aggregatorSupplier); } + /** + * Register an aggregator that applies to any values source type. This is a convenience method for aggregations that do not care at all + * about the types of their inputs. Aggregations using this version of registration should not make any other registrations, as the + * aggregator registered using this function will be applied in all cases. + * + * @param aggregationName The name of the family of aggregations, typically found via {@link ValuesSourceAggregationBuilder#getType()} + * @param aggregatorSupplier An Aggregation-specific specialization of AggregatorSupplier which will construct the mapped aggregator + * from the aggregation standard set of parameters. + */ + public void registerAny(String aggregationName, AggregatorSupplier aggregatorSupplier) { + register(aggregationName, (ignored) -> true, aggregatorSupplier); + } + private AggregatorSupplier findMatchingSuppier(ValuesSourceType valuesSourceType, List, AggregatorSupplier>> supportedTypes) { for (Map.Entry, AggregatorSupplier> candidate : supportedTypes) { From 272c1896a54eb49f11684f46b90734dc8a7b32dc Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 11 Feb 2020 14:02:50 -0500 Subject: [PATCH 2/3] fix checkstyle --- .../metrics/ValueCountAggregationSupplier.java | 18 ++++++++++++++++++ .../metrics/ValueCountAggregatorFactory.java | 3 ++- 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java index e3ed1bb8d0d04..567ab4af4e713 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.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.aggregations.Aggregator; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java index 54881b2b2a036..8da7ee30f39ab 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java @@ -80,6 +80,7 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, throw new AggregationExecutionException("Registry miss-match - expected ValueCountAggregationSupplier, found [" + aggregatorSupplier.getClass().toString() + "]"); } - return ((ValueCountAggregationSupplier) aggregatorSupplier).build(name, valuesSource, searchContext, parent, pipelineAggregators, metaData); + return ((ValueCountAggregationSupplier) aggregatorSupplier) + .build(name, valuesSource, searchContext, parent, pipelineAggregators,metaData); } } From f2a0aa06c0e573b18dd5fcf58caf95aeef36c244 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 19 Feb 2020 13:23:56 -0500 Subject: [PATCH 3/3] Fix naming mistake --- .../aggregations/metrics/ValueCountAggregatorFactory.java | 8 ++++---- ...ionSupplier.java => ValueCountAggregatorSupplier.java} | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) rename server/src/main/java/org/elasticsearch/search/aggregations/metrics/{ValueCountAggregationSupplier.java => ValueCountAggregatorSupplier.java} (95%) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java index 8da7ee30f39ab..4f4d423944025 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorFactory.java @@ -40,7 +40,7 @@ class ValueCountAggregatorFactory extends ValuesSourceAggregatorFactory { public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { valuesSourceRegistry.registerAny(ValueCountAggregationBuilder.NAME, - new ValueCountAggregationSupplier() { + new ValueCountAggregatorSupplier() { @Override public Aggregator build(String name, ValuesSource valuesSource, @@ -76,11 +76,11 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, Map metaData) throws IOException { AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config.valueSourceType(), ValueCountAggregationBuilder.NAME); - if (aggregatorSupplier instanceof ValueCountAggregationSupplier == false) { - throw new AggregationExecutionException("Registry miss-match - expected ValueCountAggregationSupplier, found [" + + if (aggregatorSupplier instanceof ValueCountAggregatorSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected ValueCountAggregatorSupplier, found [" + aggregatorSupplier.getClass().toString() + "]"); } - return ((ValueCountAggregationSupplier) aggregatorSupplier) + return ((ValueCountAggregatorSupplier) aggregatorSupplier) .build(name, valuesSource, searchContext, parent, pipelineAggregators,metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorSupplier.java similarity index 95% rename from server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java rename to server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorSupplier.java index 567ab4af4e713..bbc47e6d56e84 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregationSupplier.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/ValueCountAggregatorSupplier.java @@ -28,7 +28,7 @@ import java.util.List; import java.util.Map; -public interface ValueCountAggregationSupplier extends AggregatorSupplier { +public interface ValueCountAggregatorSupplier extends AggregatorSupplier { Aggregator build(String name, ValuesSource valuesSource, SearchContext aggregationContext,