From 60d2be74df4172ed8182e193373209147da8ee48 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 19 Feb 2020 15:35:58 -0500 Subject: [PATCH 01/10] Narrow scope and requirements --- .../ArrayValuesSourceAggregatorFactory.java | 2 +- .../CompositeValuesSourceBuilder.java | 3 +- .../DateHistogramValuesSourceBuilder.java | 2 +- .../GeoTileGridValuesSourceBuilder.java | 2 +- .../HistogramValuesSourceBuilder.java | 2 +- .../composite/TermsValuesSourceBuilder.java | 2 +- .../support/MultiValuesSource.java | 2 +- .../ValuesSourceAggregatorFactory.java | 2 +- .../support/ValuesSourceConfig.java | 23 ++----- .../support/ValuesSourceConfigTests.java | 66 +++++++++---------- .../TopMetricsAggregatorFactory.java | 2 +- 11 files changed, 48 insertions(+), 60 deletions(-) diff --git a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java index 1186aff415c63..e60a36b1a84fd 100644 --- a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java +++ b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java @@ -53,7 +53,7 @@ public Aggregator createInternal(SearchContext searchContext, HashMap valuesSources = new HashMap<>(); for (Map.Entry config : configs.entrySet()) { - ValuesSource vs = config.getValue().toValuesSource(queryShardContext); + ValuesSource vs = config.getValue().toValuesSource(queryShardContext::nowInMillis); if (vs != null) { valuesSources.put(config.getKey(), vs); } 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 9ff057a35d232..7df525fb737d9 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 @@ -26,6 +26,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.script.Script; +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.ValuesSourceConfig; @@ -277,7 +278,7 @@ protected abstract CompositeValuesSourceConfig innerBuild(QueryShardContext quer public final CompositeValuesSourceConfig build(QueryShardContext queryShardContext) throws IOException { ValuesSourceConfig config = ValuesSourceConfig.resolve(queryShardContext, - valueType, field, script, null, timeZone(), format, name()); + valueType, field, script, null, timeZone(), format, CoreValuesSourceType.BYTES, name()); return innerBuild(queryShardContext, config); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java index 33e5db10c4297..a8de521b9acae 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java @@ -252,7 +252,7 @@ public DateHistogramValuesSourceBuilder offset(long offset) { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { Rounding rounding = dateHistogramInterval.createRounding(timeZone(), offset); - ValuesSource orig = config.toValuesSource(queryShardContext); + ValuesSource orig = config.toValuesSource(queryShardContext::nowInMillis); if (orig == null) { orig = ValuesSource.Numeric.EMPTY; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java index 4911cccabc05c..d47b08163bd49 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java @@ -129,7 +129,7 @@ public boolean equals(Object obj) { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { - ValuesSource orig = config.toValuesSource(queryShardContext); + ValuesSource orig = config.toValuesSource(queryShardContext::nowInMillis); if (orig == null) { orig = ValuesSource.GeoPoint.EMPTY; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java index f88287daefe99..95e8021db6d08 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java @@ -111,7 +111,7 @@ public HistogramValuesSourceBuilder interval(double interval) { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { - ValuesSource orig = config.toValuesSource(queryShardContext); + ValuesSource orig = config.toValuesSource(queryShardContext::nowInMillis); if (orig == null) { orig = ValuesSource.Numeric.EMPTY; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java index 880fa360ea921..b8c00acf619eb 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java @@ -71,7 +71,7 @@ public String type() { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { - ValuesSource vs = config.toValuesSource(queryShardContext); + ValuesSource vs = config.toValuesSource(queryShardContext::nowInMillis); if (vs == null) { // The field is unmapped so we use a value source that can parse any type of values. // This is needed because the after values are parsed even when there are no values to process. diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java index 4efe04f18174a..a059710a9cd6a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java @@ -39,7 +39,7 @@ public NumericMultiValuesSource(Map valuesSourceConf QueryShardContext context) { values = new HashMap<>(valuesSourceConfigs.size()); for (Map.Entry entry : valuesSourceConfigs.entrySet()) { - final ValuesSource valuesSource = entry.getValue().toValuesSource(context); + final ValuesSource valuesSource = entry.getValue().toValuesSource(context::nowInMillis); if (valuesSource instanceof ValuesSource.Numeric == false) { throw new AggregationExecutionException("ValuesSource type " + valuesSource.toString() + "is not supported for multi-valued aggregation"); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java index 52954afdaac9c..b12ceb694f9b7 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java @@ -44,7 +44,7 @@ public ValuesSourceAggregatorFactory(String name, ValuesSourceConfig config, Que @Override public Aggregator createInternal(SearchContext searchContext, Aggregator parent, boolean collectsFromSingleBucket, List pipelineAggregators, Map metaData) throws IOException { - ValuesSource vs = config.toValuesSource(queryShardContext); + ValuesSource vs = config.toValuesSource(queryShardContext::nowInMillis); if (vs == null) { return createUnmapped(searchContext, parent, pipelineAggregators, metaData); } 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 bf02d13c14f39..f258a62d673d6 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 @@ -27,6 +27,7 @@ import org.elasticsearch.search.DocValueFormat; import java.time.ZoneId; +import java.util.function.LongSupplier; /** * A configuration that tells aggregations how to retrieve data from the index @@ -34,19 +35,6 @@ */ public class ValuesSourceConfig { - /** - * Resolve a {@link ValuesSourceConfig} given configuration parameters. - */ - public static ValuesSourceConfig resolve( - QueryShardContext context, - ValueType valueType, - String field, Script script, - Object missing, - ZoneId timeZone, - String format, String aggregationName) { - return resolve(context, valueType, field, script, missing, timeZone, format, CoreValuesSourceType.BYTES, aggregationName); - } - /** * Given the query context and other information, decide on the input {@link ValuesSource} for this aggretation run, and construct a new * {@link ValuesSourceConfig} based on that {@link ValuesSourceType} @@ -190,7 +178,7 @@ public ValuesSourceConfig fieldContext(FieldContext fieldContext) { return this; } - public ValuesSourceConfig script(AggregationScript.LeafFactory script) { + private ValuesSourceConfig script(AggregationScript.LeafFactory script) { this.script = script; return this; } @@ -238,12 +226,11 @@ public DocValueFormat format() { /** * Transform the {@link ValuesSourceType} we selected in resolve into the specific {@link ValuesSource} instance to use for this shard - * @param context - Literally just used to get the current time + * @param now - Should provide the current time in milliseconds. Used for parsing some missing values. * @return - A {@link ValuesSource} ready to be read from by an aggregator */ @Nullable - // TODO: Replace QueryShardContext with a LongProvider - public ValuesSource toValuesSource(QueryShardContext context) { + public ValuesSource toValuesSource(LongSupplier now) { if (!valid()) { // TODO: resolve no longer generates invalid configs. Once VSConfig is immutable, we can drop this check throw new IllegalStateException( @@ -271,6 +258,6 @@ public ValuesSource toValuesSource(QueryShardContext context) { if (missing() == null) { return vs; } - return valueSourceType().replaceMissing(vs, missing, format, context::nowInMillis); + return valueSourceType().replaceMissing(vs, missing, format, now); } } 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 43c5d76fe541a..f10e5e7065f99 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 @@ -46,8 +46,8 @@ public void testKeyword() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bytes", null, null, null, null, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context); + context, null, "bytes", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); assertTrue(values.advanceExact(0)); @@ -68,15 +68,15 @@ public void testEmptyKeyword() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bytes", null, null, null, null, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context); + context, null, "bytes", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); 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, null); - valuesSource = (ValuesSource.Bytes) config.toValuesSource(context); + context, null, "bytes", null, "abc", null, null, CoreValuesSourceType.BYTES, null); + valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); values = valuesSource.bytesValues(ctx); assertTrue(values.advanceExact(0)); assertEquals(1, values.docValueCount()); @@ -94,13 +94,13 @@ 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, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context); + context, ValueType.STRING, "bytes", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); assertNull(valuesSource); config = ValuesSourceConfig.resolve( - context, ValueType.STRING, "bytes", null, "abc", null, null, null); - valuesSource = (ValuesSource.Bytes) config.toValuesSource(context); + context, ValueType.STRING, "bytes", null, "abc", null, null, CoreValuesSourceType.BYTES, null); + valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); assertTrue(values.advanceExact(0)); @@ -121,8 +121,8 @@ public void testLong() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "long", null, null, null, null, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, null, "long", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -143,15 +143,15 @@ public void testEmptyLong() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "long", null, null, null, null, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, null, "long", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); 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, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, null, "long", null, 42, null, null, CoreValuesSourceType.BYTES, null); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); assertEquals(1, values.docValueCount()); @@ -170,13 +170,13 @@ 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, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, ValueType.NUMBER, "long", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); assertNull(valuesSource); config = ValuesSourceConfig.resolve( - context, ValueType.NUMBER, "long", null, 42, null, null, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, ValueType.NUMBER, "long", null, 42, null, null, CoreValuesSourceType.BYTES, null); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -197,8 +197,8 @@ public void testBoolean() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bool", null, null, null, null, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, null, "bool", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -219,15 +219,15 @@ public void testEmptyBoolean() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( - context, null, "bool", null, null, null, null, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, null, "bool", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); 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, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, null, "bool", null, true, null, null, CoreValuesSourceType.BYTES, null); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); assertEquals(1, values.docValueCount()); @@ -246,13 +246,13 @@ 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, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, ValueType.BOOLEAN, "bool", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); assertNull(valuesSource); config = ValuesSourceConfig.resolve( - context, ValueType.BOOLEAN, "bool", null, true, null, null, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context); + context, ValueType.BOOLEAN, "bool", null, true, null, null, CoreValuesSourceType.BYTES, null); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -267,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, null); + context, null, TypeFieldMapper.NAME, null, null, null, null, CoreValuesSourceType.BYTES, null); assertWarnings(QueryShardContext.TYPES_DEPRECATION_MESSAGE); } } @@ -283,8 +283,8 @@ 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, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context); + context, ValueType.STRING, "alias", null, null, null, null, CoreValuesSourceType.BYTES, null); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java index e257ef1357eff..363d5aafefa15 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java @@ -42,7 +42,7 @@ protected TopMetricsAggregator createInternal(SearchContext searchContext, Aggre ValuesSourceConfig metricFieldSource = ValuesSourceConfig.resolve(queryShardContext, ValueType.NUMERIC, metricField.getFieldName(), metricField.getScript(), metricField.getMissing(), metricField.getTimeZone(), null, CoreValuesSourceType.NUMERIC, TopMetricsAggregationBuilder.NAME); - ValuesSource.Numeric metricValueSource = (ValuesSource.Numeric) metricFieldSource.toValuesSource(queryShardContext); + ValuesSource.Numeric metricValueSource = (ValuesSource.Numeric) metricFieldSource.toValuesSource(queryShardContext::nowInMillis); if (metricValueSource == null) { return createUnmapped(searchContext, parent, pipelineAggregators, metaData); } From 04475e26efa0a3c58c8dd2415ff9584992c8fc3e Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 20 Feb 2020 10:46:43 -0500 Subject: [PATCH 02/10] indentation --- .../support/ValuesSourceConfig.java | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 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 f258a62d673d6..25d515ab9e03a 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 @@ -52,15 +52,15 @@ public class ValuesSourceConfig { * {@link ValuesSourceRegistry} * @return - An initialized {@link ValuesSourceConfig} that will yield the appropriate {@link ValuesSourceType} */ - public static ValuesSourceConfig resolve( - QueryShardContext context, - ValueType userValueTypeHint, - String field, Script script, - Object missing, - ZoneId timeZone, - String format, - ValuesSourceType defaultValueSourceType, - String aggregationName) { + public static ValuesSourceConfig resolve(QueryShardContext context, + ValueType userValueTypeHint, + String field, + Script script, + Object missing, + ZoneId timeZone, + String format, + ValuesSourceType defaultValueSourceType, + String aggregationName) { ValuesSourceConfig config; MappedFieldType fieldType = null; ValuesSourceType valuesSourceType; From 3aadb4b84e6cfb463fffcc90466911ea653283c2 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 20 Feb 2020 11:22:35 -0500 Subject: [PATCH 03/10] fix typo --- .../search/aggregations/support/ValuesSourceConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 25d515ab9e03a..31d64cf2fdfca 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 @@ -68,7 +68,7 @@ public static ValuesSourceConfig resolve(QueryShardContext context, // Stand Alone Script Case if (script == null) { throw new IllegalStateException( - "value source config is invalid; must have either a field context or a script or marked as unwrapped"); + "value source config is invalid; must have either a field context or a script or marked as unmapped"); } /* * This is the Stand Alone Script path. We should have a script that will produce a value independent of the presence or From 052a2d0c59bd2a8dd3331429088b74581e8c19fa Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 20 Feb 2020 16:53:18 -0500 Subject: [PATCH 04/10] Don't mutate ValuesSourceConfig in Parent & Child aggregations --- .../ChildrenAggregationBuilder.java | 16 ++----- .../ParentAggregationBuilder.java | 16 ++----- .../support/ValuesSourceConfig.java | 48 +++++++++++++++---- .../CumulativeCardinalityAggregatorTests.java | 4 +- 4 files changed, 51 insertions(+), 33 deletions(-) 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 5d6c7d6dd1187..15a991335f563 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 @@ -25,7 +25,6 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.index.fielddata.plain.SortedSetDVOrdinalsIndexFieldData; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.join.mapper.ParentIdFieldMapper; @@ -34,7 +33,6 @@ 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.FieldContext; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; @@ -109,23 +107,19 @@ protected ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardC @Override protected ValuesSourceConfig resolveConfig(QueryShardContext queryShardContext) { - ValuesSourceConfig config = new ValuesSourceConfig(CoreValuesSourceType.BYTES); - joinFieldResolveConfig(queryShardContext, config); - return config; - } - - private void joinFieldResolveConfig(QueryShardContext queryShardContext, ValuesSourceConfig config) { + ValuesSourceConfig config; ParentJoinFieldMapper parentJoinFieldMapper = ParentJoinFieldMapper.getMapper(queryShardContext.getMapperService()); ParentIdFieldMapper parentIdFieldMapper = parentJoinFieldMapper.getParentIdFieldMapper(childType, false); if (parentIdFieldMapper != null) { parentFilter = parentIdFieldMapper.getParentFilter(); childFilter = parentIdFieldMapper.getChildFilter(childType); MappedFieldType fieldType = parentIdFieldMapper.fieldType(); - final SortedSetDVOrdinalsIndexFieldData fieldData = queryShardContext.getForField(fieldType); - config.fieldContext(new FieldContext(fieldType.name(), fieldData, fieldType)); + config = new ValuesSourceConfig(fieldType.getValuesSourceType(), fieldType, queryShardContext); } else { - config.unmapped(true); + // Unmapped field case + config = new ValuesSourceConfig(defaultValueSourceType(), queryShardContext); } + return config; } @Override diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java index 5b7116d551d45..90b64ccda7280 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java @@ -25,7 +25,6 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; -import org.elasticsearch.index.fielddata.plain.SortedSetDVOrdinalsIndexFieldData; import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.join.mapper.ParentIdFieldMapper; @@ -34,7 +33,6 @@ 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.FieldContext; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder; import org.elasticsearch.search.aggregations.support.ValuesSourceAggregatorFactory; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; @@ -109,23 +107,19 @@ protected ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardC @Override protected ValuesSourceConfig resolveConfig(QueryShardContext queryShardContext) { - ValuesSourceConfig config = new ValuesSourceConfig(CoreValuesSourceType.BYTES); - joinFieldResolveConfig(queryShardContext, config); - return config; - } - - private void joinFieldResolveConfig(QueryShardContext queryShardContext, ValuesSourceConfig config) { + ValuesSourceConfig config; // = new ValuesSourceConfig(CoreValuesSourceType.BYTES, null, null); ParentJoinFieldMapper parentJoinFieldMapper = ParentJoinFieldMapper.getMapper(queryShardContext.getMapperService()); ParentIdFieldMapper parentIdFieldMapper = parentJoinFieldMapper.getParentIdFieldMapper(childType, false); if (parentIdFieldMapper != null) { parentFilter = parentIdFieldMapper.getParentFilter(); childFilter = parentIdFieldMapper.getChildFilter(childType); MappedFieldType fieldType = parentIdFieldMapper.fieldType(); - final SortedSetDVOrdinalsIndexFieldData fieldData = queryShardContext.getForField(fieldType); - config.fieldContext(new FieldContext(fieldType.name(), fieldData, fieldType)); + config = new ValuesSourceConfig(fieldType.getValuesSourceType(), fieldType, queryShardContext); } else { - config.unmapped(true); + // unmapped case + config = new ValuesSourceConfig(defaultValueSourceType(), queryShardContext); } + return config; } @Override 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 31d64cf2fdfca..f8b4e8ae9af13 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 @@ -80,7 +80,7 @@ public static ValuesSourceConfig resolve(QueryShardContext context, } else { valuesSourceType = defaultValueSourceType; } - config = new ValuesSourceConfig(valuesSourceType); + config = new ValuesSourceConfig(valuesSourceType, null, false, null); config.script(createScript(script, context)); config.scriptValueType(userValueTypeHint); } else { @@ -98,7 +98,7 @@ public static ValuesSourceConfig resolve(QueryShardContext context, } else { valuesSourceType = defaultValueSourceType; } - config = new ValuesSourceConfig(valuesSourceType); + config = new ValuesSourceConfig(valuesSourceType, null, false, null); // TODO: PLAN - get rid of the unmapped flag field; it's only used by valid(), and we're intending to get rid of that. // TODO: Once we no longer care about unmapped, we can merge this case with the mapped case. config.unmapped(true); @@ -110,7 +110,7 @@ public static ValuesSourceConfig resolve(QueryShardContext context, IndexFieldData indexFieldData = context.getForField(fieldType); valuesSourceType = context.getValuesSourceRegistry().getValuesSourceType(fieldType, aggregationName, indexFieldData, userValueTypeHint, script, defaultValueSourceType); - config = new ValuesSourceConfig(valuesSourceType); + config = new ValuesSourceConfig(valuesSourceType, null, false, null); config.fieldContext(new FieldContext(field, indexFieldData, fieldType)); config.script(createScript(script, context)); @@ -140,21 +140,49 @@ private static DocValueFormat resolveFormat(@Nullable String format, @Nullable V return valuesSourceType.getFormatter(format, tz); } - private final ValuesSourceType valueSourceType; + private final ValuesSourceType valuesSourceType; private FieldContext fieldContext; private AggregationScript.LeafFactory script; private ValueType scriptValueType; - private boolean unmapped = false; + private boolean unmapped; private DocValueFormat format = DocValueFormat.RAW; private Object missing; private ZoneId timeZone; - public ValuesSourceConfig(ValuesSourceType valueSourceType) { - this.valueSourceType = valueSourceType; + + /** + * Config instances which only need to specify a field should use this constructor + */ + public ValuesSourceConfig(ValuesSourceType valuesSourceType, + MappedFieldType fieldType, + QueryShardContext queryShardContext) { + this(valuesSourceType, fieldType, false, queryShardContext); + } + + /** + * Convenience method for creating unmapped configs + */ + public ValuesSourceConfig(ValuesSourceType valuesSourceType, QueryShardContext queryShardContext) { + this(valuesSourceType, null, true, queryShardContext); + } + + public ValuesSourceConfig(ValuesSourceType valuesSourceType, + MappedFieldType fieldType, + boolean unmapped, + QueryShardContext queryShardContext) { + if (unmapped && fieldType != null) { + throw new IllegalStateException("value source config is invalid; marked as unmapped but specified a mapped field"); + } + this.valuesSourceType = valuesSourceType; + if (fieldType != null) { + this.fieldContext = new FieldContext(fieldType.name(), queryShardContext.getForField(fieldType), fieldType); + } + this.unmapped = unmapped; + } public ValuesSourceType valueSourceType() { - return valueSourceType; + return valuesSourceType; } public FieldContext fieldContext() { @@ -240,7 +268,9 @@ public ValuesSource toValuesSource(LongSupplier now) { final ValuesSource vs; if (unmapped()) { if (missing() == null) { - // otherwise we will have values because of the missing value + /* Null values source signals to the AggregationBuilder to use the createUnmapped method, which aggregator factories can + * override to provide an aggregator optimized to return empty values + */ vs = null; } else { vs = valueSourceType().getEmpty(); diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java index ada59511a2b29..8bebb3ff75164 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java @@ -117,7 +117,7 @@ public void testAllNull() throws IOException { } public void testParentValidations() throws IOException { - ValuesSourceConfig valuesSource = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC); + ValuesSourceConfig valuesSource = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, null, false, null); // Histogram Set aggBuilders = new HashSet<>(); @@ -140,7 +140,7 @@ public void testParentValidations() throws IOException { builder.validate(parent, Collections.emptySet(), aggBuilders); // Auto Date Histogram - ValuesSourceConfig numericVS = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC); + ValuesSourceConfig numericVS = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, null, false, null); aggBuilders.clear(); aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum")); AutoDateHistogramAggregationBuilder.RoundingInfo[] roundings = new AutoDateHistogramAggregationBuilder.RoundingInfo[1]; From 1a947edfb6e38b81c18e1af01ccb9787634f7cbf Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Thu, 20 Feb 2020 17:35:21 -0500 Subject: [PATCH 05/10] remove setters for several fields on VSConfig --- .../support/ValuesSourceConfig.java | 46 ++++++------------- .../CumulativeCardinalityAggregatorTests.java | 4 +- 2 files changed, 17 insertions(+), 33 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 f8b4e8ae9af13..2eff1be5a2d80 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 @@ -64,6 +64,9 @@ public static ValuesSourceConfig resolve(QueryShardContext context, ValuesSourceConfig config; MappedFieldType fieldType = null; ValuesSourceType valuesSourceType; + ValueType scriptValueType = null; + AggregationScript.LeafFactory aggregationScript = null; + boolean unmapped = false; if (field == null) { // Stand Alone Script Case if (script == null) { @@ -80,9 +83,8 @@ public static ValuesSourceConfig resolve(QueryShardContext context, } else { valuesSourceType = defaultValueSourceType; } - config = new ValuesSourceConfig(valuesSourceType, null, false, null); - config.script(createScript(script, context)); - config.scriptValueType(userValueTypeHint); + aggregationScript = createScript(script, context); + scriptValueType = userValueTypeHint; } else { // Field case fieldType = context.fieldMapper(field); @@ -98,24 +100,22 @@ public static ValuesSourceConfig resolve(QueryShardContext context, } else { valuesSourceType = defaultValueSourceType; } - config = new ValuesSourceConfig(valuesSourceType, null, false, null); // TODO: PLAN - get rid of the unmapped flag field; it's only used by valid(), and we're intending to get rid of that. // TODO: Once we no longer care about unmapped, we can merge this case with the mapped case. - config.unmapped(true); + unmapped = true; if (userValueTypeHint != null) { // todo do we really need this for unmapped? - config.scriptValueType(userValueTypeHint); + scriptValueType = userValueTypeHint; } } else { IndexFieldData indexFieldData = context.getForField(fieldType); valuesSourceType = context.getValuesSourceRegistry().getValuesSourceType(fieldType, aggregationName, indexFieldData, userValueTypeHint, script, defaultValueSourceType); - config = new ValuesSourceConfig(valuesSourceType, null, false, null); - config.fieldContext(new FieldContext(field, indexFieldData, fieldType)); - config.script(createScript(script, context)); + aggregationScript = createScript(script, context); } } + config = new ValuesSourceConfig(valuesSourceType, fieldType, unmapped, aggregationScript, scriptValueType , context); config.format(resolveFormat(format, valuesSourceType, timeZone, fieldType)); config.missing(missing); config.timezone(timeZone); @@ -156,19 +156,21 @@ private static DocValueFormat resolveFormat(@Nullable String format, @Nullable V public ValuesSourceConfig(ValuesSourceType valuesSourceType, MappedFieldType fieldType, QueryShardContext queryShardContext) { - this(valuesSourceType, fieldType, false, queryShardContext); + this(valuesSourceType, fieldType, false, null, null, queryShardContext); } /** * Convenience method for creating unmapped configs */ public ValuesSourceConfig(ValuesSourceType valuesSourceType, QueryShardContext queryShardContext) { - this(valuesSourceType, null, true, queryShardContext); + this(valuesSourceType, null, true, null, null, queryShardContext); } public ValuesSourceConfig(ValuesSourceType valuesSourceType, MappedFieldType fieldType, boolean unmapped, + AggregationScript.LeafFactory script, + ValueType scriptValueType, QueryShardContext queryShardContext) { if (unmapped && fieldType != null) { throw new IllegalStateException("value source config is invalid; marked as unmapped but specified a mapped field"); @@ -178,6 +180,8 @@ public ValuesSourceConfig(ValuesSourceType valuesSourceType, this.fieldContext = new FieldContext(fieldType.name(), queryShardContext.getForField(fieldType), fieldType); } this.unmapped = unmapped; + this.script = script; + this.scriptValueType = scriptValueType; } @@ -201,30 +205,10 @@ public boolean valid() { return fieldContext != null || script != null || unmapped; } - public ValuesSourceConfig fieldContext(FieldContext fieldContext) { - this.fieldContext = fieldContext; - return this; - } - - private ValuesSourceConfig script(AggregationScript.LeafFactory script) { - this.script = script; - return this; - } - - private ValuesSourceConfig scriptValueType(ValueType scriptValueType) { - this.scriptValueType = scriptValueType; - return this; - } - public ValueType scriptValueType() { return this.scriptValueType; } - public ValuesSourceConfig unmapped(boolean unmapped) { - this.unmapped = unmapped; - return this; - } - public ValuesSourceConfig format(final DocValueFormat format) { this.format = format; return this; diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java index 8bebb3ff75164..74a8f1de3dced 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java @@ -117,7 +117,7 @@ public void testAllNull() throws IOException { } public void testParentValidations() throws IOException { - ValuesSourceConfig valuesSource = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, null, false, null); + ValuesSourceConfig valuesSource = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, mock(QueryShardContext.class)); // Histogram Set aggBuilders = new HashSet<>(); @@ -140,7 +140,7 @@ public void testParentValidations() throws IOException { builder.validate(parent, Collections.emptySet(), aggBuilders); // Auto Date Histogram - ValuesSourceConfig numericVS = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, null, false, null); + ValuesSourceConfig numericVS = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, mock(QueryShardContext.class)); aggBuilders.clear(); aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum")); AutoDateHistogramAggregationBuilder.RoundingInfo[] roundings = new AutoDateHistogramAggregationBuilder.RoundingInfo[1]; From 646408090524b8f9e6d87f7a729b9455d356fee2 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 24 Feb 2020 15:17:51 -0500 Subject: [PATCH 06/10] Restrict access on remaining mutators --- .../search/aggregations/support/ValuesSourceConfig.java | 6 +++--- 1 file changed, 3 insertions(+), 3 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 2eff1be5a2d80..4e4ffc16e7fb4 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 @@ -209,12 +209,12 @@ public ValueType scriptValueType() { return this.scriptValueType; } - public ValuesSourceConfig format(final DocValueFormat format) { + private ValuesSourceConfig format(final DocValueFormat format) { this.format = format; return this; } - public ValuesSourceConfig missing(final Object missing) { + private ValuesSourceConfig missing(final Object missing) { this.missing = missing; return this; } @@ -223,7 +223,7 @@ public Object missing() { return this.missing; } - public ValuesSourceConfig timezone(final ZoneId timeZone) { + private ValuesSourceConfig timezone(final ZoneId timeZone) { this.timeZone = timeZone; return this; } From d36cfd3679f79a6906fe59713ccaeadfbe19793b Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 24 Feb 2020 15:24:55 -0500 Subject: [PATCH 07/10] remove some outdated TODO notes --- .../search/aggregations/support/ValuesSourceConfig.java | 3 --- 1 file changed, 3 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 4e4ffc16e7fb4..b1d4fa494f209 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 @@ -94,14 +94,11 @@ public static ValuesSourceConfig resolve(QueryShardContext context, * 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() if (userValueTypeHint != null) { valuesSourceType = userValueTypeHint.getValuesSourceType(); } else { valuesSourceType = defaultValueSourceType; } - // TODO: PLAN - get rid of the unmapped flag field; it's only used by valid(), and we're intending to get rid of that. - // TODO: Once we no longer care about unmapped, we can merge this case with the mapped case. unmapped = true; if (userValueTypeHint != null) { // todo do we really need this for unmapped? From 0ecc7d9f26bf8c64df7f4a38f9b44acf5bbf1960 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 24 Feb 2020 15:37:29 -0500 Subject: [PATCH 08/10] Remove redundant parameter to toValuesSource --- .../ArrayValuesSourceAggregatorFactory.java | 2 +- .../DateHistogramValuesSourceBuilder.java | 2 +- .../GeoTileGridValuesSourceBuilder.java | 2 +- .../HistogramValuesSourceBuilder.java | 2 +- .../composite/TermsValuesSourceBuilder.java | 2 +- .../support/MultiValuesSource.java | 2 +- .../ValuesSourceAggregatorFactory.java | 2 +- .../support/ValuesSourceConfig.java | 5 +-- .../support/ValuesSourceConfigTests.java | 32 +++++++++---------- .../TopMetricsAggregatorFactory.java | 2 +- 10 files changed, 27 insertions(+), 26 deletions(-) diff --git a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java index e60a36b1a84fd..d1124c9e693a5 100644 --- a/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java +++ b/modules/aggs-matrix-stats/src/main/java/org/elasticsearch/search/aggregations/support/ArrayValuesSourceAggregatorFactory.java @@ -53,7 +53,7 @@ public Aggregator createInternal(SearchContext searchContext, HashMap valuesSources = new HashMap<>(); for (Map.Entry config : configs.entrySet()) { - ValuesSource vs = config.getValue().toValuesSource(queryShardContext::nowInMillis); + ValuesSource vs = config.getValue().toValuesSource(); if (vs != null) { valuesSources.put(config.getKey(), vs); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java index a8de521b9acae..1caf22a23d36c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/DateHistogramValuesSourceBuilder.java @@ -252,7 +252,7 @@ public DateHistogramValuesSourceBuilder offset(long offset) { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { Rounding rounding = dateHistogramInterval.createRounding(timeZone(), offset); - ValuesSource orig = config.toValuesSource(queryShardContext::nowInMillis); + ValuesSource orig = config.toValuesSource(); if (orig == null) { orig = ValuesSource.Numeric.EMPTY; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java index d47b08163bd49..c74d62e623ebe 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/GeoTileGridValuesSourceBuilder.java @@ -129,7 +129,7 @@ public boolean equals(Object obj) { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { - ValuesSource orig = config.toValuesSource(queryShardContext::nowInMillis); + ValuesSource orig = config.toValuesSource(); if (orig == null) { orig = ValuesSource.GeoPoint.EMPTY; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java index 95e8021db6d08..4ef15908ddd7a 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/HistogramValuesSourceBuilder.java @@ -111,7 +111,7 @@ public HistogramValuesSourceBuilder interval(double interval) { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { - ValuesSource orig = config.toValuesSource(queryShardContext::nowInMillis); + ValuesSource orig = config.toValuesSource(); if (orig == null) { orig = ValuesSource.Numeric.EMPTY; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java index b8c00acf619eb..2954c155e0d7c 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/TermsValuesSourceBuilder.java @@ -71,7 +71,7 @@ public String type() { @Override protected CompositeValuesSourceConfig innerBuild(QueryShardContext queryShardContext, ValuesSourceConfig config) throws IOException { - ValuesSource vs = config.toValuesSource(queryShardContext::nowInMillis); + ValuesSource vs = config.toValuesSource(); if (vs == null) { // The field is unmapped so we use a value source that can parse any type of values. // This is needed because the after values are parsed even when there are no values to process. diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java index a059710a9cd6a..fe1d5be993209 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/MultiValuesSource.java @@ -39,7 +39,7 @@ public NumericMultiValuesSource(Map valuesSourceConf QueryShardContext context) { values = new HashMap<>(valuesSourceConfigs.size()); for (Map.Entry entry : valuesSourceConfigs.entrySet()) { - final ValuesSource valuesSource = entry.getValue().toValuesSource(context::nowInMillis); + final ValuesSource valuesSource = entry.getValue().toValuesSource(); if (valuesSource instanceof ValuesSource.Numeric == false) { throw new AggregationExecutionException("ValuesSource type " + valuesSource.toString() + "is not supported for multi-valued aggregation"); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java index b12ceb694f9b7..d9cfc453d6e98 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceAggregatorFactory.java @@ -44,7 +44,7 @@ public ValuesSourceAggregatorFactory(String name, ValuesSourceConfig config, Que @Override public Aggregator createInternal(SearchContext searchContext, Aggregator parent, boolean collectsFromSingleBucket, List pipelineAggregators, Map metaData) throws IOException { - ValuesSource vs = config.toValuesSource(queryShardContext::nowInMillis); + ValuesSource vs = config.toValuesSource(); if (vs == null) { return createUnmapped(searchContext, parent, pipelineAggregators, metaData); } 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 b1d4fa494f209..62af06e5df39b 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 @@ -145,6 +145,7 @@ private static DocValueFormat resolveFormat(@Nullable String format, @Nullable V private DocValueFormat format = DocValueFormat.RAW; private Object missing; private ZoneId timeZone; + private LongSupplier now; /** @@ -179,6 +180,7 @@ public ValuesSourceConfig(ValuesSourceType valuesSourceType, this.unmapped = unmapped; this.script = script; this.scriptValueType = scriptValueType; + this.now = queryShardContext::nowInMillis; } @@ -235,11 +237,10 @@ public DocValueFormat format() { /** * Transform the {@link ValuesSourceType} we selected in resolve into the specific {@link ValuesSource} instance to use for this shard - * @param now - Should provide the current time in milliseconds. Used for parsing some missing values. * @return - A {@link ValuesSource} ready to be read from by an aggregator */ @Nullable - public ValuesSource toValuesSource(LongSupplier now) { + public ValuesSource toValuesSource() { if (!valid()) { // TODO: resolve no longer generates invalid configs. Once VSConfig is immutable, we can drop this check throw new IllegalStateException( 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 f10e5e7065f99..25a71bd2ed39e 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 @@ -47,7 +47,7 @@ public void testKeyword() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, null, "bytes", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); assertTrue(values.advanceExact(0)); @@ -69,14 +69,14 @@ public void testEmptyKeyword() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, null, "bytes", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(); 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, CoreValuesSourceType.BYTES, null); - valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); + valuesSource = (ValuesSource.Bytes) config.toValuesSource(); values = valuesSource.bytesValues(ctx); assertTrue(values.advanceExact(0)); assertEquals(1, values.docValueCount()); @@ -95,12 +95,12 @@ public void testUnmappedKeyword() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( context, ValueType.STRING, "bytes", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(); assertNull(valuesSource); config = ValuesSourceConfig.resolve( context, ValueType.STRING, "bytes", null, "abc", null, null, CoreValuesSourceType.BYTES, null); - valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); + valuesSource = (ValuesSource.Bytes) config.toValuesSource(); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); assertTrue(values.advanceExact(0)); @@ -122,7 +122,7 @@ public void testLong() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, null, "long", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -144,14 +144,14 @@ public void testEmptyLong() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, null, "long", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(); 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, CoreValuesSourceType.BYTES, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(); values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); assertEquals(1, values.docValueCount()); @@ -171,12 +171,12 @@ public void testUnmappedLong() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, ValueType.NUMBER, "long", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(); assertNull(valuesSource); config = ValuesSourceConfig.resolve( context, ValueType.NUMBER, "long", null, 42, null, null, CoreValuesSourceType.BYTES, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -198,7 +198,7 @@ public void testBoolean() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, null, "bool", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -220,14 +220,14 @@ public void testEmptyBoolean() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, null, "bool", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(); 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, CoreValuesSourceType.BYTES, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(); values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); assertEquals(1, values.docValueCount()); @@ -247,12 +247,12 @@ public void testUnmappedBoolean() throws Exception { ValuesSourceConfig config = ValuesSourceConfig.resolve( context, ValueType.BOOLEAN, "bool", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + ValuesSource.Numeric valuesSource = (ValuesSource.Numeric) config.toValuesSource(); assertNull(valuesSource); config = ValuesSourceConfig.resolve( context, ValueType.BOOLEAN, "bool", null, true, null, null, CoreValuesSourceType.BYTES, null); - valuesSource = (ValuesSource.Numeric) config.toValuesSource(context::nowInMillis); + valuesSource = (ValuesSource.Numeric) config.toValuesSource(); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedNumericDocValues values = valuesSource.longValues(ctx); assertTrue(values.advanceExact(0)); @@ -284,7 +284,7 @@ public void testFieldAlias() throws Exception { QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); ValuesSourceConfig config = ValuesSourceConfig.resolve( context, ValueType.STRING, "alias", null, null, null, null, CoreValuesSourceType.BYTES, null); - ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(context::nowInMillis); + ValuesSource.Bytes valuesSource = (ValuesSource.Bytes) config.toValuesSource(); LeafReaderContext ctx = searcher.getIndexReader().leaves().get(0); SortedBinaryDocValues values = valuesSource.bytesValues(ctx); diff --git a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java index 363d5aafefa15..aa796bb4075f4 100644 --- a/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java +++ b/x-pack/plugin/analytics/src/main/java/org/elasticsearch/xpack/analytics/topmetrics/TopMetricsAggregatorFactory.java @@ -42,7 +42,7 @@ protected TopMetricsAggregator createInternal(SearchContext searchContext, Aggre ValuesSourceConfig metricFieldSource = ValuesSourceConfig.resolve(queryShardContext, ValueType.NUMERIC, metricField.getFieldName(), metricField.getScript(), metricField.getMissing(), metricField.getTimeZone(), null, CoreValuesSourceType.NUMERIC, TopMetricsAggregationBuilder.NAME); - ValuesSource.Numeric metricValueSource = (ValuesSource.Numeric) metricFieldSource.toValuesSource(queryShardContext::nowInMillis); + ValuesSource.Numeric metricValueSource = (ValuesSource.Numeric) metricFieldSource.toValuesSource(); if (metricValueSource == null) { return createUnmapped(searchContext, parent, pipelineAggregators, metaData); } From c293a8aa988f641e04183048cdb17f732973dfb5 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Mon, 24 Feb 2020 16:54:28 -0500 Subject: [PATCH 09/10] Response to PR feedback --- .../ChildrenAggregationBuilder.java | 4 +- .../ParentAggregationBuilder.java | 6 +-- .../support/ValuesSourceConfig.java | 39 ++++++++++--------- .../CumulativeCardinalityAggregatorTests.java | 4 +- 4 files changed, 27 insertions(+), 26 deletions(-) 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 15a991335f563..8eedf429fa5d7 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 @@ -114,10 +114,10 @@ protected ValuesSourceConfig resolveConfig(QueryShardContext queryShardContext) parentFilter = parentIdFieldMapper.getParentFilter(); childFilter = parentIdFieldMapper.getChildFilter(childType); MappedFieldType fieldType = parentIdFieldMapper.fieldType(); - config = new ValuesSourceConfig(fieldType.getValuesSourceType(), fieldType, queryShardContext); + config = ValuesSourceConfig.resolveFieldOnly(fieldType, queryShardContext); } else { // Unmapped field case - config = new ValuesSourceConfig(defaultValueSourceType(), queryShardContext); + config = ValuesSourceConfig.resolveUnmapped(defaultValueSourceType(), queryShardContext); } return config; } diff --git a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java index 90b64ccda7280..f920965e178cd 100644 --- a/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java +++ b/modules/parent-join/src/main/java/org/elasticsearch/join/aggregations/ParentAggregationBuilder.java @@ -107,17 +107,17 @@ protected ValuesSourceAggregatorFactory innerBuild(QueryShardContext queryShardC @Override protected ValuesSourceConfig resolveConfig(QueryShardContext queryShardContext) { - ValuesSourceConfig config; // = new ValuesSourceConfig(CoreValuesSourceType.BYTES, null, null); + ValuesSourceConfig config; ParentJoinFieldMapper parentJoinFieldMapper = ParentJoinFieldMapper.getMapper(queryShardContext.getMapperService()); ParentIdFieldMapper parentIdFieldMapper = parentJoinFieldMapper.getParentIdFieldMapper(childType, false); if (parentIdFieldMapper != null) { parentFilter = parentIdFieldMapper.getParentFilter(); childFilter = parentIdFieldMapper.getChildFilter(childType); MappedFieldType fieldType = parentIdFieldMapper.fieldType(); - config = new ValuesSourceConfig(fieldType.getValuesSourceType(), fieldType, queryShardContext); + config = ValuesSourceConfig.resolveFieldOnly(fieldType, queryShardContext); } else { // unmapped case - config = new ValuesSourceConfig(defaultValueSourceType(), queryShardContext); + config = ValuesSourceConfig.resolveUnmapped(defaultValueSourceType(), queryShardContext); } return config; } 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 62af06e5df39b..93dcfd90fa5e9 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 @@ -137,6 +137,23 @@ private static DocValueFormat resolveFormat(@Nullable String format, @Nullable V return valuesSourceType.getFormatter(format, tz); } + /** + * Special case factory method, intended to be used by aggregations which have some specialized logic for figuring out what field they + * are operating on, for example Parent & Child join aggregations, which use the join relation to find the field they are reading from + * rather than a user specified field. + */ + public static ValuesSourceConfig resolveFieldOnly(MappedFieldType fieldType, + QueryShardContext queryShardContext) { + return new ValuesSourceConfig(fieldType.getValuesSourceType(), fieldType, false, null, null, queryShardContext); + } + + /** + * Convenience method for creating unmapped configs + */ + public static ValuesSourceConfig resolveUnmapped(ValuesSourceType valuesSourceType, QueryShardContext queryShardContext) { + return new ValuesSourceConfig(valuesSourceType, null, true, null, null, queryShardContext); + } + private final ValuesSourceType valuesSourceType; private FieldContext fieldContext; private AggregationScript.LeafFactory script; @@ -145,25 +162,9 @@ private static DocValueFormat resolveFormat(@Nullable String format, @Nullable V private DocValueFormat format = DocValueFormat.RAW; private Object missing; private ZoneId timeZone; - private LongSupplier now; + private LongSupplier nowSupplier; - /** - * Config instances which only need to specify a field should use this constructor - */ - public ValuesSourceConfig(ValuesSourceType valuesSourceType, - MappedFieldType fieldType, - QueryShardContext queryShardContext) { - this(valuesSourceType, fieldType, false, null, null, queryShardContext); - } - - /** - * Convenience method for creating unmapped configs - */ - public ValuesSourceConfig(ValuesSourceType valuesSourceType, QueryShardContext queryShardContext) { - this(valuesSourceType, null, true, null, null, queryShardContext); - } - public ValuesSourceConfig(ValuesSourceType valuesSourceType, MappedFieldType fieldType, boolean unmapped, @@ -180,7 +181,7 @@ public ValuesSourceConfig(ValuesSourceType valuesSourceType, this.unmapped = unmapped; this.script = script; this.scriptValueType = scriptValueType; - this.now = queryShardContext::nowInMillis; + this.nowSupplier = queryShardContext::nowInMillis; } @@ -270,6 +271,6 @@ public ValuesSource toValuesSource() { if (missing() == null) { return vs; } - return valueSourceType().replaceMissing(vs, missing, format, now); + return valueSourceType().replaceMissing(vs, missing, format, nowSupplier); } } diff --git a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java index 74a8f1de3dced..c939e105ea0e8 100644 --- a/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java +++ b/x-pack/plugin/analytics/src/test/java/org/elasticsearch/xpack/analytics/cumulativecardinality/CumulativeCardinalityAggregatorTests.java @@ -117,7 +117,7 @@ public void testAllNull() throws IOException { } public void testParentValidations() throws IOException { - ValuesSourceConfig valuesSource = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, mock(QueryShardContext.class)); + ValuesSourceConfig valuesSource = ValuesSourceConfig.resolveUnmapped(CoreValuesSourceType.NUMERIC, mock(QueryShardContext.class)); // Histogram Set aggBuilders = new HashSet<>(); @@ -140,7 +140,7 @@ public void testParentValidations() throws IOException { builder.validate(parent, Collections.emptySet(), aggBuilders); // Auto Date Histogram - ValuesSourceConfig numericVS = new ValuesSourceConfig(CoreValuesSourceType.NUMERIC, mock(QueryShardContext.class)); + ValuesSourceConfig numericVS = ValuesSourceConfig.resolveUnmapped(CoreValuesSourceType.NUMERIC, mock(QueryShardContext.class)); aggBuilders.clear(); aggBuilders.add(new CumulativeCardinalityPipelineAggregationBuilder("cumulative_card", "sum")); AutoDateHistogramAggregationBuilder.RoundingInfo[] roundings = new AutoDateHistogramAggregationBuilder.RoundingInfo[1]; From bc792961d2b34131ae175b6e9bbf53bfdd07d245 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 25 Feb 2020 09:39:08 -0500 Subject: [PATCH 10/10] Ampersands in javadoc are compile errors --- .../search/aggregations/support/ValuesSourceConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 93dcfd90fa5e9..fa1cd558f8d09 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 @@ -139,7 +139,7 @@ private static DocValueFormat resolveFormat(@Nullable String format, @Nullable V /** * Special case factory method, intended to be used by aggregations which have some specialized logic for figuring out what field they - * are operating on, for example Parent & Child join aggregations, which use the join relation to find the field they are reading from + * are operating on, for example Parent and Child join aggregations, which use the join relation to find the field they are reading from * rather than a user specified field. */ public static ValuesSourceConfig resolveFieldOnly(MappedFieldType fieldType,