From 765a631bd9268025d5f81f23b1e5eccdc4a8fe80 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 23 Oct 2019 16:35:45 -0400 Subject: [PATCH 01/13] Prototype for registry to aggregator factory interaction --- .../histogram/HistogramAggregatorFactory.java | 28 +++---- .../histogram/NumericHistogramAggregator.java | 4 +- .../histogram/RangeHistogramAggregator.java | 2 +- .../support/AggregatorSupplier.java | 22 +++++ .../support/HistogramAggregatorSupplier.java | 39 +++++++++ .../support/ValuesSourceRegistry.java | 83 +++++++++++++++++++ 6 files changed, 159 insertions(+), 19 deletions(-) create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/support/AggregatorSupplier.java create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/support/HistogramAggregatorSupplier.java create mode 100644 server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.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 670b8008bc512..d0cfec112f427 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 @@ -20,14 +20,18 @@ package org.elasticsearch.search.aggregations.bucket.histogram; 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.BucketOrder; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.aggregations.support.AggregatorSupplier; +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; +import org.elasticsearch.search.aggregations.support.ValuesSourceRegistry; import org.elasticsearch.search.internal.SearchContext; import java.io.IOException; @@ -92,23 +96,15 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, if (collectsFromSingleBucket == false) { return asMultiBucketAggregator(this, searchContext, parent); } - if (valuesSource instanceof ValuesSource.Numeric) { - return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, - (ValuesSource.Numeric) valuesSource, config.format(), searchContext, parent, pipelineAggregators, metaData); - } else if (valuesSource instanceof ValuesSource.Range) { - ValuesSource.Range rangeValueSource = (ValuesSource.Range) valuesSource; - if (rangeValueSource.rangeType().isNumeric() == false) { - throw new IllegalArgumentException("Expected numeric range type but found non-numeric range [" - + rangeValueSource.rangeType().name + "]"); - } - return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, - (ValuesSource.Range) valuesSource, config.format(), searchContext, parent, pipelineAggregators, - metaData); - } - else { - throw new IllegalArgumentException("Expected one of [Numeric, Range] values source, found [" - + valuesSource.toString() + "]"); + + AggregatorSupplier aggregatorSupplier = ValuesSourceRegistry.INSTANCE.getAggregator(valuesSource, HistogramAggregationBuilder.NAME); + if (aggregatorSupplier instanceof HistogramAggregatorSupplier == false) { + throw new AggregationExecutionException("Registry miss-match - expected HistogramAggregatorSupplier, found [" + + aggregatorSupplier.getClass().toString() + "]"); } + HistogramAggregatorSupplier histogramAggregatorSupplier = (HistogramAggregatorSupplier) aggregatorSupplier; + return histogramAggregatorSupplier.build(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, + valuesSource, config.format(), searchContext, parent, pipelineAggregators, metaData); } @Override diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java index b63cf94a98085..b8e597ccf7462 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java @@ -52,7 +52,7 @@ * written as {@code interval * x + offset} and yet is less than or equal to * {@code value}. */ -class NumericHistogramAggregator extends BucketsAggregator { +public class NumericHistogramAggregator extends BucketsAggregator { private final ValuesSource.Numeric valuesSource; private final DocValueFormat formatter; @@ -64,7 +64,7 @@ class NumericHistogramAggregator extends BucketsAggregator { private final LongHash bucketOrds; - NumericHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, + public NumericHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, @Nullable ValuesSource.Numeric valuesSource, DocValueFormat formatter, SearchContext context, Aggregator parent, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java index 1a722dc951418..8d11286b50092 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/RangeHistogramAggregator.java @@ -58,7 +58,7 @@ public class RangeHistogramAggregator extends BucketsAggregator { private final LongHash bucketOrds; - RangeHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, + public RangeHistogramAggregator(String name, AggregatorFactories factories, double interval, double offset, BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, @Nullable ValuesSource.Range valuesSource, DocValueFormat formatter, SearchContext context, Aggregator parent, 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 new file mode 100644 index 0000000000000..97ca793faedb3 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/AggregatorSupplier.java @@ -0,0 +1,22 @@ +/* + * 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; + +public interface AggregatorSupplier { +} diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/HistogramAggregatorSupplier.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/HistogramAggregatorSupplier.java new file mode 100644 index 0000000000000..021577852c39e --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/HistogramAggregatorSupplier.java @@ -0,0 +1,39 @@ +/* + * 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; + +import org.elasticsearch.common.Nullable; +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.Aggregator; +import org.elasticsearch.search.aggregations.AggregatorFactories; +import org.elasticsearch.search.aggregations.BucketOrder; +import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.internal.SearchContext; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +public interface HistogramAggregatorSupplier extends AggregatorSupplier { + Aggregator build(String name, AggregatorFactories factories, double interval, double offset, + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + @Nullable ValuesSource valuesSource, DocValueFormat formatter, + SearchContext context, Aggregator parent, + List pipelineAggregators, Map metaData) throws IOException; +} 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 new file mode 100644 index 0000000000000..87682cc826ef2 --- /dev/null +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceRegistry.java @@ -0,0 +1,83 @@ +/* + * 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; + +import org.elasticsearch.search.DocValueFormat; +import org.elasticsearch.search.aggregations.AggregationExecutionException; +import org.elasticsearch.search.aggregations.Aggregator; +import org.elasticsearch.search.aggregations.AggregatorFactories; +import org.elasticsearch.search.aggregations.BucketOrder; +import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; +import org.elasticsearch.search.aggregations.bucket.histogram.NumericHistogramAggregator; +import org.elasticsearch.search.aggregations.bucket.histogram.RangeHistogramAggregator; +import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; +import org.elasticsearch.search.internal.SearchContext; + +import java.io.IOException; +import java.util.List; +import java.util.Map; + +/* +This is a _very_ crude prototype for the ValuesSourceRegistry which basically hard-codes everything. The intent is to define the API +for aggregations using the registry to resolve aggregators. + */ +public enum ValuesSourceRegistry { + INSTANCE { + @Override + public AggregatorSupplier getAggregator(ValuesSource valuesSource, String aggregationName) { + if (aggregationName.equals(HistogramAggregationBuilder.NAME)) { + if (valuesSource instanceof ValuesSource.Numeric) { + return new HistogramAggregatorSupplier() { + @Override + public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + ValuesSource valuesSource, DocValueFormat formatter, SearchContext context, + Aggregator parent, List pipelineAggregators, + Map metaData) throws IOException { + return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, + maxBound, (ValuesSource.Numeric) valuesSource, formatter, context, parent, pipelineAggregators, metaData); + } + }; + } else if (valuesSource instanceof ValuesSource.Range) { + return new HistogramAggregatorSupplier() { + @Override + public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + ValuesSource valuesSource, DocValueFormat formatter, SearchContext context, + Aggregator parent, List pipelineAggregators, + Map metaData) throws IOException { + ValuesSource.Range rangeValueSource = (ValuesSource.Range) valuesSource; + if (rangeValueSource.rangeType().isNumeric() == false) { + throw new IllegalArgumentException("Expected numeric range type but found non-numeric range [" + + rangeValueSource.rangeType().name + "]"); + } + return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, + maxBound, rangeValueSource, formatter, context, parent, pipelineAggregators, metaData); + } + }; + } + } + // TODO: Error message should list valid ValuesSource types + throw new AggregationExecutionException("ValuesSource type " + valuesSource.toString() + " is not supported for aggregation" + + aggregationName); + } + }; + + public abstract AggregatorSupplier getAggregator(ValuesSource valuesSource, String aggregationName); +} From 606ea551d2b5eb159419130227c3640f35ea02d9 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 23 Oct 2019 17:31:15 -0400 Subject: [PATCH 02/13] Prototype what registering an aggregator looks like, more or less --- .../histogram/HistogramAggregatorFactory.java | 42 +++++++++++- .../histogram/NumericHistogramAggregator.java | 4 +- .../support/ValuesSourceRegistry.java | 68 +++++++------------ 3 files changed, 67 insertions(+), 47 deletions(-) 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 d0cfec112f427..2c95418870f80 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 @@ -20,6 +20,7 @@ package org.elasticsearch.search.aggregations.bucket.histogram; 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; @@ -32,6 +33,7 @@ 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 org.elasticsearch.search.internal.SearchContext; import java.io.IOException; @@ -50,6 +52,43 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact private final long minDocCount; private final double minBound, maxBound; + // TODO: Registration should happen on the actual aggregator classes, but I don't want to set up the whole dynamic loading thing yet + static { + ValuesSourceRegistry.INSTANCE.register(HistogramAggregationBuilder.NAME, ValuesSourceType.RANGE, + new HistogramAggregatorSupplier() { + @Override + public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + ValuesSource valuesSource, DocValueFormat formatter, SearchContext context, + Aggregator parent, List pipelineAggregators, + Map metaData) throws IOException { + ValuesSource.Range rangeValueSource = (ValuesSource.Range) valuesSource; + if (rangeValueSource.rangeType().isNumeric() == false) { + throw new IllegalArgumentException("Expected numeric range type but found non-numeric range [" + + rangeValueSource.rangeType().name + "]"); + } + return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, + maxBound, rangeValueSource, formatter, context, parent, pipelineAggregators, metaData); + } + } + ); + + ValuesSourceRegistry.INSTANCE.register(HistogramAggregationBuilder.NAME, ValuesSourceType.NUMERIC, + new HistogramAggregatorSupplier() { + @Override + public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, + BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, + ValuesSource valuesSource, DocValueFormat formatter, SearchContext context, + Aggregator parent, List pipelineAggregators, + Map metaData) throws IOException { + return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, + maxBound, (ValuesSource.Numeric) valuesSource, formatter, context, parent, pipelineAggregators, metaData); + } + } + ); + } + + @Override protected ValuesSource resolveMissingAny(Object missing) { if (missing instanceof Number) { @@ -97,7 +136,8 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, return asMultiBucketAggregator(this, searchContext, parent); } - AggregatorSupplier aggregatorSupplier = ValuesSourceRegistry.INSTANCE.getAggregator(valuesSource, HistogramAggregationBuilder.NAME); + AggregatorSupplier aggregatorSupplier = ValuesSourceRegistry.INSTANCE.getAggregator(config.valueSourceType(), + HistogramAggregationBuilder.NAME); if (aggregatorSupplier instanceof HistogramAggregatorSupplier == false) { throw new AggregationExecutionException("Registry miss-match - expected HistogramAggregatorSupplier, found [" + aggregatorSupplier.getClass().toString() + "]"); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java index b8e597ccf7462..e14a4cd4a23e8 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregator.java @@ -29,14 +29,14 @@ import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.Aggregator; import org.elasticsearch.search.aggregations.AggregatorFactories; +import org.elasticsearch.search.aggregations.BucketOrder; import org.elasticsearch.search.aggregations.InternalAggregation; +import org.elasticsearch.search.aggregations.InternalOrder; import org.elasticsearch.search.aggregations.LeafBucketCollector; import org.elasticsearch.search.aggregations.LeafBucketCollectorBase; import org.elasticsearch.search.aggregations.bucket.BucketsAggregator; import org.elasticsearch.search.aggregations.bucket.histogram.InternalHistogram.EmptyBucketInfo; import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import org.elasticsearch.search.aggregations.BucketOrder; -import org.elasticsearch.search.aggregations.InternalOrder; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.internal.SearchContext; 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 87682cc826ef2..b7a1916981314 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 @@ -18,19 +18,9 @@ */ package org.elasticsearch.search.aggregations.support; -import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationExecutionException; -import org.elasticsearch.search.aggregations.Aggregator; -import org.elasticsearch.search.aggregations.AggregatorFactories; -import org.elasticsearch.search.aggregations.BucketOrder; -import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder; -import org.elasticsearch.search.aggregations.bucket.histogram.NumericHistogramAggregator; -import org.elasticsearch.search.aggregations.bucket.histogram.RangeHistogramAggregator; -import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator; -import org.elasticsearch.search.internal.SearchContext; -import java.io.IOException; -import java.util.List; +import java.util.HashMap; import java.util.Map; /* @@ -39,45 +29,35 @@ */ public enum ValuesSourceRegistry { INSTANCE { + Map> aggregatorRegistry = new HashMap<>(); + + @Override + public void register(String aggregationName, ValuesSourceType valuesSourceType, AggregatorSupplier aggregatorSupplier) { + if (aggregatorRegistry.containsKey(aggregationName) == false) { + aggregatorRegistry.put(aggregationName, new HashMap<>()); + } + Map innerMap = aggregatorRegistry.get(aggregationName); + if (innerMap.containsKey(valuesSourceType)) { + throw new IllegalStateException("Attempted to register already registered pair [" + aggregationName + ", " + + valuesSourceType.toString() + "]"); + } + innerMap.put(valuesSourceType, aggregatorSupplier); + } + @Override - public AggregatorSupplier getAggregator(ValuesSource valuesSource, String aggregationName) { - if (aggregationName.equals(HistogramAggregationBuilder.NAME)) { - if (valuesSource instanceof ValuesSource.Numeric) { - return new HistogramAggregatorSupplier() { - @Override - public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, - BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, - ValuesSource valuesSource, DocValueFormat formatter, SearchContext context, - Aggregator parent, List pipelineAggregators, - Map metaData) throws IOException { - return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, - maxBound, (ValuesSource.Numeric) valuesSource, formatter, context, parent, pipelineAggregators, metaData); - } - }; - } else if (valuesSource instanceof ValuesSource.Range) { - return new HistogramAggregatorSupplier() { - @Override - public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, - BucketOrder order, boolean keyed, long minDocCount, double minBound, double maxBound, - ValuesSource valuesSource, DocValueFormat formatter, SearchContext context, - Aggregator parent, List pipelineAggregators, - Map metaData) throws IOException { - ValuesSource.Range rangeValueSource = (ValuesSource.Range) valuesSource; - if (rangeValueSource.rangeType().isNumeric() == false) { - throw new IllegalArgumentException("Expected numeric range type but found non-numeric range [" - + rangeValueSource.rangeType().name + "]"); - } - return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, - maxBound, rangeValueSource, formatter, context, parent, pipelineAggregators, metaData); - } - }; + public AggregatorSupplier getAggregator(ValuesSourceType valuesSourceType, String aggregationName) { + if (aggregatorRegistry.containsKey(aggregationName)) { + Map innerMap = aggregatorRegistry.get(aggregationName); + if (innerMap.containsKey(valuesSourceType)) { + return innerMap.get(valuesSourceType); } } // TODO: Error message should list valid ValuesSource types - throw new AggregationExecutionException("ValuesSource type " + valuesSource.toString() + " is not supported for aggregation" + + throw new AggregationExecutionException("ValuesSource type " + valuesSourceType.toString() + " is not supported for aggregation" + aggregationName); } }; - public abstract AggregatorSupplier getAggregator(ValuesSource valuesSource, String aggregationName); + public abstract void register(String aggregationName, ValuesSourceType valuesSourceType, AggregatorSupplier aggregatorSupplier); + public abstract AggregatorSupplier getAggregator(ValuesSourceType valuesSourceType, String aggregationName); } From 3f7aea2798229bcca41043f8542bda906dbfbfcf Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 30 Oct 2019 13:46:36 -0400 Subject: [PATCH 03/13] Pass aggregation name to resolve --- .../support/ValuesSourceAggregationBuilder.java | 2 +- .../aggregations/support/ValuesSourceConfig.java | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) 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 9c8cacc7859e0..57a28a028d136 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 @@ -338,7 +338,7 @@ protected ValueType defaultValueType(Script script) { protected ValuesSourceConfig resolveConfig(QueryShardContext queryShardContext) { ValueType valueType = this.valueType != null ? this.valueType : targetValueType; return ValuesSourceConfig.resolve(queryShardContext, - valueType, field, script, missing, timeZone, format, this::resolveScriptAny); + valueType, field, script, missing, timeZone, format, this::resolveScriptAny, this.getType()); } protected abstract ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardContext, diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index d906260c75694..b9a65cb554024 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -54,10 +54,10 @@ public static ValuesSourceConfig resolve( String field, Script script, Object missing, ZoneId timeZone, - String format) { - return resolve(context, valueType, field, script, missing, timeZone, format, s -> ValuesSourceType.BYTES); + String format, String aggregationName) { + return resolve(context, valueType, field, script, missing, timeZone, format, s -> ValuesSourceType.BYTES, aggregationName); } - + /** * Resolve a {@link ValuesSourceConfig} given configuration parameters. */ @@ -68,8 +68,8 @@ public static ValuesSourceConfig resolve( Object missing, ZoneId timeZone, String format, - Function resolveScriptAny - ) { + Function resolveScriptAny, + String aggregationName) { if (field == null) { if (script == null) { From c4df79f9d9002ddb11164d0f6e0442a314e8d567 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 30 Oct 2019 13:53:39 -0400 Subject: [PATCH 04/13] Future-proof access to registry instance --- .../bucket/histogram/HistogramAggregatorFactory.java | 6 +++--- .../search/aggregations/support/ValuesSourceConfig.java | 2 +- .../search/aggregations/support/ValuesSourceRegistry.java | 2 ++ 3 files changed, 6 insertions(+), 4 deletions(-) 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 2c95418870f80..28e41cd5e178f 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 @@ -54,7 +54,7 @@ public final class HistogramAggregatorFactory extends ValuesSourceAggregatorFact // TODO: Registration should happen on the actual aggregator classes, but I don't want to set up the whole dynamic loading thing yet static { - ValuesSourceRegistry.INSTANCE.register(HistogramAggregationBuilder.NAME, ValuesSourceType.RANGE, + ValuesSourceRegistry.getInstance().register(HistogramAggregationBuilder.NAME, ValuesSourceType.RANGE, new HistogramAggregatorSupplier() { @Override public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, @@ -73,7 +73,7 @@ public Aggregator build(String name, AggregatorFactories factories, double inter } ); - ValuesSourceRegistry.INSTANCE.register(HistogramAggregationBuilder.NAME, ValuesSourceType.NUMERIC, + ValuesSourceRegistry.getInstance().register(HistogramAggregationBuilder.NAME, ValuesSourceType.NUMERIC, new HistogramAggregatorSupplier() { @Override public Aggregator build(String name, AggregatorFactories factories, double interval, double offset, @@ -136,7 +136,7 @@ protected Aggregator doCreateInternal(ValuesSource valuesSource, return asMultiBucketAggregator(this, searchContext, parent); } - AggregatorSupplier aggregatorSupplier = ValuesSourceRegistry.INSTANCE.getAggregator(config.valueSourceType(), + AggregatorSupplier aggregatorSupplier = ValuesSourceRegistry.getInstance().getAggregator(config.valueSourceType(), HistogramAggregationBuilder.NAME); if (aggregatorSupplier instanceof HistogramAggregatorSupplier == false) { throw new AggregationExecutionException("Registry miss-match - expected HistogramAggregatorSupplier, found [" + diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index b9a65cb554024..7f709648ecd15 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -57,7 +57,7 @@ public static ValuesSourceConfig resolve( String format, String aggregationName) { return resolve(context, valueType, field, script, missing, timeZone, format, s -> ValuesSourceType.BYTES, aggregationName); } - + /** * Resolve a {@link ValuesSourceConfig} given configuration parameters. */ 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 b7a1916981314..24e91fd15d45d 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 @@ -60,4 +60,6 @@ public AggregatorSupplier getAggregator(ValuesSourceType valuesSourceType, Strin public abstract void register(String aggregationName, ValuesSourceType valuesSourceType, AggregatorSupplier aggregatorSupplier); public abstract AggregatorSupplier getAggregator(ValuesSourceType valuesSourceType, String aggregationName); + + public static ValuesSourceRegistry getInstance() {return INSTANCE;} } From 268cb9e58ea50ef7d738c032b54a42aa1cc976dc Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 31 Oct 2019 10:30:26 -0400 Subject: [PATCH 05/13] Prototype for selecting values source type from the registry --- .../CompositeValuesSourceBuilder.java | 2 +- .../histogram/HistogramAggregatorFactory.java | 8 ++- .../MultiValuesSourceAggregationBuilder.java | 2 +- .../support/ValuesSourceConfig.java | 17 +---- .../support/ValuesSourceRegistry.java | 65 +++++++++++++++++-- .../support/ValuesSourceConfigTests.java | 35 +++++----- 6 files changed, 90 insertions(+), 39 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java index 70687cf9efe0c..98b42c640df6e 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSourceBuilder.java @@ -276,7 +276,7 @@ protected abstract CompositeValuesSourceConfig innerBuild(QueryShardContext quer public final CompositeValuesSourceConfig build(QueryShardContext queryShardContext) throws IOException { ValuesSourceConfig config = ValuesSourceConfig.resolve(queryShardContext, - valueType, field, script, null,null, format); + valueType, field, script, null,null, format, name()); return innerBuild(queryShardContext, config); } } 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 28e41cd5e178f..dda9b3bcbc264 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 @@ -19,6 +19,8 @@ package org.elasticsearch.search.aggregations.bucket.histogram; +import org.elasticsearch.index.fielddata.IndexNumericFieldData; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.aggregations.AggregationExecutionException; @@ -70,7 +72,8 @@ public Aggregator build(String name, AggregatorFactories factories, double inter return new RangeHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, rangeValueSource, formatter, context, parent, pipelineAggregators, metaData); } - } + }, + (fieldType, indexFieldData) -> fieldType instanceof RangeFieldMapper.RangeFieldType ); ValuesSourceRegistry.getInstance().register(HistogramAggregationBuilder.NAME, ValuesSourceType.NUMERIC, @@ -84,7 +87,8 @@ public Aggregator build(String name, AggregatorFactories factories, double inter return new NumericHistogramAggregator(name, factories, interval, offset, order, keyed, minDocCount, minBound, maxBound, (ValuesSource.Numeric) valuesSource, formatter, context, parent, pipelineAggregators, metaData); } - } + }, + (fieldType, indexFieldData) -> indexFieldData instanceof IndexNumericFieldData ); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java index efeed0c9efb39..f654b876127a1 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSourceAggregationBuilder.java @@ -170,7 +170,7 @@ protected final MultiValuesSourceAggregatorFactory doBuild(QueryShardContext Map> configs = new HashMap<>(fields.size()); fields.forEach((key, value) -> { ValuesSourceConfig config = ValuesSourceConfig.resolve(queryShardContext, finalValueType, - value.getFieldName(), value.getScript(), value.getMissing(), value.getTimeZone(), format); + value.getFieldName(), value.getScript(), value.getMissing(), value.getTimeZone(), format, getType()); configs.put(key, config); }); DocValueFormat docValueFormat = resolveFormat(format, finalValueType); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index 7f709648ecd15..c3d6e4f94a9f0 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -112,20 +112,9 @@ public static ValuesSourceConfig resolve( IndexFieldData indexFieldData = context.getForField(fieldType); ValuesSourceConfig config; - if (indexFieldData instanceof IndexNumericFieldData) { - config = new ValuesSourceConfig<>(ValuesSourceType.NUMERIC); - } else if (indexFieldData instanceof IndexGeoPointFieldData) { - config = new ValuesSourceConfig<>(ValuesSourceType.GEOPOINT); - } else if (fieldType instanceof RangeFieldMapper.RangeFieldType) { - config = new ValuesSourceConfig<>(ValuesSourceType.RANGE); - } else { - if (valueType == null) { - config = new ValuesSourceConfig<>(ValuesSourceType.BYTES); - } else { - config = new ValuesSourceConfig<>(valueType.getValuesSourceType()); - } - } - + ValuesSourceType valuesSourceType = ValuesSourceRegistry.getInstance().getValuesSourceType(fieldType, indexFieldData, + aggregationName, valueType); + config = new ValuesSourceConfig<>(valuesSourceType); config.fieldContext(new FieldContext(field, indexFieldData, fieldType)); config.missing(missing); 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 24e91fd15d45d..3364477f38e80 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 @@ -18,10 +18,19 @@ */ package org.elasticsearch.search.aggregations.support; +import org.elasticsearch.index.fielddata.IndexFieldData; +import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; +import org.elasticsearch.index.fielddata.IndexNumericFieldData; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.search.aggregations.AggregationExecutionException; +import java.util.AbstractMap; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.function.BiFunction; /* This is a _very_ crude prototype for the ValuesSourceRegistry which basically hard-codes everything. The intent is to define the API @@ -30,9 +39,20 @@ public enum ValuesSourceRegistry { INSTANCE { Map> aggregatorRegistry = new HashMap<>(); + // We use a List of Entries here to approximate an ordered map + Map, ValuesSourceType>>> resolverRegistry + = new HashMap<>(); @Override - public void register(String aggregationName, ValuesSourceType valuesSourceType, AggregatorSupplier aggregatorSupplier) { + public void register(String aggregationName, ValuesSourceType valuesSourceType,AggregatorSupplier aggregatorSupplier, + BiFunction resolveValuesSourceType) { + if (resolverRegistry.containsKey(aggregationName) == false) { + resolverRegistry.put(aggregationName, new ArrayList<>()); + } + List, ValuesSourceType>> resolverList + = resolverRegistry.get(aggregationName); + resolverList.add(new AbstractMap.SimpleEntry<>(resolveValuesSourceType, valuesSourceType)); + if (aggregatorRegistry.containsKey(aggregationName) == false) { aggregatorRegistry.put(aggregationName, new HashMap<>()); } @@ -53,13 +73,50 @@ public AggregatorSupplier getAggregator(ValuesSourceType valuesSourceType, Strin } } // TODO: Error message should list valid ValuesSource types - throw new AggregationExecutionException("ValuesSource type " + valuesSourceType.toString() + " is not supported for aggregation" + - aggregationName); + throw new AggregationExecutionException("ValuesSource type " + valuesSourceType.toString() + + " is not supported for aggregation" + aggregationName); + } + + @Override + public ValuesSourceType getValuesSourceType(MappedFieldType fieldType, IndexFieldData indexFieldData, String aggregationName, + ValueType valueType) { + if (resolverRegistry.containsKey(aggregationName)) { + List, ValuesSourceType>> resolverList + = resolverRegistry.get(aggregationName); + for (AbstractMap.SimpleEntry, ValuesSourceType> entry : resolverList) { + BiFunction matcher = entry.getKey(); + if (matcher.apply(fieldType, indexFieldData)) { + return entry.getValue(); + } + } + // TODO: Error message should list valid field types; not sure fieldType.toString() is the best choice. + throw new IllegalArgumentException("Field type " + fieldType.toString() + " is not supported for aggregation " + + aggregationName); + } else { + // TODO: Legacy resolve logic; remove this after converting all aggregations to the new system + if (indexFieldData instanceof IndexNumericFieldData) { + return ValuesSourceType.NUMERIC; + } else if (indexFieldData instanceof IndexGeoPointFieldData) { + return ValuesSourceType.GEOPOINT; + } else if (fieldType instanceof RangeFieldMapper.RangeFieldType) { + return ValuesSourceType.RANGE; + } else { + if (valueType == null) { + return ValuesSourceType.BYTES; + } else { + return valueType.getValuesSourceType(); + } + } + } } }; - public abstract void register(String aggregationName, ValuesSourceType valuesSourceType, AggregatorSupplier aggregatorSupplier); + public abstract void register(String aggregationName, ValuesSourceType valuesSourceType, AggregatorSupplier aggregatorSupplier, + BiFunction resolveValuesSourceType); public abstract AggregatorSupplier getAggregator(ValuesSourceType valuesSourceType, String aggregationName); + // TODO: ValueType argument is only needed for legacy logic + public abstract ValuesSourceType getValuesSourceType(MappedFieldType fieldType, IndexFieldData indexFieldData, String aggregationName, + ValueType valueType); public static ValuesSourceRegistry getInstance() {return INSTANCE;} } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfigTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfigTests.java index 2831aad4da25f..11bffc07579a4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfigTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfigTests.java @@ -31,6 +31,7 @@ import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.test.ESSingleNodeTestCase; +// TODO: This whole set of tests needs to be rethought. public class ValuesSourceConfigTests extends ESSingleNodeTestCase { public void testKeyword() throws Exception { @@ -45,7 +46,7 @@ public void testKeyword() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bytes", null, null, null, null); + context, null, "bytes", null, null, null, null, null); ValuesSource.Bytes valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); @@ -67,14 +68,14 @@ public void testEmptyKeyword() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bytes", null, null, null, null); + context, null, "bytes", null, null, null, null, null); ValuesSource.Bytes valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); assertFalse(values.advanceExact(0)); config = ValuesSourceConfig.resolve( - context, null, "bytes", null, "abc", null, null); + context, null, "bytes", null, "abc", null, null, null); valuesSource = config.toValuesSource(context); values = valuesSource.bytesValues(ctx); assertTrue(values.advanceExact(0)); @@ -93,12 +94,12 @@ public void testUnmappedKeyword() throws Exception { try (Engine.Searcher searcher = indexService.getShard(0).acquireSearcher("test")) { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, ValueType.STRING, "bytes", null, null, null, null); + context, ValueType.STRING, "bytes", null, null, null, null, null); ValuesSource.Bytes valuesSource = config.toValuesSource(context); assertNull(valuesSource); config = ValuesSourceConfig.resolve( - context, ValueType.STRING, "bytes", null, "abc", null, null); + context, ValueType.STRING, "bytes", null, "abc", null, null, null); valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); @@ -120,7 +121,7 @@ public void testLong() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "long", null, null, null, null); + context, null, "long", null, null, null, null, null); ValuesSource.Numeric valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); @@ -142,14 +143,14 @@ public void testEmptyLong() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "long", null, null, null, null); + context, null, "long", null, null, null, null, null); ValuesSource.Numeric valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertFalse(values.advanceExact(0)); config = ValuesSourceConfig.resolve( - context, null, "long", null, 42, null, null); + context, null, "long", null, 42, null, null, null); valuesSource = config.toValuesSource(context); values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -169,12 +170,12 @@ public void testUnmappedLong() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, ValueType.NUMBER, "long", null, null, null, null); + context, ValueType.NUMBER, "long", null, null, null, null, null); ValuesSource.Numeric valuesSource = config.toValuesSource(context); assertNull(valuesSource); config = ValuesSourceConfig.resolve( - context, ValueType.NUMBER, "long", null, 42, null, null); + context, ValueType.NUMBER, "long", null, 42, null, null, null); valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); @@ -196,7 +197,7 @@ public void testBoolean() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bool", null, null, null, null); + context, null, "bool", null, null, null, null, null); ValuesSource.Numeric valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); @@ -218,14 +219,14 @@ public void testEmptyBoolean() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bool", null, null, null, null); + context, null, "bool", null, null, null, null, null); ValuesSource.Numeric valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertFalse(values.advanceExact(0)); config = ValuesSourceConfig.resolve( - context, null, "bool", null, true, null, null); + context, null, "bool", null, true, null, null, null); valuesSource = config.toValuesSource(context); values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -245,12 +246,12 @@ public void testUnmappedBoolean() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, ValueType.BOOLEAN, "bool", null, null, null, null); + context, ValueType.BOOLEAN, "bool", null, null, null, null, null); ValuesSource.Numeric valuesSource = config.toValuesSource(context); assertNull(valuesSource); config = ValuesSourceConfig.resolve( - context, ValueType.BOOLEAN, "bool", null, true, null, null); + context, ValueType.BOOLEAN, "bool", null, true, null, null, null); valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); @@ -266,7 +267,7 @@ public void testTypeFieldDeprecation() { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, TypeFieldMapper.NAME, null, null, null, null); + context, null, TypeFieldMapper.NAME, null, null, null, null, null); assertWarnings(QueryShardContext.TYPES_DEPRECATION_MESSAGE); } } @@ -282,7 +283,7 @@ public void testFieldAlias() throws Exception { try (Engine.Searcher searcher = indexService.getShard(0).acquireSearcher("test")) { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, ValueType.STRING, "alias", null, null, null, null); + context, ValueType.STRING, "alias", null, null, null, null, null); ValuesSource.Bytes valuesSource = config.toValuesSource(context); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); From 6f90369363865ba99c6d6080f58625c5962486ce Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Fri, 1 Nov 2019 16:22:09 -0400 Subject: [PATCH 06/13] Added a bunch of notes for future me --- .../aggregations/support/ValuesSourceConfig.java | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index c3d6e4f94a9f0..f1d0822b488c3 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -73,10 +73,17 @@ public static ValuesSourceConfig resolve( if (field == null) { if (script == null) { + // TODO: The ValuesSourceConfig this constructs is invalid. We should just throw here. ValuesSourceConfig config = new ValuesSourceConfig<>(ValuesSourceType.ANY); config.format(resolveFormat(null, valueType, timeZone)); return config; } + /* + * This is the Stand Alone Script path. We should have a script that will produce a value independent of the presence or + * absence of any one field. The type of the script is given by the valueType field, and defaults to bytes if not specified. + */ + // TODO: Not pluggable, should always be valueType if specified, BYTES if not. + // TODO: Probably should validate that the resulting type is valid for this agg. That needs to be plugable. ValuesSourceType valuesSourceType = valueType != null ? valueType.getValuesSourceType() : ValuesSourceType.ANY; if (valuesSourceType == ValuesSourceType.ANY) { // the specific value source type is undefined, but for scripts, @@ -96,6 +103,11 @@ public static ValuesSourceConfig resolve( MappedFieldType fieldType = context.fieldMapper(field); if (fieldType == null) { + // TODO: This should be pluggable too; Effectively that will replace the missingAny() case from toValuesSource() + /* We got here because the user specified a field, but it doesn't exist on this index, possibly because of a wildcard index + * pattern. In this case, we're going to end up using the EMPTY variant of the ValuesSource, and possibly applying a user + * specified missing value. + */ ValuesSourceType valuesSourceType = valueType != null ? valueType.getValuesSourceType() : ValuesSourceType.ANY; ValuesSourceConfig config = new ValuesSourceConfig<>(valuesSourceType); config.missing(missing); From dccd0aab385cfaf93c1d12aa3bbec259079650e1 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 5 Nov 2019 08:09:24 -0500 Subject: [PATCH 07/13] Javadoc for register method --- .../aggregations/support/ValuesSourceRegistry.java | 11 +++++++++++ 1 file changed, 11 insertions(+) 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 3364477f38e80..dabf96b836c0c 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 @@ -111,8 +111,19 @@ public ValuesSourceType getValuesSourceType(MappedFieldType fieldType, IndexFiel } }; + /** + * Register a ValuesSource to Aggregator mapping. + * + * @param aggregationName The name of the family of aggregations, typically found via ValuesSourceAggregationBuilder.getType() + * @param valuesSourceType The ValuesSourceType this mapping applies to. + * @param aggregatorSupplier An Aggregation-specific specialization of AggregatorSupplier which will construct the mapped aggregator + * from the aggregation standard set of parameters + * @param resolveValuesSourceType A predicate operating on MappedFieldType and IndexFieldData instances which decides if the mapped + * ValuesSourceType can be applied to the given field. + */ public abstract void register(String aggregationName, ValuesSourceType valuesSourceType, AggregatorSupplier aggregatorSupplier, BiFunction resolveValuesSourceType); + public abstract AggregatorSupplier getAggregator(ValuesSourceType valuesSourceType, String aggregationName); // TODO: ValueType argument is only needed for legacy logic public abstract ValuesSourceType getValuesSourceType(MappedFieldType fieldType, IndexFieldData indexFieldData, String aggregationName, From 7bb97544e9c0e1425636f42054b895555824a88f Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 5 Nov 2019 08:10:32 -0500 Subject: [PATCH 08/13] intermediate state cleanup for resolve --- .../support/ValuesSourceConfig.java | 71 +++++++++---------- 1 file changed, 35 insertions(+), 36 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java index f1d0822b488c3..11e2e9cfec0d5 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceConfig.java @@ -70,13 +70,13 @@ public static ValuesSourceConfig resolve( String format, Function resolveScriptAny, String aggregationName) { + ValuesSourceConfig config; if (field == null) { + // Stand Alone Script Case if (script == null) { - // TODO: The ValuesSourceConfig this constructs is invalid. We should just throw here. - ValuesSourceConfig config = new ValuesSourceConfig<>(ValuesSourceType.ANY); - config.format(resolveFormat(null, valueType, timeZone)); - return config; + throw new IllegalStateException( + "value source config is invalid; must have either a field context or a script or marked as unwrapped"); } /* * This is the Stand Alone Script path. We should have a script that will produce a value independent of the presence or @@ -92,47 +92,45 @@ public static ValuesSourceConfig resolve( // on Bytes valuesSourceType = resolveScriptAny.apply(script); } - ValuesSourceConfig config = new ValuesSourceConfig<>(valuesSourceType); - config.missing(missing); - config.timezone(timeZone); + config = new ValuesSourceConfig<>(valuesSourceType); config.format(resolveFormat(format, valueType, timeZone)); config.script(createScript(script, context)); config.scriptValueType(valueType); - return config; - } + } else { + // Field case + MappedFieldType fieldType = context.fieldMapper(field); + if (fieldType == null) { + /* Unmapped Field Case + * We got here because the user specified a field, but it doesn't exist on this index, possibly because of a wildcard index + * pattern. In this case, we're going to end up using the EMPTY variant of the ValuesSource, and possibly applying a user + * specified missing value. + */ + // TODO: This should be pluggable too; Effectively that will replace the missingAny() case from toValuesSource() + ValuesSourceType valuesSourceType = valueType != null ? valueType.getValuesSourceType() : ValuesSourceType.ANY; + config = new ValuesSourceConfig<>(valuesSourceType); + config.format(resolveFormat(format, valueType, timeZone)); + config.unmapped(true); + if (valueType != null) { + // todo do we really need this for unmapped? + config.scriptValueType(valueType); + } + } else { - MappedFieldType fieldType = context.fieldMapper(field); - if (fieldType == null) { - // TODO: This should be pluggable too; Effectively that will replace the missingAny() case from toValuesSource() - /* We got here because the user specified a field, but it doesn't exist on this index, possibly because of a wildcard index - * pattern. In this case, we're going to end up using the EMPTY variant of the ValuesSource, and possibly applying a user - * specified missing value. - */ - ValuesSourceType valuesSourceType = valueType != null ? valueType.getValuesSourceType() : ValuesSourceType.ANY; - ValuesSourceConfig config = new ValuesSourceConfig<>(valuesSourceType); - config.missing(missing); - config.timezone(timeZone); - config.format(resolveFormat(format, valueType, timeZone)); - config.unmapped(true); - if (valueType != null) { - // todo do we really need this for unmapped? - config.scriptValueType(valueType); - } - return config; - } + IndexFieldData indexFieldData = context.getForField(fieldType); - IndexFieldData indexFieldData = context.getForField(fieldType); - ValuesSourceConfig config; - ValuesSourceType valuesSourceType = ValuesSourceRegistry.getInstance().getValuesSourceType(fieldType, indexFieldData, - aggregationName, valueType); - config = new ValuesSourceConfig<>(valuesSourceType); + ValuesSourceType valuesSourceType = ValuesSourceRegistry.getInstance().getValuesSourceType(fieldType, indexFieldData, + aggregationName, valueType); - config.fieldContext(new FieldContext(field, indexFieldData, fieldType)); + config = new ValuesSourceConfig<>(valuesSourceType); + + config.fieldContext(new FieldContext(field, indexFieldData, fieldType)); + config.script(createScript(script, context)); + config.format(fieldType.docValueFormat(format, timeZone)); + } + } config.missing(missing); config.timezone(timeZone); - config.script(createScript(script, context)); - config.format(fieldType.docValueFormat(format, timeZone)); return config; } @@ -254,6 +252,7 @@ public VS toValuesSource(QueryShardContext context) { @Nullable public VS toValuesSource(QueryShardContext context, Function resolveMissingAny) { if (!valid()) { + // TODO: resolve no longer generates invalid configs. Once VSConfig is immutable, we can drop this check throw new IllegalStateException( "value source config is invalid; must have either a field context or a script or marked as unwrapped"); } From 59b0c42505e5ce956c824e4340110ba02a56afe2 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 14 Nov 2019 16:11:01 -0500 Subject: [PATCH 09/13] test numeric histogram over dates. --- .../NumericHistogramAggregatorTests.java | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java index e3d1b931c71d5..44153e6bc9ee7 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/histogram/NumericHistogramAggregatorTests.java @@ -29,11 +29,17 @@ import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; import org.apache.lucene.util.NumericUtils; +import org.elasticsearch.common.time.DateFormatters; +import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper; + +import java.util.Arrays; +import java.util.List; + import static org.hamcrest.Matchers.containsString; public class NumericHistogramAggregatorTests extends AggregatorTestCase { @@ -100,6 +106,41 @@ public void testDoubles() throws Exception { } } + public void testDates() throws Exception { + List dataset = Arrays.asList( + "2019-11-01T01:07:45", + "2019-11-02T03:43:34", + "2019-11-03T04:11:00", + "2019-11-04T05:11:31", + "2019-11-05T08:24:05", + "2019-11-06T13:09:32", + "2019-11-07T13:47:43", + "2019-11-08T16:14:34", + "2019-11-09T17:09:50", + "2019-11-10T22:55:46"); + + try (Directory dir = newDirectory(); + RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { + for (String value : dataset) { + Document doc = new Document(); + long millis = DateFormatters.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse(value)).toInstant().toEpochMilli(); + doc.add(new SortedNumericDocValuesField("field", millis)); + w.addDocument(doc); + } + + HistogramAggregationBuilder aggBuilder = new HistogramAggregationBuilder("my_agg") + .field("field") + .interval(1000 * 60 * 60 * 24); + MappedFieldType fieldType = new NumberFieldMapper.NumberFieldType(NumberFieldMapper.NumberType.DOUBLE); + fieldType.setName("field"); + try (IndexReader reader = w.getReader()) { + IndexSearcher searcher = new IndexSearcher(reader); + InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); + assertTrue(AggregationInspectionHelper.hasValue(histogram)); + } + } + } + public void testIrrationalInterval() throws Exception { try (Directory dir = newDirectory(); RandomIndexWriter w = new RandomIndexWriter(random(), dir)) { From d2c8fa913aec4661e29c6908d8c8c5d6aa9046d3 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 14 Nov 2019 17:14:44 -0500 Subject: [PATCH 10/13] Fix seemantic merge conflict --- .../analytics/stringstats/StringStatsAggregationBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/stringstats/StringStatsAggregationBuilder.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/stringstats/StringStatsAggregationBuilder.java index 59097a69e16df..0626e84e5a9ee 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/stringstats/StringStatsAggregationBuilder.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/stringstats/StringStatsAggregationBuilder.java @@ -46,7 +46,7 @@ public static StringStatsAggregationBuilder parse(String aggregationName, XConte } public StringStatsAggregationBuilder(String name) { - super(name, ValuesSourceType.BYTES, ValueType.STRING); + super(name, ValueType.STRING); } public StringStatsAggregationBuilder(StringStatsAggregationBuilder clone, From f5a412d51c265ed06364172a6ef0ade446afd2e6 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 25 Nov 2019 12:51:19 -0500 Subject: [PATCH 11/13] no idea how this broke, but easy fix --- .../join/aggregations/ChildrenAggregationBuilder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregationBuilder.java b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregationBuilder.java index d8efaab9b0ebb..c15d8571ec9e4 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregationBuilder.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ChildrenAggregationBuilder.java @@ -62,7 +62,7 @@ public class ChildrenAggregationBuilder */ public ChildrenAggregationBuilder(String name, String childType) { super(name, ValueType.STRING); - f (childType == null) { + if (childType == null) { throw new IllegalArgumentException("[childType] must not be null: [" + name + "]"); } this.childType = childType; From 8f90085013830343f33cd7c67327610fcd3c8e59 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 25 Nov 2019 15:22:04 -0500 Subject: [PATCH 12/13] Fix failing test This is slightly non-obvious. Basically, we moved the check for not having a data source forward in the parsing, so then the aggregation in this test was throwing the wrong exception. So we needed to specify a field now, so it can get far enough to fail with the exception it expects. --- .../elasticsearch/search/aggregations/bucket/GeoDistanceIT.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java index c4cad9097e380..0d391ba05a5d4 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/GeoDistanceIT.java @@ -444,7 +444,7 @@ public void testEmptyAggregation() throws Exception { public void testNoRangesInQuery() { try { client().prepareSearch("idx") - .addAggregation(geoDistance("geo_dist", new GeoPoint(52.3760, 4.894))) + .addAggregation(geoDistance("geo_dist", new GeoPoint(52.3760, 4.894)).field("location")) .get(); fail(); } catch (SearchPhaseExecutionException spee){ From 2fec74109a69d7bdddb1d5082790a607a567a519 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 25 Nov 2019 15:42:59 -0500 Subject: [PATCH 13/13] fix checkstyle --- .../transform/transforms/MockDeprecatedAggregationBuilder.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/MockDeprecatedAggregationBuilder.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/MockDeprecatedAggregationBuilder.java index 02f9f905fcda5..a043c88b4f063 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/MockDeprecatedAggregationBuilder.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/transform/transforms/MockDeprecatedAggregationBuilder.java @@ -16,7 +16,6 @@ import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.AggregatorFactories.Builder; import org.elasticsearch.search.aggregations.AggregatorFactory; -import org.elasticsearch.search.aggregations.support.CoreValuesSourceType; import org.elasticsearch.search.aggregations.support.ValueType; import org.elasticsearch.search.aggregations.support.ValuesSource; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;