From b435cd0e12585aa17c5108c51f5ff951ebb56187 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 24 Jun 2020 15:12:10 -0400 Subject: [PATCH 1/6] Pull getPointReader up to AggregatorBase --- .../search/aggregations/AggregatorBase.java | 40 +++++++++++++++++++ .../aggregations/metrics/MaxAggregator.java | 2 - .../aggregations/metrics/MinAggregator.java | 38 ------------------ .../metrics/MinAggregatorTests.java | 25 ++++++------ 4 files changed, 53 insertions(+), 52 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java index 11ec88a8ce8bd..4833137f4876d 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java @@ -19,11 +19,16 @@ package org.elasticsearch.search.aggregations; import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.ScoreMode; import org.elasticsearch.common.breaker.CircuitBreaker; import org.elasticsearch.common.breaker.CircuitBreakingException; +import org.elasticsearch.index.mapper.DateFieldMapper; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.indices.breaker.CircuitBreakerService; import org.elasticsearch.search.SearchShardTarget; +import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.internal.SearchContext.Lifetime; import org.elasticsearch.search.query.QueryPhaseExecutionException; @@ -34,6 +39,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Function; /** * Base implementation for concrete aggregators. @@ -105,6 +111,40 @@ public ScoreMode scoreMode() { addRequestCircuitBreakerBytes(DEFAULT_WEIGHT); } + /** + * Returns a converter for point values if early termination is applicable to + * the context or null otherwise. + * + * @param context The {@link SearchContext} of the aggregation. + * @param parent The parent aggregator. + * @param config The config for the values source metric. + */ + protected static Function getPointReaderOrNull(SearchContext context, Aggregator parent, + ValuesSourceConfig config) { + if (context.query() != null && + context.query().getClass() != MatchAllDocsQuery.class) { + return null; + } + if (parent != null) { + return null; + } + if (config.fieldContext() != null && config.script() == null && config.missing() == null) { + MappedFieldType fieldType = config.fieldContext().fieldType(); + if (fieldType == null || fieldType.isSearchable() == false) { + return null; + } + Function converter = null; + if (fieldType instanceof NumberFieldMapper.NumberFieldType) { + converter = ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint; + } else if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) { + DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType; + converter = dft.resolution()::parsePointAsMillis; + } + return converter; + } + return null; + } + /** * Increment or decrement the number of bytes that have been allocated to service * this request and potentially trigger a {@link CircuitBreakingException}. The diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java index 20e354d01575e..a7e841668c208 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java @@ -44,8 +44,6 @@ import java.util.Map; import java.util.function.Function; -import static org.elasticsearch.search.aggregations.metrics.MinAggregator.getPointReaderOrNull; - class MaxAggregator extends NumericMetricsAggregator.SingleValue { final ValuesSource.Numeric valuesSource; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java index 34b905ec5fc57..4b580e4d44718 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java @@ -22,7 +22,6 @@ import org.apache.lucene.index.LeafReaderContext; import org.apache.lucene.index.PointValues; import org.apache.lucene.search.CollectionTerminatedException; -import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.ScoreMode; import org.apache.lucene.util.Bits; import org.elasticsearch.common.lease.Releasables; @@ -30,9 +29,6 @@ import org.elasticsearch.common.util.DoubleArray; import org.elasticsearch.index.fielddata.NumericDoubleValues; import org.elasticsearch.index.fielddata.SortedNumericDoubleValues; -import org.elasticsearch.index.mapper.DateFieldMapper; -import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.search.DocValueFormat; import org.elasticsearch.search.MultiValueMode; import org.elasticsearch.search.aggregations.Aggregator; @@ -159,40 +155,6 @@ public void doClose() { } - /** - * Returns a converter for point values if early termination is applicable to - * the context or null otherwise. - * - * @param context The {@link SearchContext} of the aggregation. - * @param parent The parent aggregator. - * @param config The config for the values source metric. - */ - static Function getPointReaderOrNull(SearchContext context, Aggregator parent, - ValuesSourceConfig config) { - if (context.query() != null && - context.query().getClass() != MatchAllDocsQuery.class) { - return null; - } - if (parent != null) { - return null; - } - if (config.fieldContext() != null && config.script() == null && config.missing() == null) { - MappedFieldType fieldType = config.fieldContext().fieldType(); - if (fieldType == null || fieldType.isSearchable() == false) { - return null; - } - Function converter = null; - if (fieldType instanceof NumberFieldMapper.NumberFieldType) { - converter = ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint; - } else if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) { - DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType; - converter = dft.resolution()::parsePointAsMillis; - } - return converter; - } - return null; - } - /** * Returns the minimum value indexed in the fieldName field or null * if the value cannot be inferred from the indexed {@link PointValues}. diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java index 8991639f5ac7e..9b9addd15361b 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java @@ -71,6 +71,7 @@ import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.aggregations.AggregationBuilder; import org.elasticsearch.search.aggregations.Aggregator; +import org.elasticsearch.search.aggregations.AggregatorBase; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.BucketOrder; import org.elasticsearch.search.aggregations.bucket.filter.Filter; @@ -693,42 +694,42 @@ public void testScriptCaching() throws IOException { public void testShortcutIsApplicable() { for (NumberFieldMapper.NumberType type : NumberFieldMapper.NumberType.values()) { assertNotNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(new MatchAllDocsQuery()), null, mockNumericValuesSourceConfig("number", type, true) ) ); assertNotNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(null), null, mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(null), mockAggregator(), mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(new TermQuery(new Term("foo", "bar"))), null, mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(null), mockAggregator(), mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(null), null, mockNumericValuesSourceConfig("number", type, false) @@ -737,28 +738,28 @@ public void testShortcutIsApplicable() { } for (DateFieldMapper.Resolution resolution : DateFieldMapper.Resolution.values()) { assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(new MatchAllDocsQuery()), mockAggregator(), mockDateValuesSourceConfig("number", true, resolution) ) ); assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(new TermQuery(new Term("foo", "bar"))), null, mockDateValuesSourceConfig("number", true, resolution) ) ); assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(null), mockAggregator(), mockDateValuesSourceConfig("number", true, resolution) ) ); assertNull( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(null), null, mockDateValuesSourceConfig("number", false, resolution) @@ -770,7 +771,7 @@ public void testShortcutIsApplicable() { byte[] scratch = new byte[8]; LongPoint.encodeDimension(DateFieldMapper.Resolution.MILLISECONDS.convert(expected), scratch, 0); assertThat( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(new MatchAllDocsQuery()), null, mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.MILLISECONDS) @@ -778,7 +779,7 @@ public void testShortcutIsApplicable() { ); LongPoint.encodeDimension(DateFieldMapper.Resolution.NANOSECONDS.convert(expected), scratch, 0); assertThat( - MinAggregator.getPointReaderOrNull( + AggregatorBase.getPointReaderOrNull( mockSearchContext(new MatchAllDocsQuery()), null, mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.NANOSECONDS) From c4f79ed84da874f803818e1687f49b82447a6d91 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 24 Jun 2020 15:28:20 -0400 Subject: [PATCH 2/6] Make getPointReader instance method --- .../search/aggregations/AggregatorBase.java | 5 +--- .../aggregations/metrics/MaxAggregator.java | 4 +-- .../aggregations/metrics/MinAggregator.java | 2 +- .../metrics/MinAggregatorTests.java | 25 ------------------- 4 files changed, 4 insertions(+), 32 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java index 4833137f4876d..e58904674d1e0 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java @@ -115,12 +115,9 @@ public ScoreMode scoreMode() { * Returns a converter for point values if early termination is applicable to * the context or null otherwise. * - * @param context The {@link SearchContext} of the aggregation. - * @param parent The parent aggregator. * @param config The config for the values source metric. */ - protected static Function getPointReaderOrNull(SearchContext context, Aggregator parent, - ValuesSourceConfig config) { + protected Function getPointReaderOrNull(ValuesSourceConfig config) { if (context.query() != null && context.query().getClass() != MatchAllDocsQuery.class) { return null; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java index a7e841668c208..0cb4202818baa 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java @@ -66,7 +66,7 @@ class MaxAggregator extends NumericMetricsAggregator.SingleValue { maxes.fill(0, maxes.size(), Double.NEGATIVE_INFINITY); } this.formatter = config.format(); - this.pointConverter = getPointReaderOrNull(context, parent, config); + this.pointConverter = getPointReaderOrNull(config); if (pointConverter != null) { pointField = config.fieldContext().field(); } else { @@ -94,7 +94,7 @@ public LeafBucketCollector getLeafCollector(LeafReaderContext ctx, Number segMax = findLeafMaxValue(ctx.reader(), pointField, pointConverter); if (segMax != null) { /* - * There is no parent aggregator (see {@link MinAggregator#getPointReaderOrNull} + * There is no parent aggregator (see {@link AggregatorBase#getPointReaderOrNull} * so the ordinal for the bucket is always 0. */ assert maxes.size() == 1; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java index 4b580e4d44718..c809957e1dc83 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java @@ -67,7 +67,7 @@ class MinAggregator extends NumericMetricsAggregator.SingleValue { mins.fill(0, mins.size(), Double.POSITIVE_INFINITY); } this.format = config.format(); - this.pointConverter = getPointReaderOrNull(context, parent, config); + this.pointConverter = getPointReaderOrNull(config); if (pointConverter != null) { pointField = config.fieldContext().field(); } else { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java index 9b9addd15361b..b5ef56daba542 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java @@ -43,7 +43,6 @@ import org.apache.lucene.search.IndexSearcher; import org.apache.lucene.search.MatchAllDocsQuery; import org.apache.lucene.search.Query; -import org.apache.lucene.search.TermQuery; import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; import org.elasticsearch.Version; @@ -695,43 +694,31 @@ public void testShortcutIsApplicable() { for (NumberFieldMapper.NumberType type : NumberFieldMapper.NumberType.values()) { assertNotNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(new MatchAllDocsQuery()), - null, mockNumericValuesSourceConfig("number", type, true) ) ); assertNotNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(null), - null, mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(null), - mockAggregator(), mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(new TermQuery(new Term("foo", "bar"))), - null, mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(null), - mockAggregator(), mockNumericValuesSourceConfig("number", type, true) ) ); assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(null), - null, mockNumericValuesSourceConfig("number", type, false) ) ); @@ -739,29 +726,21 @@ public void testShortcutIsApplicable() { for (DateFieldMapper.Resolution resolution : DateFieldMapper.Resolution.values()) { assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(new MatchAllDocsQuery()), - mockAggregator(), mockDateValuesSourceConfig("number", true, resolution) ) ); assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(new TermQuery(new Term("foo", "bar"))), - null, mockDateValuesSourceConfig("number", true, resolution) ) ); assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(null), - mockAggregator(), mockDateValuesSourceConfig("number", true, resolution) ) ); assertNull( AggregatorBase.getPointReaderOrNull( - mockSearchContext(null), - null, mockDateValuesSourceConfig("number", false, resolution) ) ); @@ -772,16 +751,12 @@ public void testShortcutIsApplicable() { LongPoint.encodeDimension(DateFieldMapper.Resolution.MILLISECONDS.convert(expected), scratch, 0); assertThat( AggregatorBase.getPointReaderOrNull( - mockSearchContext(new MatchAllDocsQuery()), - null, mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.MILLISECONDS) ).apply(scratch), equalTo(expected.toEpochMilli()) ); LongPoint.encodeDimension(DateFieldMapper.Resolution.NANOSECONDS.convert(expected), scratch, 0); assertThat( AggregatorBase.getPointReaderOrNull( - mockSearchContext(new MatchAllDocsQuery()), - null, mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.NANOSECONDS) ).apply(scratch), equalTo(expected.toEpochMilli()) ); From 0f1dc77dd0dd50f93d6d75cbb0090fa74d144d0f Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Tue, 30 Jun 2020 13:42:05 -0400 Subject: [PATCH 3/6] pull getPointReaderOrNull up to the aggregator base --- .../search/aggregations/AggregatorBase.java | 24 +-- .../support/ValuesSourceConfig.java | 23 +++ .../aggregations/AggregatorBaseTests.java | 192 ++++++++++++++++++ .../metrics/MinAggregatorTests.java | 120 ----------- 4 files changed, 218 insertions(+), 141 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java index e58904674d1e0..5360fce4f4b53 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java @@ -23,9 +23,6 @@ import org.apache.lucene.search.ScoreMode; import org.elasticsearch.common.breaker.CircuitBreaker; import org.elasticsearch.common.breaker.CircuitBreakingException; -import org.elasticsearch.index.mapper.DateFieldMapper; -import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.indices.breaker.CircuitBreakerService; import org.elasticsearch.search.SearchShardTarget; import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; @@ -117,29 +114,14 @@ public ScoreMode scoreMode() { * * @param config The config for the values source metric. */ - protected Function getPointReaderOrNull(ValuesSourceConfig config) { - if (context.query() != null && - context.query().getClass() != MatchAllDocsQuery.class) { + public Function getPointReaderOrNull(ValuesSourceConfig config) { + if (context.query() != null && context.query().getClass() != MatchAllDocsQuery.class) { return null; } if (parent != null) { return null; } - if (config.fieldContext() != null && config.script() == null && config.missing() == null) { - MappedFieldType fieldType = config.fieldContext().fieldType(); - if (fieldType == null || fieldType.isSearchable() == false) { - return null; - } - Function converter = null; - if (fieldType instanceof NumberFieldMapper.NumberFieldType) { - converter = ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint; - } else if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) { - DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType; - converter = dft.resolution()::parsePointAsMillis; - } - return converter; - } - return null; + return config.getPointReaderOrNull(); } /** 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 a4ae01f372d4b..0dfbd7e5178f1 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 @@ -22,7 +22,9 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; import org.elasticsearch.index.fielddata.IndexNumericFieldData; +import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.script.AggregationScript; @@ -30,6 +32,7 @@ import org.elasticsearch.search.DocValueFormat; import java.time.ZoneId; +import java.util.function.Function; import java.util.function.LongSupplier; /** @@ -371,4 +374,24 @@ public ValuesSource getValuesSource() { public boolean hasGlobalOrdinals() { return valuesSource.hasGlobalOrdinals(); } + + @Nullable + public Function getPointReaderOrNull() { + if (fieldContext() != null && script() == null && missing() == null) { + MappedFieldType fieldType = fieldContext().fieldType(); + if (fieldType == null || fieldType.isSearchable() == false) { + return null; + } + Function converter = null; + if (fieldType instanceof NumberFieldMapper.NumberFieldType) { + converter = ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint; + } else if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) { + DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType; + converter = dft.resolution()::parsePointAsMillis; + } + return converter; + } + return null; + + } } diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java new file mode 100644 index 0000000000000..21857057aabc6 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java @@ -0,0 +1,192 @@ +/* + * 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; + +import org.apache.lucene.document.LongPoint; +import org.apache.lucene.index.LeafReaderContext; +import org.apache.lucene.index.Term; +import org.apache.lucene.search.MatchAllDocsQuery; +import org.apache.lucene.search.Query; +import org.apache.lucene.search.TermQuery; +import org.elasticsearch.Version; +import org.elasticsearch.action.support.WriteRequest; +import org.elasticsearch.cluster.metadata.IndexMetadata; +import org.elasticsearch.common.breaker.CircuitBreaker; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.BigArrays; +import org.elasticsearch.index.IndexService; +import org.elasticsearch.index.engine.Engine; +import org.elasticsearch.index.mapper.ContentPath; +import org.elasticsearch.index.mapper.DateFieldMapper; +import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.Mapper; +import org.elasticsearch.index.mapper.NumberFieldMapper; +import org.elasticsearch.index.query.QueryShardContext; +import org.elasticsearch.indices.breaker.CircuitBreakerService; +import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; +import org.elasticsearch.search.internal.SearchContext; +import org.elasticsearch.test.ESSingleNodeTestCase; + +import java.io.IOException; +import java.time.Instant; +import java.util.Collections; +import java.util.Map; +import java.util.function.Function; + +import static org.hamcrest.Matchers.equalTo; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +public class AggregatorBaseTests extends ESSingleNodeTestCase { + + class BogusAggregator extends AggregatorBase { + BogusAggregator(SearchContext searchContext, Aggregator parent) throws IOException { + super("bogus", AggregatorFactories.EMPTY, searchContext, parent, Map.of()); + } + + @Override + protected LeafBucketCollector getLeafCollector(LeafReaderContext ctx, LeafBucketCollector sub) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public InternalAggregation[] buildAggregations(long[] owningBucketOrds) throws IOException { + throw new UnsupportedOperationException(); + } + + @Override + public InternalAggregation buildEmptyAggregation() { + throw new UnsupportedOperationException(); + } + } + + private SearchContext mockSearchContext(Query query) { + SearchContext searchContext = mock(SearchContext.class); + when(searchContext.query()).thenReturn(query); + BigArrays mockBigArrays = mock(BigArrays.class); + CircuitBreakerService mockCBS = mock(CircuitBreakerService.class); + when(mockCBS.getBreaker(CircuitBreaker.REQUEST)).thenReturn(mock(CircuitBreaker.class)); + when(mockBigArrays.breakerService()).thenReturn(mockCBS); + when(searchContext.bigArrays()).thenReturn(mockBigArrays); + return searchContext; + } + + private Function pointReaderShim(SearchContext context, Aggregator parent, ValuesSourceConfig config) + throws IOException { + BogusAggregator aggregator = new BogusAggregator(context, parent); + return aggregator.getPointReaderOrNull(config); + } + + private Aggregator mockAggregator() { + return mock(Aggregator.class); + } + + private ValuesSourceConfig getVSConfig( + String fieldName, + NumberFieldMapper.NumberType numType, + boolean indexed, + QueryShardContext context + ) { + MappedFieldType ft = new NumberFieldMapper.NumberFieldType(fieldName, numType, indexed, true, Collections.emptyMap()); + return ValuesSourceConfig.resolveFieldOnly(ft, context); + } + + private ValuesSourceConfig getVSConfig( + String fieldName, + DateFieldMapper.Resolution resolution, + boolean indexed, + QueryShardContext context + ) { + Mapper.BuilderContext builderContext = new Mapper.BuilderContext( + Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT).build(), + new ContentPath() + ); + MappedFieldType ft = new DateFieldMapper.Builder(fieldName).index(indexed) + .withResolution(resolution) + .build(builderContext) + .fieldType(); + return ValuesSourceConfig.resolveFieldOnly(ft, context); + } + + public void testShortcutIsApplicable() throws IOException { + IndexService indexService = createIndex("index", Settings.EMPTY, "type", "bytes", "type=keyword"); + client().prepareIndex("index").setId("1").setSource("bytes", "abc").setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE).get(); + + try (Engine.Searcher searcher = indexService.getShard(0).acquireSearcher("test")) { + QueryShardContext context = indexService.newQueryShardContext(0, searcher, () -> 42L, null); + + for (NumberFieldMapper.NumberType type : NumberFieldMapper.NumberType.values()) { + assertNotNull( + pointReaderShim(mockSearchContext(new MatchAllDocsQuery()), null, getVSConfig("number", type, true, context)) + ); + assertNotNull(pointReaderShim(mockSearchContext(null), null, getVSConfig("number", type, true, context))); + assertNull(pointReaderShim(mockSearchContext(null), mockAggregator(), getVSConfig("number", type, true, context))); + assertNull( + pointReaderShim( + mockSearchContext(new TermQuery(new Term("foo", "bar"))), + null, + getVSConfig("number", type, true, context) + ) + ); + assertNull(pointReaderShim(mockSearchContext(null), mockAggregator(), getVSConfig("number", type, true, context))); + assertNull(pointReaderShim(mockSearchContext(null), null, getVSConfig("number", type, false, context))); + } + for (DateFieldMapper.Resolution resolution : DateFieldMapper.Resolution.values()) { + assertNull( + pointReaderShim( + mockSearchContext(new MatchAllDocsQuery()), + mockAggregator(), + getVSConfig("number", resolution, true, context) + ) + ); + assertNull( + pointReaderShim( + mockSearchContext(new TermQuery(new Term("foo", "bar"))), + null, + getVSConfig("number", resolution, true, context) + ) + ); + assertNull(pointReaderShim(mockSearchContext(null), mockAggregator(), getVSConfig("number", resolution, true, context))); + assertNull(pointReaderShim(mockSearchContext(null), null, getVSConfig("number", resolution, false, context))); + } + // Check that we decode a dates "just like" the doc values instance. + Instant expected = Instant.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse("2020-01-01T00:00:00Z")); + byte[] scratch = new byte[8]; + LongPoint.encodeDimension(DateFieldMapper.Resolution.MILLISECONDS.convert(expected), scratch, 0); + assertThat( + pointReaderShim( + mockSearchContext(new MatchAllDocsQuery()), + null, + getVSConfig("number", DateFieldMapper.Resolution.MILLISECONDS, true, context) + ).apply(scratch), + equalTo(expected.toEpochMilli()) + ); + LongPoint.encodeDimension(DateFieldMapper.Resolution.NANOSECONDS.convert(expected), scratch, 0); + assertThat( + pointReaderShim( + mockSearchContext(new MatchAllDocsQuery()), + null, + getVSConfig("number", DateFieldMapper.Resolution.NANOSECONDS, true, context) + ).apply(scratch), + equalTo(expected.toEpochMilli()) + ); + } + } +} diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java index b5ef56daba542..0886ad91015f1 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java @@ -45,19 +45,14 @@ import org.apache.lucene.search.Query; import org.apache.lucene.store.Directory; import org.apache.lucene.util.BytesRef; -import org.elasticsearch.Version; -import org.elasticsearch.cluster.metadata.IndexMetadata; import org.elasticsearch.common.CheckedConsumer; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.BigArrays; import org.elasticsearch.index.IndexSettings; -import org.elasticsearch.index.mapper.ContentPath; -import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.IpFieldMapper; import org.elasticsearch.index.mapper.KeywordFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.index.mapper.Mapper; import org.elasticsearch.index.mapper.MapperService; import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.index.query.QueryShardContext; @@ -69,8 +64,6 @@ import org.elasticsearch.script.ScriptService; import org.elasticsearch.script.ScriptType; import org.elasticsearch.search.aggregations.AggregationBuilder; -import org.elasticsearch.search.aggregations.Aggregator; -import org.elasticsearch.search.aggregations.AggregatorBase; import org.elasticsearch.search.aggregations.AggregatorTestCase; import org.elasticsearch.search.aggregations.BucketOrder; import org.elasticsearch.search.aggregations.bucket.filter.Filter; @@ -84,13 +77,9 @@ import org.elasticsearch.search.aggregations.bucket.terms.Terms; import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder; import org.elasticsearch.search.aggregations.support.AggregationInspectionHelper; -import org.elasticsearch.search.aggregations.support.FieldContext; -import org.elasticsearch.search.aggregations.support.ValuesSourceConfig; -import org.elasticsearch.search.internal.SearchContext; import org.elasticsearch.search.lookup.LeafDocLookup; import java.io.IOException; -import java.time.Instant; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -106,8 +95,6 @@ import static java.util.Collections.singleton; import static org.elasticsearch.index.query.QueryBuilders.termQuery; import static org.hamcrest.Matchers.equalTo; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.when; public class MinAggregatorTests extends AggregatorTestCase { @@ -690,79 +677,6 @@ public void testScriptCaching() throws IOException { } } - public void testShortcutIsApplicable() { - for (NumberFieldMapper.NumberType type : NumberFieldMapper.NumberType.values()) { - assertNotNull( - AggregatorBase.getPointReaderOrNull( - mockNumericValuesSourceConfig("number", type, true) - ) - ); - assertNotNull( - AggregatorBase.getPointReaderOrNull( - mockNumericValuesSourceConfig("number", type, true) - ) - ); - assertNull( - AggregatorBase.getPointReaderOrNull( - mockNumericValuesSourceConfig("number", type, true) - ) - ); - assertNull( - AggregatorBase.getPointReaderOrNull( - mockNumericValuesSourceConfig("number", type, true) - ) - ); - assertNull( - AggregatorBase.getPointReaderOrNull( - mockNumericValuesSourceConfig("number", type, true) - ) - ); - assertNull( - AggregatorBase.getPointReaderOrNull( - mockNumericValuesSourceConfig("number", type, false) - ) - ); - } - for (DateFieldMapper.Resolution resolution : DateFieldMapper.Resolution.values()) { - assertNull( - AggregatorBase.getPointReaderOrNull( - mockDateValuesSourceConfig("number", true, resolution) - ) - ); - assertNull( - AggregatorBase.getPointReaderOrNull( - mockDateValuesSourceConfig("number", true, resolution) - ) - ); - assertNull( - AggregatorBase.getPointReaderOrNull( - mockDateValuesSourceConfig("number", true, resolution) - ) - ); - assertNull( - AggregatorBase.getPointReaderOrNull( - mockDateValuesSourceConfig("number", false, resolution) - ) - ); - } - // Check that we decode a dates "just like" the doc values instance. - Instant expected = Instant.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse("2020-01-01T00:00:00Z")); - byte[] scratch = new byte[8]; - LongPoint.encodeDimension(DateFieldMapper.Resolution.MILLISECONDS.convert(expected), scratch, 0); - assertThat( - AggregatorBase.getPointReaderOrNull( - mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.MILLISECONDS) - ).apply(scratch), equalTo(expected.toEpochMilli()) - ); - LongPoint.encodeDimension(DateFieldMapper.Resolution.NANOSECONDS.convert(expected), scratch, 0); - assertThat( - AggregatorBase.getPointReaderOrNull( - mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.NANOSECONDS) - ).apply(scratch), equalTo(expected.toEpochMilli()) - ); - - } - public void testMinShortcutRandom() throws Exception { testMinShortcutCase( () -> randomLongBetween(Integer.MIN_VALUE, Integer.MAX_VALUE), @@ -838,40 +752,6 @@ private void testMinShortcutCase(Supplier randomNumber, directory.close(); } - private SearchContext mockSearchContext(Query query) { - SearchContext searchContext = mock(SearchContext.class); - when(searchContext.query()).thenReturn(query); - return searchContext; - } - - private Aggregator mockAggregator() { - return mock(Aggregator.class); - } - - private ValuesSourceConfig mockNumericValuesSourceConfig(String fieldName, - NumberFieldMapper.NumberType numType, - boolean indexed) { - ValuesSourceConfig config = mock(ValuesSourceConfig.class); - MappedFieldType ft = new NumberFieldMapper.NumberFieldType(fieldName, numType, indexed, true, Collections.emptyMap()); - when(config.fieldContext()).thenReturn(new FieldContext(fieldName, null, ft)); - return config; - } - - private ValuesSourceConfig mockDateValuesSourceConfig(String fieldName, boolean indexed, - DateFieldMapper.Resolution resolution) { - ValuesSourceConfig config = mock(ValuesSourceConfig.class); - Mapper.BuilderContext builderContext = new Mapper.BuilderContext( - Settings.builder().put(IndexMetadata.SETTING_VERSION_CREATED, Version.CURRENT).build(), - new ContentPath()); - MappedFieldType ft = new DateFieldMapper.Builder(fieldName) - .index(indexed) - .withResolution(resolution) - .build(builderContext) - .fieldType(); - when(config.fieldContext()).thenReturn(new FieldContext(fieldName, null, ft)); - return config; - } - private void testCase(Query query, CheckedConsumer buildIndex, Consumer verify) throws IOException { From fcd9704d36ee6d7490fc203ce9d789c7f0fff5af Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 1 Jul 2020 09:42:59 -0400 Subject: [PATCH 4/6] response to PR feedback --- .../search/aggregations/AggregatorBase.java | 10 +++++++--- .../search/aggregations/metrics/MaxAggregator.java | 2 +- .../search/aggregations/metrics/MinAggregator.java | 2 +- .../search/aggregations/AggregatorBaseTests.java | 2 +- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java index 214d2676d6dbf..520094fe98143 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/AggregatorBase.java @@ -109,12 +109,16 @@ public ScoreMode scoreMode() { } /** - * Returns a converter for point values if early termination is applicable to - * the context or null otherwise. + * Returns a converter for point values if it's safe to use the indexed data instead of + * doc values. Generally, this means that the query has no filters or scripts, the aggregation is + * top level, and the underlying field is indexed, and the index is sorted in the right order. + * + * If those conditions aren't met, return null to indicate a point reader cannot + * be used in this case. * * @param config The config for the values source metric. */ - public Function getPointReaderOrNull(ValuesSourceConfig config) { + public final Function pointReaderIfAvailable(ValuesSourceConfig config) { if (context.query() != null && context.query().getClass() != MatchAllDocsQuery.class) { return null; } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java index 0cb4202818baa..0e454db0d6566 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java @@ -66,7 +66,7 @@ class MaxAggregator extends NumericMetricsAggregator.SingleValue { maxes.fill(0, maxes.size(), Double.NEGATIVE_INFINITY); } this.formatter = config.format(); - this.pointConverter = getPointReaderOrNull(config); + this.pointConverter = pointReaderIfAvailable(config); if (pointConverter != null) { pointField = config.fieldContext().field(); } else { diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java index c809957e1dc83..1fa12e9719882 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java @@ -67,7 +67,7 @@ class MinAggregator extends NumericMetricsAggregator.SingleValue { mins.fill(0, mins.size(), Double.POSITIVE_INFINITY); } this.format = config.format(); - this.pointConverter = getPointReaderOrNull(config); + this.pointConverter = pointReaderIfAvailable(config); if (pointConverter != null) { pointField = config.fieldContext().field(); } else { diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java index 21857057aabc6..5ec245f3f2649 100644 --- a/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java +++ b/server/src/test/java/org/elasticsearch/search/aggregations/AggregatorBaseTests.java @@ -91,7 +91,7 @@ private SearchContext mockSearchContext(Query query) { private Function pointReaderShim(SearchContext context, Aggregator parent, ValuesSourceConfig config) throws IOException { BogusAggregator aggregator = new BogusAggregator(context, parent); - return aggregator.getPointReaderOrNull(config); + return aggregator.pointReaderIfAvailable(config); } private Aggregator mockAggregator() { From 37aa3694a0b0dddc2cdfa693ad0423286bb282a9 Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 1 Jul 2020 11:17:38 -0400 Subject: [PATCH 5/6] Add extension point for point reader lookup --- .../support/CoreValuesSourceType.java | 18 ++++++++++++++++++ .../support/ValuesSourceConfig.java | 19 +++++++++---------- .../support/ValuesSourceType.java | 11 +++++++++++ 3 files changed, 38 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java index a6944a6e4119f..fece0ebd7183f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java @@ -32,6 +32,7 @@ import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.DateFieldMapper.DateFieldType; import org.elasticsearch.index.mapper.MappedFieldType; +import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.script.AggregationScript; import org.elasticsearch.search.DocValueFormat; @@ -85,6 +86,14 @@ public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, return MissingValues.replaceMissing((ValuesSource.Numeric) valuesSource, missing); } + @Override + public Function getPointReader(MappedFieldType fieldType) { + if (fieldType instanceof NumberFieldMapper.NumberFieldType) { + return ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint; + } + return null; + } + @Override public DocValueFormat getFormatter(String format, ZoneId tz) { /* TODO: this silently ignores a timezone argument, whereas NumberFieldType#docValueFormat throws if given a time zone. @@ -294,6 +303,15 @@ public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, return NUMERIC.replaceMissing(valuesSource, rawMissing, docValueFormat, nowSupplier); } + @Override + public Function getPointReader(MappedFieldType fieldType) { + if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) { + DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType; + return dft.resolution()::parsePointAsMillis; + } + return null; + } + @Override public DocValueFormat getFormatter(String format, ZoneId tz) { return new DocValueFormat.DateTime( 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 0dfbd7e5178f1..b5366530e974f 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 @@ -22,9 +22,7 @@ import org.elasticsearch.index.fielddata.IndexFieldData; import org.elasticsearch.index.fielddata.IndexGeoPointFieldData; import org.elasticsearch.index.fielddata.IndexNumericFieldData; -import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.index.query.QueryShardContext; import org.elasticsearch.script.AggregationScript; @@ -375,6 +373,14 @@ public boolean hasGlobalOrdinals() { return valuesSource.hasGlobalOrdinals(); } + /** + * This method is used when an aggregation can optimize by using the indexed data instead of the doc values. We check to see if the + * indexed data will match the values source output (meaning there isn't a script or a missing value, since both could modify the + * value at read time). If the settings allow for it, we then ask the {@link ValuesSourceType} to build the actual point reader + * based on the field type. This allows for a point of extensibility in plugins. + * + * @return null if we cannot apply the optimization, otherwise the point reader function. + */ @Nullable public Function getPointReaderOrNull() { if (fieldContext() != null && script() == null && missing() == null) { @@ -382,14 +388,7 @@ public Function getPointReaderOrNull() { if (fieldType == null || fieldType.isSearchable() == false) { return null; } - Function converter = null; - if (fieldType instanceof NumberFieldMapper.NumberFieldType) { - converter = ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint; - } else if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) { - DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType; - converter = dft.resolution()::parsePointAsMillis; - } - return converter; + return valuesSourceType.getPointReader(fieldType); } return null; diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java index 1e858528b7399..540679a855731 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java @@ -19,10 +19,12 @@ package org.elasticsearch.search.aggregations.support; +import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.script.AggregationScript; import org.elasticsearch.search.DocValueFormat; import java.time.ZoneId; +import java.util.function.Function; import java.util.function.LongSupplier; /** @@ -85,6 +87,15 @@ public interface ValuesSourceType { ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier nowSupplier); + /** + * Attempts to return a reader function for the indexed data of this field. Some aggregations are able to use this as an optimization + * instead of relying on doc values, when the index sort order matches that of the aggregation. + * + * @param fieldType The field type we want to get a reader for + * @return null if we can't get a reader (e.g. because the field is the wrong type), otherwise a point reader function. + */ + default Function getPointReader(MappedFieldType fieldType) { return null; } + /** * This method provides a hook for specifying a type-specific formatter. When {@link ValuesSourceConfig} can resolve a * {@link org.elasticsearch.index.mapper.MappedFieldType}, it prefers to get the formatter from there. Only when a field can't be From a5c82c4d5af43bdc882b052f5ac36d8bfb0bceea Mon Sep 17 00:00:00 2001 From: Mark Tozzi Date: Wed, 1 Jul 2020 17:22:36 -0400 Subject: [PATCH 6/6] move point reader creation onto MappedFieldType --- .../index/mapper/DateFieldMapper.java | 9 +++++++++ .../index/mapper/MappedFieldType.java | 14 +++++++++++++- .../index/mapper/NumberFieldMapper.java | 9 +++++++++ .../support/CoreValuesSourceType.java | 18 ------------------ .../support/ValuesSourceConfig.java | 10 +++------- .../aggregations/support/ValuesSourceType.java | 11 ----------- 6 files changed, 34 insertions(+), 37 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java index 98f222e822477..0abc7c5ceafcc 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/DateFieldMapper.java @@ -65,6 +65,7 @@ import java.util.Locale; import java.util.Map; import java.util.Objects; +import java.util.function.Function; import java.util.function.LongSupplier; import static org.elasticsearch.common.time.DateUtils.toLong; @@ -516,6 +517,14 @@ private Relation isFieldWithinRange(IndexReader reader, long fromInclusive, long } } + @Override + public Function pointReaderIfPossible() { + if (isSearchable()) { + return resolution()::parsePointAsMillis; + } + return null; + } + @Override public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName) { failIfNoDocValues(); diff --git a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java index b7c9ed5e7a9d5..3ca0a7c28c9e7 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/MappedFieldType.java @@ -53,6 +53,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.function.Function; /** * This defines the core properties and functions to operate on a field. @@ -135,7 +136,7 @@ public int hashCode() { /** Returns the name of this type, as would be specified in mapping properties */ public abstract String typeName(); - + /** Returns the field family type, as used in field capabilities */ public String familyTypeName() { return typeName(); @@ -195,6 +196,17 @@ public boolean isSearchable() { return isIndexed; } + /** + * If the field supports using the indexed data to speed up operations related to ordering of data, such as sorting or aggs, return + * a function for doing that. If it is unsupported for this field type, there is no need to override this method. + * + * @return null if the optimization cannot be applied, otherwise a function to use for the optimization + */ + @Nullable + public Function pointReaderIfPossible() { + return null; + } + /** Returns true if the field is aggregatable. * */ diff --git a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java index 94af1ce030f30..86ede5e1c2120 100644 --- a/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java +++ b/server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java @@ -66,6 +66,7 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.function.Function; /** A {@link FieldMapper} for numeric types: byte, short, int, long, float and double. */ public class NumberFieldMapper extends FieldMapper { @@ -972,6 +973,14 @@ public Query rangeQuery(Object lowerTerm, Object upperTerm, boolean includeLower return query; } + @Override + public Function pointReaderIfPossible() { + if (isSearchable()) { + return this::parsePoint; + } + return null; + } + @Override public IndexFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName) { failIfNoDocValues(); diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java index fece0ebd7183f..a6944a6e4119f 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/CoreValuesSourceType.java @@ -32,7 +32,6 @@ import org.elasticsearch.index.mapper.DateFieldMapper; import org.elasticsearch.index.mapper.DateFieldMapper.DateFieldType; import org.elasticsearch.index.mapper.MappedFieldType; -import org.elasticsearch.index.mapper.NumberFieldMapper; import org.elasticsearch.index.mapper.RangeFieldMapper; import org.elasticsearch.script.AggregationScript; import org.elasticsearch.search.DocValueFormat; @@ -86,14 +85,6 @@ public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, return MissingValues.replaceMissing((ValuesSource.Numeric) valuesSource, missing); } - @Override - public Function getPointReader(MappedFieldType fieldType) { - if (fieldType instanceof NumberFieldMapper.NumberFieldType) { - return ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint; - } - return null; - } - @Override public DocValueFormat getFormatter(String format, ZoneId tz) { /* TODO: this silently ignores a timezone argument, whereas NumberFieldType#docValueFormat throws if given a time zone. @@ -303,15 +294,6 @@ public ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, return NUMERIC.replaceMissing(valuesSource, rawMissing, docValueFormat, nowSupplier); } - @Override - public Function getPointReader(MappedFieldType fieldType) { - if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) { - DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType; - return dft.resolution()::parsePointAsMillis; - } - return null; - } - @Override public DocValueFormat getFormatter(String format, ZoneId tz) { return new DocValueFormat.DateTime( 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 b5366530e974f..504aba53c4766 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 @@ -383,14 +383,10 @@ public boolean hasGlobalOrdinals() { */ @Nullable public Function getPointReaderOrNull() { - if (fieldContext() != null && script() == null && missing() == null) { - MappedFieldType fieldType = fieldContext().fieldType(); - if (fieldType == null || fieldType.isSearchable() == false) { - return null; - } - return valuesSourceType.getPointReader(fieldType); + MappedFieldType fieldType = fieldType(); + if (fieldType != null && script() == null && missing() == null) { + return fieldType.pointReaderIfPossible(); } return null; - } } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java index 540679a855731..1e858528b7399 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/support/ValuesSourceType.java @@ -19,12 +19,10 @@ package org.elasticsearch.search.aggregations.support; -import org.elasticsearch.index.mapper.MappedFieldType; import org.elasticsearch.script.AggregationScript; import org.elasticsearch.search.DocValueFormat; import java.time.ZoneId; -import java.util.function.Function; import java.util.function.LongSupplier; /** @@ -87,15 +85,6 @@ public interface ValuesSourceType { ValuesSource replaceMissing(ValuesSource valuesSource, Object rawMissing, DocValueFormat docValueFormat, LongSupplier nowSupplier); - /** - * Attempts to return a reader function for the indexed data of this field. Some aggregations are able to use this as an optimization - * instead of relying on doc values, when the index sort order matches that of the aggregation. - * - * @param fieldType The field type we want to get a reader for - * @return null if we can't get a reader (e.g. because the field is the wrong type), otherwise a point reader function. - */ - default Function getPointReader(MappedFieldType fieldType) { return null; } - /** * This method provides a hook for specifying a type-specific formatter. When {@link ValuesSourceConfig} can resolve a * {@link org.elasticsearch.index.mapper.MappedFieldType}, it prefers to get the formatter from there. Only when a field can't be