From 3b48465228474d76935d991bf859a3e0ce056f3b Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 27 Feb 2020 13:08:49 -0500 Subject: [PATCH 1/3] Initial wire up - tests still failing --- .../elasticsearch/search/SearchModule.java | 8 ++- .../range/AbstractRangeAggregatorFactory.java | 57 ++++++++++++++----- .../range/DateRangeAggregationBuilder.java | 5 ++ .../range/DateRangeAggregatorFactory.java | 19 ++++--- .../bucket/range/RangeAggregationBuilder.java | 5 ++ .../range/RangeAggregationSupplier.java | 45 +++++++++++++++ .../bucket/range/RangeAggregatorFactory.java | 15 +++-- 7 files changed, 126 insertions(+), 28 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationSupplier.java diff --git a/server/src/main/java/org/elasticsearch/search/SearchModule.java b/server/src/main/java/org/elasticsearch/search/SearchModule.java index e6db21648cc72..f07a844dfa5a3 100644 --- a/server/src/main/java/org/elasticsearch/search/SearchModule.java +++ b/server/src/main/java/org/elasticsearch/search/SearchModule.java @@ -413,9 +413,13 @@ private void registerAggregations(List plugins) { registerAggregation(new AggregationSpec(SignificantTextAggregationBuilder.NAME, SignificantTextAggregationBuilder::new, SignificantTextAggregationBuilder::parse)); registerAggregation(new AggregationSpec(RangeAggregationBuilder.NAME, RangeAggregationBuilder::new, - RangeAggregationBuilder::parse).addResultReader(InternalRange::new)); + RangeAggregationBuilder::parse) + .addResultReader(InternalRange::new) + .setAggregatorRegistrar(RangeAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(DateRangeAggregationBuilder.NAME, DateRangeAggregationBuilder::new, - DateRangeAggregationBuilder::parse).addResultReader(InternalDateRange::new)); + DateRangeAggregationBuilder::parse) + .addResultReader(InternalDateRange::new) + .setAggregatorRegistrar(DateRangeAggregationBuilder::registerAggregators)); registerAggregation(new AggregationSpec(IpRangeAggregationBuilder.NAME, IpRangeAggregationBuilder::new, IpRangeAggregationBuilder::parse).addResultReader(InternalBinaryRange::new)); registerAggregation(new AggregationSpec(HistogramAggregationBuilder.NAME, HistogramAggregationBuilder::new, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java index 87068a1643522..01bb6817b9dad 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.bucket.range; 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; @@ -27,10 +28,13 @@ import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range; import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Unmapped; 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; @@ -42,20 +46,44 @@ public class AbstractRangeAggregatorFactory extends ValuesSourc private final InternalRange.Factory rangeFactory; private final R[] ranges; private final boolean keyed; + private final String aggregationTypeName; + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry, String aggregationName) { + valuesSourceRegistry.register(aggregationName, + List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN), + new RangeAggregationSupplier() { + @Override + public Aggregator build(String name, + AggregatorFactories factories, + Numeric valuesSource, + DocValueFormat format, + InternalRange.Factory rangeFactory, + Range[] ranges, + boolean keyed, + SearchContext context, + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException { + return new RangeAggregator(name, factories, valuesSource, format, rangeFactory, ranges, keyed, context, parent, + pipelineAggregators, metaData); + } + }); + } public AbstractRangeAggregatorFactory(String name, - ValuesSourceConfig config, - R[] ranges, - boolean keyed, - InternalRange.Factory rangeFactory, - QueryShardContext queryShardContext, - AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, - Map metaData) throws IOException { + String aggregationTypeName, + ValuesSourceConfig config, + R[] ranges, + boolean keyed, + InternalRange.Factory rangeFactory, + QueryShardContext queryShardContext, + AggregatorFactory parent, + AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { super(name, config, queryShardContext, parent, subFactoriesBuilder, metaData); this.ranges = ranges; this.keyed = keyed; this.rangeFactory = rangeFactory; + this.aggregationTypeName = aggregationTypeName; } @Override @@ -73,12 +101,15 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, 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()); + + AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config.valueSourceType(), + aggregationTypeName); + if (aggregatorSupplier instanceof RangeAggregationSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected HistogramAggregatorSupplier, found [" + + aggregatorSupplier.getClass().toString() + "]"); } - return new RangeAggregator(name, factories, (Numeric) valuesSource, config.format(), rangeFactory, ranges, keyed, searchContext, - parent, pipelineAggregators, metaData); + return ((RangeAggregationSupplier)aggregatorSupplier).build(name, factories, (Numeric) valuesSource, config.format(), rangeFactory, + ranges, keyed, searchContext, parent, pipelineAggregators, metaData); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java index 0aa2c5116d50f..c6590b42ff32b 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregationBuilder.java @@ -30,6 +30,7 @@ import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; 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; @@ -60,6 +61,10 @@ private static RangeAggregator.Range parseRange(XContentParser parser) throws IO return RangeAggregator.Range.fromXContent(parser); } + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + AbstractRangeAggregatorFactory.registerAggregators(valuesSourceRegistry, NAME); + } + public DateRangeAggregationBuilder(String name) { super(name, InternalDateRange.FACTORY); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorFactory.java index 1e5f742f3f963..c981c13c839a9 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorFactory.java @@ -30,15 +30,16 @@ public class DateRangeAggregatorFactory extends AbstractRangeAggregatorFactory { public DateRangeAggregatorFactory(String name, - ValuesSourceConfig config, - RangeAggregator.Range[] ranges, - boolean keyed, - InternalRange.Factory rangeFactory, - QueryShardContext queryShardContext, - AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, - Map metaData) throws IOException { - super(name, config, ranges, keyed, rangeFactory, queryShardContext, parent, subFactoriesBuilder, metaData); + ValuesSourceConfig config, + RangeAggregator.Range[] ranges, + boolean keyed, + InternalRange.Factory rangeFactory, + QueryShardContext queryShardContext, + AggregatorFactory parent, + AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { + super(name, DateRangeAggregationBuilder.NAME, config, ranges, keyed, rangeFactory, queryShardContext, parent, subFactoriesBuilder, + metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java index d50a1e070c502..2a679a0161d18 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationBuilder.java @@ -30,6 +30,7 @@ import org.elasticsearch.search.aggregations.bucket.range.RangeAggregator.Range; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.aggregations.support.ValuesSourceParserHelper; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import java.io.IOException; import java.util.Map; @@ -58,6 +59,10 @@ private static Range parseRange(XContentParser parser) throws IOException { return Range.fromXContent(parser); } + public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry) { + AbstractRangeAggregatorFactory.registerAggregators(valuesSourceRegistry, NAME); + } + public RangeAggregationBuilder(String name) { super(name, InternalRange.FACTORY); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationSupplier.java new file mode 100644 index 0000000000000..5e369c2ebfd10 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationSupplier.java @@ -0,0 +1,45 @@ +/* + * 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.range; + +import org.elasticsearch.search.DocValueFormat; +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; + +public interface RangeAggregationSupplier extends AggregatorSupplier { + Aggregator build(String name, + AggregatorFactories factories, + ValuesSource.Numeric valuesSource, + DocValueFormat format, + InternalRange.Factory rangeFactory, + RangeAggregator.Range[] ranges, + boolean keyed, + SearchContext context, + Aggregator parent, + List pipelineAggregators, + Map metaData) throws IOException; +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java index cee6838f0f18d..7d30605d19646 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorFactory.java @@ -31,10 +31,17 @@ public class RangeAggregatorFactory extends AbstractRangeAggregatorFactory { - public RangeAggregatorFactory(String name, ValuesSourceConfig config, Range[] ranges, boolean keyed, - Factory rangeFactory, QueryShardContext queryShardContext, AggregatorFactory parent, - AggregatorFactories.Builder subFactoriesBuilder, Map metaData) throws IOException { - super(name, config, ranges, keyed, rangeFactory, queryShardContext, parent, subFactoriesBuilder, metaData); + public RangeAggregatorFactory(String name, + ValuesSourceConfig config, + Range[] ranges, + boolean keyed, + Factory rangeFactory, + QueryShardContext queryShardContext, + AggregatorFactory parent, + AggregatorFactories.Builder subFactoriesBuilder, + Map metaData) throws IOException { + super(name, RangeAggregationBuilder.NAME, config, ranges, keyed, rangeFactory, queryShardContext, parent, subFactoriesBuilder, + metaData); } } From 8098ec5058f9e1bd2ca9a93e6cf5ddcacedc3dc7 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 27 Feb 2020 15:08:14 -0500 Subject: [PATCH 2/3] Fix exception messages in tests --- .../bucket/range/AbstractRangeAggregatorFactory.java | 2 -- .../aggregations/bucket/range/DateRangeAggregatorTests.java | 4 ++-- .../aggregations/bucket/range/RangeAggregatorTests.java | 2 +- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java index 01bb6817b9dad..7d182767c0ae2 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java @@ -111,6 +111,4 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, return ((RangeAggregationSupplier)aggregatorSupplier).build(name, factories, (Numeric) valuesSource, config.format(), rangeFactory, ranges, keyed, searchContext, parent, pipelineAggregators, metaData); } - - } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorTests.java index 2746e69be29de..2fb281170b732 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/DateRangeAggregatorTests.java @@ -224,8 +224,8 @@ public void testKeywordField() { () -> testCase(aggregationBuilder, new MatchAllDocsQuery(), iw -> { iw.addDocument(singleton(new SortedSetDocValuesField("string", new BytesRef("foo")))); }, range -> fail("Should have thrown exception"), fieldType)); - // I believe this error is coming from improperly parsing the range, not the field. - assertEquals("For input string: \"2015-11-13\"", e.getMessage()); + assertEquals("Field [not_a_number] of type [keyword(indexed,tokenized)] is not supported for aggregation [date_range]", + e.getMessage()); } public void testBadMissingField() { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java index 2b3571ffaac85..092a0319c9c21 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorTests.java @@ -238,7 +238,7 @@ public void testUnsupportedType() { () -> testCase(aggregationBuilder, new MatchAllDocsQuery(), iw -> { iw.addDocument(singleton(new SortedSetDocValuesField("string", new BytesRef("foo")))); }, range -> fail("Should have thrown exception"), fieldType)); - assertEquals(e.getMessage(), "Expected numeric type on field [not_a_number], but got [keyword]"); + assertEquals("Field [not_a_number] of type [keyword(indexed,tokenized)] is not supported for aggregation [range]", e.getMessage()); } public void testBadMissingField() { From 60dca7c1d42add089266689783df4e5090d2468f Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 4 Mar 2020 15:52:33 -0500 Subject: [PATCH 3/3] fixed error message, supplier name --- .../bucket/range/AbstractRangeAggregatorFactory.java | 8 ++++---- ...regationSupplier.java => RangeAggregatorSupplier.java} | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) rename server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/{RangeAggregationSupplier.java => RangeAggregatorSupplier.java} (96%) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java index 7d182767c0ae2..6e7974f68d605 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/AbstractRangeAggregatorFactory.java @@ -51,7 +51,7 @@ public class AbstractRangeAggregatorFactory extends ValuesSourc public static void registerAggregators(ValuesSourceRegistry valuesSourceRegistry, String aggregationName) { valuesSourceRegistry.register(aggregationName, List.of(CoreValuesSourceType.NUMERIC, CoreValuesSourceType.DATE, CoreValuesSourceType.BOOLEAN), - new RangeAggregationSupplier() { + new RangeAggregatorSupplier() { @Override public Aggregator build(String name, AggregatorFactories factories, @@ -104,11 +104,11 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, AggregatorSupplier aggregatorSupplier = queryShardContext.getValuesSourceRegistry().getAggregator(config.valueSourceType(), aggregationTypeName); - if (aggregatorSupplier instanceof RangeAggregationSupplier == false) { - throw new AggregationExecutionException("Registry miss-match - expected HistogramAggregatorSupplier, found [" + + if (aggregatorSupplier instanceof RangeAggregatorSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected RangeAggregatorSupplier, found [" + aggregatorSupplier.getClass().toString() + "]"); } - return ((RangeAggregationSupplier)aggregatorSupplier).build(name, factories, (Numeric) valuesSource, config.format(), rangeFactory, + return ((RangeAggregatorSupplier)aggregatorSupplier).build(name, factories, (Numeric) valuesSource, config.format(), rangeFactory, ranges, keyed, searchContext, parent, pipelineAggregators, metaData); } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorSupplier.java similarity index 96% rename from server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationSupplier.java rename to server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorSupplier.java index 5e369c2ebfd10..2594396e33a3f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregationSupplier.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/range/RangeAggregatorSupplier.java @@ -30,7 +30,7 @@ import java.util.List; import java.util.Map; -public interface RangeAggregationSupplier extends AggregatorSupplier { +public interface RangeAggregatorSupplier extends AggregatorSupplier { Aggregator build(String name, AggregatorFactories factories, ValuesSource.Numeric valuesSource,