From 151a3477865f9c4a19067be450e550b1dc349033 Mon Sep 17 00:00:00 2001 From: Andy Bristol Date: Fri, 13 Mar 2020 12:29:59 -0700 Subject: [PATCH 1/3] ValuesSource refactor wire up missing agg (#53511) Part of refactoring ValuesSource in #42949 --- .../elasticsearch/search/SearchModule.java | 4 +- .../missing/MissingAggregationBuilder.java | 5 +++ .../missing/MissingAggregatorFactory.java | 33 +++++++++++++- .../missing/MissingAggregatorSupplier.java | 44 +++++++++++++++++++ 4 files changed, 83 insertions(+), 3 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorSupplier.java diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index 6b2b0d846e132..d649165052be4 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -381,7 +381,9 @@ private void registerAggregations(List plugins) { registerAggregation(new AggregationSpec(GlobalAggregationBuilder.NAME, GlobalAggregationBuilder::new, GlobalAggregationBuilder::parse).addResultReader(InternalGlobal::new)); registerAggregation(new AggregationSpec(MissingAggregationBuilder.NAME, MissingAggregationBuilder::new, - MissingAggregationBuilder.PARSER).addResultReader(InternalMissing::new)); + MissingAggregationBuilder.PARSER) + .addResultReader(InternalMissing::new) + .setAggregatorRegistrar(MissingAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(FilterAggregationBuilder.NAME, FilterAggregationBuilder::new, FilterAggregationBuilder::parse).addResultReader(InternalFilter::new)); registerAggregation(new AggregationSpec(FiltersAggregationBuilder.NAME, FiltersAggregationBuilder::new, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java index d0a631240367c..aff30afb84db4 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregationBuilder.java @@ -32,6 +32,7 @@ import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; 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.aggregations.support.ValuesSourceType; import java.io.IOException; @@ -46,6 +47,10 @@ public class MissingAggregationBuilder extends ValuesSourceAggregationBuilder metaData) throws IOException { @@ -50,13 +70,22 @@ protected MissingAggregator createUnmapped(SearchContext searchContext, } @Override - protected MissingAggregator doCreateInternal(ValuesSource valuesSource, + protected Aggregator doCreateInternal(ValuesSource valuesSource, SearchContext searchContext, Aggregator parent, boolean collectsFromSingleBucket, List pipelineAggregators, Map metaData) throws IOException { - return new MissingAggregator(name, factories, valuesSource, searchContext, parent, pipelineAggregators, metaData); + + final AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry() + .getAggregator(config.valueSourceType(), MissingAggregationBuilder.NAME); + if (aggregatorSupplier instanceof MissingAggregatorSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected MissingAggregatorSupplier, found [" + + aggregatorSupplier.getClass().toString() + "]"); + } + + return ((MissingAggregatorSupplier) aggregatorSupplier) + .build(name, factories, valuesSource, searchContext, parent, pipelineAggregators, metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorSupplier.java new file mode 100644 index 0000000000000..2aca451accb08 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/missing/MissingAggregatorSupplier.java @@ -0,0 +1,44 @@ +/* + * 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.bucket.missing; + +import org.elasticsearch.common.Nullable; +import org.elasticsearch.search.aggregations.Aggregator; +import org.elasticsearch.search.aggregations.AggregatorFactories; +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; + +@FunctionalInterface +public interface MissingAggregatorSupplier extends AggregatorSupplier { + + Aggregator build(String name, + AggregatorFactories factories, + @Nullable ValuesSource valuesSource, + SearchContext aggregationContext, + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException; +} From 5219383ce5ba1cd0f2b7c46e49f012b86c0ee0d9 Mon Sep 17 00:00:00 2001 From: Christos Soulios <1561376+csoulios@users.noreply.github.com> Date: Mon, 16 Mar 2020 16:31:01 +0200 Subject: [PATCH 2/3] ValuesSource refactoring: Wire up ExtendedStats aggregation (#53227) --- .../elasticsearch/search/SearchModule.java | 38 +++++++++-------- .../ExtendedStatsAggregationBuilder.java | 4 ++ .../ExtendedStatsAggregatorFactory.java | 30 +++++++++---- .../ExtendedStatsAggregatorProvider.java | 42 +++++++++++++++++++ 4 files changed, 87 insertions(+), 27 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 d649165052be4..79dadfcde9d06 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -347,37 +347,39 @@ private void registerAggregations(List plugins) { .addResultReader(InternalSum::new) .setAggregatorRegistrar(SumAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(MinAggregationBuilder.NAME, MinAggregationBuilder::new, MinAggregationBuilder.PARSER) - .addResultReader(InternalMin::new) - .setAggregatorRegistrar(MinAggregationBuilder::registerAggregators)); + .addResultReader(InternalMin::new) + .setAggregatorRegistrar(MinAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(MaxAggregationBuilder.NAME, MaxAggregationBuilder::new, MaxAggregationBuilder.PARSER) - .addResultReader(InternalMax::new) - .setAggregatorRegistrar(MaxAggregationBuilder::registerAggregators)); + .addResultReader(InternalMax::new) + .setAggregatorRegistrar(MaxAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(StatsAggregationBuilder.NAME, StatsAggregationBuilder::new, StatsAggregationBuilder.PARSER) .addResultReader(InternalStats::new) .setAggregatorRegistrar(StatsAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(ExtendedStatsAggregationBuilder.NAME, ExtendedStatsAggregationBuilder::new, - ExtendedStatsAggregationBuilder.PARSER).addResultReader(InternalExtendedStats::new)); + ExtendedStatsAggregationBuilder.PARSER) + .addResultReader(InternalExtendedStats::new) + .setAggregatorRegistrar(ExtendedStatsAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(ValueCountAggregationBuilder.NAME, ValueCountAggregationBuilder::new, - ValueCountAggregationBuilder.PARSER) - .addResultReader(InternalValueCount::new) - .setAggregatorRegistrar(ValueCountAggregationBuilder::registerAggregators)); + ValueCountAggregationBuilder.PARSER) + .addResultReader(InternalValueCount::new) + .setAggregatorRegistrar(ValueCountAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(PercentilesAggregationBuilder.NAME, PercentilesAggregationBuilder::new, - PercentilesAggregationBuilder.PARSER) - .addResultReader(InternalTDigestPercentiles.NAME, InternalTDigestPercentiles::new) - .addResultReader(InternalHDRPercentiles.NAME, InternalHDRPercentiles::new) - .setAggregatorRegistrar(PercentilesAggregationBuilder::registerAggregators)); + PercentilesAggregationBuilder.PARSER) + .addResultReader(InternalTDigestPercentiles.NAME, InternalTDigestPercentiles::new) + .addResultReader(InternalHDRPercentiles.NAME, InternalHDRPercentiles::new) + .setAggregatorRegistrar(PercentilesAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(PercentileRanksAggregationBuilder.NAME, PercentileRanksAggregationBuilder::new, - PercentileRanksAggregationBuilder.PARSER) - .addResultReader(InternalTDigestPercentileRanks.NAME, InternalTDigestPercentileRanks::new) - .addResultReader(InternalHDRPercentileRanks.NAME, InternalHDRPercentileRanks::new) - .setAggregatorRegistrar(PercentileRanksAggregationBuilder::registerAggregators)); + PercentileRanksAggregationBuilder.PARSER) + .addResultReader(InternalTDigestPercentileRanks.NAME, InternalTDigestPercentileRanks::new) + .addResultReader(InternalHDRPercentileRanks.NAME, InternalHDRPercentileRanks::new) + .setAggregatorRegistrar(PercentileRanksAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(MedianAbsoluteDeviationAggregationBuilder.NAME, MedianAbsoluteDeviationAggregationBuilder::new, MedianAbsoluteDeviationAggregationBuilder.PARSER) .addResultReader(InternalMedianAbsoluteDeviation::new) .setAggregatorRegistrar(MedianAbsoluteDeviationAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(CardinalityAggregationBuilder.NAME, CardinalityAggregationBuilder::new, - CardinalityAggregationBuilder.PARSER).addResultReader(InternalCardinality::new) - .setAggregatorRegistrar(CardinalityAggregationBuilder::registerAggregators)); + CardinalityAggregationBuilder.PARSER).addResultReader(InternalCardinality::new) + .setAggregatorRegistrar(CardinalityAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(GlobalAggregationBuilder.NAME, GlobalAggregationBuilder::new, GlobalAggregationBuilder::parse).addResultReader(InternalGlobal::new)); registerAggregation(new AggregationSpec(MissingAggregationBuilder.NAME, MissingAggregationBuilder::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 94d9ec9b8931a..d19646c48d692 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 @@ -31,6 +31,7 @@ import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import org.elasticsearch.search.aggregations.support.ValuesSourceType; import java.io.IOException; @@ -48,6 +49,9 @@ public class ExtendedStatsAggregationBuilder PARSER.declareDouble(ExtendedStatsAggregationBuilder::sigma, ExtendedStatsAggregator.SIGMA_FIELD); } + 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 0af59c501ab2bc2cf0cf0be1d6b4858026e121bf Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 19 Mar 2020 10:01:04 -0400 Subject: [PATCH 3/3] Javadoc for ValuesSource related work (#53427) --- .../histogram/HistogramAggregatorFactory.java | 1 - .../HistogramAggregatorSupplier.java | 4 +- .../bucket/terms/TermsAggregatorFactory.java | 1 - .../support/AggregatorSupplier.java | 19 +++++ .../support/CoreValuesSourceType.java | 35 +++++---- .../aggregations/support/ValueType.java | 2 +- .../aggregations/support/ValuesSource.java | 8 ++ .../ValuesSourceAggregationBuilder.java | 9 +++ .../support/ValuesSourceType.java | 25 ++++-- .../aggregations/support/package-info.java | 77 +++++++++++++++++++ 10 files changed, 155 insertions(+), 26 deletions(-) rename server/src/main/java/org/elasticsearch/search/aggregations/{support => bucket/histogram}/HistogramAggregatorSupplier.java (89%) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/support/package-info.java diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java index 347834f5fb825..49e0064c6e94a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorFactory.java @@ -29,7 +29,6 @@ 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.HistogramAggregatorSupplier; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/HistogramAggregatorSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorSupplier.java similarity index 89% rename from server/src/main/java/org/elasticsearch/search/aggregations/support/HistogramAggregatorSupplier.java rename to server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorSupplier.java index 021577852c39e..1945a5bb1d4ef 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/HistogramAggregatorSupplier.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/HistogramAggregatorSupplier.java @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -package org.elasticsearch.search.aggregations.support; +package org.elasticsearch.search.aggregations.bucket.histogram; import org.elasticsearch.common.Nullable; import org.elasticsearch.search.DocValueFormat; @@ -24,6 +24,8 @@ import org.elasticsearch.search.aggregations.AggregatorFactories; import org.elasticsearch.search.aggregations.BucketOrder; 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; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java index 4cf73196f23e6..a94be4004893b 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorFactory.java @@ -62,7 +62,6 @@ public class TermsAggregatorFactory extends ValuesSourceAggregatorFactory { private final TermsAggregator.BucketCountThresholds bucketCountThresholds; private final boolean showTermDocCountError; - // TODO: Registration should happen on the actual aggregator classes static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { valuesSourceRegistry.register(TermsAggregationBuilder.NAME, List.of(CoreValuesSourceType.BYTES, CoreValuesSourceType.IP), diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregatorSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregatorSupplier.java index 97ca793faedb3..b646bc6b9e1d9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregatorSupplier.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregatorSupplier.java @@ -18,5 +18,24 @@ */ package org.elasticsearch.search.aggregations.support; +/** + * {@link AggregatorSupplier} serves as a marker for what the {@link ValuesSourceRegistry} holds to construct aggregator instances. + * The aggregators for each aggregation should all share a signature, and that signature should be used to create an AggregatorSupplier for + * that aggregation. Alternatively, if an existing supplier has a matching signature, please re-use that. + * + * In many cases, this can be a simple wrapper over the aggregator constructor. If that is sufficient, please consider the "typecast + * lambda" syntax: + * + * {@code + * (GeoCentroidAggregatorSupplier) (name, context, parent, valuesSource, pipelineAggregators, metaData) -> + * new GeoCentroidAggregator(name, context, parent, (ValuesSource.GeoPoint) valuesSource, pipelineAggregators, metaData)); + * } + * + * The suppliers are responsible for any casting of {@link ValuesSource} that needs to happen. They must accept a base {@link ValuesSource} + * instance. The suppliers may perform additional logic to configure the aggregator as needed, such as in + * {@link org.elasticsearch.search.aggregations.bucket.terms.TermsAggregatorFactory} deciding the execution mode. + * + * There is ongoing work to normalize aggregator constructor signatures, and thus reduce the number of AggregatorSupplier interfaces. + */ public interface AggregatorSupplier { } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java index e6064cbd1f35d..c41f65cb8b1e5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java @@ -57,7 +57,6 @@ public ValuesSource getScript(AggregationScript.LeafFactory script, ValueType sc public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFactory script) { if ((fieldContext.indexFieldData() instanceof IndexNumericFieldData) == false) { - // TODO: Is this the correct exception type here? throw new IllegalArgumentException("Expected numeric type on field [" + fieldContext.field() + "], but got [" + fieldContext.fieldType().typeName() + "]"); } @@ -71,8 +70,9 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa } @Override - public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier now) { - Number missing = docValueFormat.parseDouble(rawMissing.toString(), false, now); + public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, + LongSupplier nowSupplier) { + Number missing = docValueFormat.parseDouble(rawMissing.toString(), false, nowSupplier); return MissingValues.replaceMissing((ValuesSource.Numeric) valuesSource, missing); } }, @@ -104,7 +104,8 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa } @Override - public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier now) { + public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, + LongSupplier nowSupplier) { final BytesRef missing = docValueFormat.parseBytesRef(rawMissing.toString()); if (valuesSource instanceof ValuesSource.Bytes.WithOrdinals) { return MissingValues.replaceMissing((ValuesSource.Bytes.WithOrdinals) valuesSource, missing); @@ -127,7 +128,6 @@ public ValuesSource getScript(AggregationScript.LeafFactory script, ValueType sc @Override public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFactory script) { if (!(fieldContext.indexFieldData() instanceof IndexGeoPointFieldData)) { - // TODO: Is this the correct exception type here? throw new IllegalArgumentException("Expected geo_point type on field [" + fieldContext.field() + "], but got [" + fieldContext.fieldType().typeName() + "]"); } @@ -136,7 +136,8 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa } @Override - public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier now) { + public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, + LongSupplier nowSupplier) { // TODO: also support the structured formats of geo points final GeoPoint missing = new GeoPoint(rawMissing.toString()); return MissingValues.replaceMissing((ValuesSource.GeoPoint) valuesSource, missing); @@ -150,7 +151,6 @@ public DocValueFormat getFormatter(String format, ZoneId tz) { RANGE() { @Override public ValuesSource getEmpty() { - // TODO: Is this the correct exception type here? throw new IllegalArgumentException("Can't deal with unmapped ValuesSource type " + this.value()); } @@ -164,15 +164,15 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa MappedFieldType fieldType = fieldContext.fieldType(); if (fieldType instanceof RangeFieldMapper.RangeFieldType == false) { - // TODO: Is this the correct exception type here? - throw new IllegalStateException("Asked for range ValuesSource, but field is of type " + fieldType.name()); + throw new IllegalArgumentException("Asked for range ValuesSource, but field is of type " + fieldType.name()); } RangeFieldMapper.RangeFieldType rangeFieldType = (RangeFieldMapper.RangeFieldType) fieldType; return new ValuesSource.Range(fieldContext.indexFieldData(), rangeFieldType.rangeType()); } @Override - public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier now) { + public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, + LongSupplier nowSupplier) { throw new IllegalArgumentException("Can't apply missing values on a " + valuesSource.getClass()); } }, @@ -193,8 +193,9 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa } @Override - public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier now) { - return BYTES.replaceMissing(valuesSource, rawMissing, docValueFormat, now); + public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, + LongSupplier nowSupplier) { + return BYTES.replaceMissing(valuesSource, rawMissing, docValueFormat, nowSupplier); } @Override @@ -219,8 +220,9 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa } @Override - public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier now) { - return NUMERIC.replaceMissing(valuesSource, rawMissing, docValueFormat, now); + public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, + LongSupplier nowSupplier) { + return NUMERIC.replaceMissing(valuesSource, rawMissing, docValueFormat, nowSupplier); } @Override @@ -250,8 +252,9 @@ public ValuesSource getField(FieldContext fieldContext, AggregationScript.LeafFa } @Override - public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier now) { - return NUMERIC.replaceMissing(valuesSource, rawMissing, docValueFormat, now); + public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, + LongSupplier nowSupplier) { + return NUMERIC.replaceMissing(valuesSource, rawMissing, docValueFormat, nowSupplier); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java index 068790030bd49..3df1146533238 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValueType.java @@ -30,6 +30,7 @@ import java.time.ZoneOffset; import java.util.Set; +@Deprecated public enum ValueType implements Writeable { STRING((byte) 1, "string", "string", CoreValuesSourceType.BYTES, @@ -42,7 +43,6 @@ public enum ValueType implements Writeable { new DocValueFormat.DateTime(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER, ZoneOffset.UTC, DateFieldMapper.Resolution.MILLISECONDS)), IP((byte) 6, "ip", "ip", CoreValuesSourceType.IP, DocValueFormat.IP), - // TODO: what is the difference between "number" and "numeric"? NUMERIC((byte) 7, "numeric", "numeric", CoreValuesSourceType.NUMERIC, DocValueFormat.RAW), GEOPOINT((byte) 8, "geo_point", "geo_point", CoreValuesSourceType.GEOPOINT, DocValueFormat.GEOHASH), BOOLEAN((byte) 9, "boolean", "boolean", CoreValuesSourceType.BOOLEAN, DocValueFormat.BOOLEAN), diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java index 4bd3acfeeb08f..1b1552bf0336e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSource.java @@ -52,6 +52,14 @@ import java.io.IOException; import java.util.function.LongUnaryOperator; +/** + * Note on subclassing ValuesSources: Generally, direct subclasses of ValuesSource should also be abstract, representing types. These + * subclasses are free to add new methods specific to that type (e.g. {@link Numeric#isFloatingPoint()}). Subclasses of these should, in + * turn, be concrete and implement specific ways of reading the given values (e.g. script and field based sources). It is also possible + * to see sub-sub-classes of ValuesSource that act as wrappers on other concrete values sources to add functionality, such as the + * anonymous subclasses returned from {@link MissingValues} or the GeoPoint to Numeric conversion logic in + * {@link org.elasticsearch.search.aggregations.bucket.geogrid.CellIdSource} + */ public abstract class ValuesSource { /** diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java index f7bdecfa3e448..a7c423d658683 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregationBuilder.java @@ -334,6 +334,15 @@ protected final ValuesSourceAggregatorFactory doBuild(QueryShardContext querySha */ protected abstract ValuesSourceType defaultValueSourceType(); + /** + * Aggregations should override this if they need non-standard logic for resolving where to get values from. For example, join + * aggregations (like Parent and Child) ask the user to specify one side of the join and then look up the other field to read values + * from. + * + * The default implementation just uses the field and/or script the user provided. + * + * @return A {@link ValuesSourceConfig} configured based on the parsed field and/or script. + */ protected ValuesSourceConfig resolveConfig(QueryShardContext queryShardContext) { return ValuesSourceConfig.resolve(queryShardContext, this.userValueTypeHint, field, script, missing, timeZone, format, this.defaultValueSourceType(), this.getType()); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java index 040ede186c7ab..ab995f9c53b5d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java @@ -26,10 +26,23 @@ import java.util.function.LongSupplier; /** - * ValuesSourceType wraps the creation of specific per-source instances each {@link ValuesSource} needs to provide. Every top-level - * subclass of {@link ValuesSource} should have a corresponding implementation of this interface. In general, new data types seeking - * aggregation support should create a top level {@link ValuesSource}, then implement this to return wrappers for the specific sources of - * values. + * {@link ValuesSourceType} represents a collection of fields that share a common set of operations, for example all numeric fields. + * Aggregations declare their support for a given ValuesSourceType (via {@link ValuesSourceRegistry#register}), and should then not need + * to care about the fields which use that ValuesSourceType. + * + * ValuesSourceTypes provide a set of methods to instantiate concrete {@link ValuesSource} instances, based on the actual source of the + * data for the aggregations. In general, aggregations should not call these methods, but rather rely on {@link ValuesSourceConfig} to have + * selected the correct implementation. + * + * ValuesSourceTypes should be stateless. We recommend that plugins define an enum for their ValuesSourceTypes, even if the plugin only + * intends to define one ValuesSourceType. ValuesSourceTypes are not serialized as part of the aggregations framework. + * + * Prefer reusing an existing ValuesSourceType (ideally from {@link CoreValuesSourceType}) over creating a new type. There are some cases + * where creating a new type is necessary however. In particular, consider a new ValuesSourceType if the field has custom encoding/decoding + * requirements; if the field needs to expose additional information to the aggregation (e.g. {@link ValuesSource.Range#rangeType()}); or + * if logically the type needs a more restricted use (e.g. even though dates are stored as numbers, it doesn't make sense to pass them to + * a sum aggregation). When adding a new ValuesSourceType, new aggregators should be added and registered at the same time, to add support + * for the new type to existing aggregations, as appropriate. */ public interface ValuesSourceType { /** @@ -66,11 +79,11 @@ public interface ValuesSourceType { * @param valuesSource - The original {@link ValuesSource} * @param rawMissing - The missing value we got from the parser, typically a string or number * @param docValueFormat - The format to use for further parsing the user supplied value, e.g. a date format - * @param now - Used in conjunction with the formatter, should return the current time in milliseconds + * @param nowSupplier - Used in conjunction with the formatter, should return the current time in milliseconds * @return - Wrapper over the provided {@link ValuesSource} to apply the given missing value */ ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, - LongSupplier now); + LongSupplier nowSupplier); /** * This method provides a hook for specifying a type-specific formatter. When {@link ValuesSourceConfig} can resolve a diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/package-info.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/package-info.java new file mode 100644 index 0000000000000..da5469e873133 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/package-info.java @@ -0,0 +1,77 @@ +/* + * 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.support; + +/** + *

+ * This package holds shared code for the aggregations framework, especially around dealing with values. + *

+ * + *

Key Classes

+ * + *

{@link org.elasticsearch.search.aggregations.support.ValuesSource} and its subclasses

+ *

+ * These are thin wrappers which provide a unified interface to different ways of getting input data (e.g. DocValues from Lucene, or script + * output). A class hierarchy defines the type of values returned by the source. The top level sub-classes define type-specific behavior, + * such as {@link org.elasticsearch.search.aggregations.support.ValuesSource.Numeric#isFloatingPoint()}. Second level subclasses are + * then specialized based on where they read values from, e.g. script or field cases. There are also adapter classes like + * {@link org.elasticsearch.search.aggregations.bucket.geogrid.CellIdSource} which do run-time conversion from one type to another, often + * dependent on a user specified parameter (precision in that case). + *

+ * + *

{@link org.elasticsearch.search.aggregations.support.ValuesSourceRegistry}

+ *

+ * ValuesSourceRegistry stores the mappings for what types are supported by what aggregations. It is configured at startup, when + * {@link org.elasticsearch.search.SearchModule} is configuring aggregations. It shouldn't be necessary to access the registry in most + * cases, but you can get a read copy from {@link org.elasticsearch.index.query.QueryShardContext#getValuesSourceRegistry()} if necessary. + *

+ * + *

{@link org.elasticsearch.search.aggregations.support.ValuesSourceType}

+ *

+ * ValuesSourceTypes are the quantum of support in the aggregations framework, and provide a common language between fields and + * aggregations. Fields which support aggregation override {@link org.elasticsearch.index.mapper.MappedFieldType#getValuesSourceType()} to + * return a compatible VaulesSourceType (based on how the field is stored), and aggregations register what types they support via one of the + * {@link org.elasticsearch.search.aggregations.support.ValuesSourceRegistry#register} methods. The VaulesSourceType itself holds + * information on how to with values of that type, including methods for creating + * {@link org.elasticsearch.search.aggregations.support.ValuesSource} instances and {@link org.elasticsearch.search.DocValueFormat} + * instances. + *

+ * + *

{@link org.elasticsearch.search.aggregations.support.ValuesSourceConfig}

+ *

+ * There are two things going on in ValuesSourceConfig. First, there is a collection of static factory methods to build valid configs for + * different situations. {@link org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder#resolveConfig} has a good + * default for what to call here and generally aggregations shouldn't need to deviate from that. + *

+ * + *

+ * Once properly constructed, the ValuesSourceConfig provides access to the ValuesSource instance, as well as related information like the + * formatter. Aggregations are free to use this information as needed, such as Max and Min which inspect the field context to see if they + * can apply an optimization. + *

+ * + *

Classes we are trying to phase out

+ *

{@link org.elasticsearch.search.aggregations.support.ValueType}

+ *

+ * This class is primarily used for parsing user type hints, and is deprecated for new use. Work is ongoing to remove it from existing + * code. + *

+ * + */