Decode max and min optimization more carefully (#52336) (#52358)

Fixes the the no-query optimization for `min` and `max` aggregations
for `date_nanos` fields by delegating decoding dates "through" their
`resolution` member.

Closes #52220

Author: nik9000

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java

@@ -98,7 +98,7 @@ public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
         if (pointConverter != null) {
             Number segMax = findLeafMaxValue(ctx.reader(), pointField, pointConverter);
             if (segMax != null) {
-                /**
+                /*
                  * There is no parent aggregator (see {@link MinAggregator#getPointReaderOrNull}
                  * so the ordinal for the bucket is always 0.
                  */

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java

@@ -103,7 +103,7 @@ public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
         if (pointConverter != null) {
             Number segMin = findLeafMinValue(ctx.reader(), pointField, pointConverter);
             if (segMin != null) {
-                /**
+                /*
                  * There is no parent aggregator (see {@link MinAggregator#getPointReaderOrNull}
                  * so the ordinal for the bucket is always 0.
                  */
@@ -190,7 +190,12 @@ static Function<byte[], Number> getPointReaderOrNull(SearchContext context, Aggr
             if (fieldType instanceof NumberFieldMapper.NumberFieldType) {
                 converter = ((NumberFieldMapper.NumberFieldType) fieldType)::parsePoint;
             } else if (fieldType.getClass() == DateFieldMapper.DateFieldType.class) {
-                converter = (in) -> LongPoint.decodeDimension(in, 0);
+                DateFieldMapper.DateFieldType dft = (DateFieldMapper.DateFieldType) fieldType;
+                /*
+                 * Makes sure that nanoseconds decode to milliseconds, just
+                 * like they do when you run the agg without the optimization.
+                 */
+                converter = (in) -> dft.resolution().toInstant(LongPoint.decodeDimension(in, 0)).toEpochMilli();
             }
             return converter;
         }

server/src/test/java/org/elasticsearch/search/aggregations/metrics/MinAggregatorTests.java

@@ -46,13 +46,17 @@
 import org.apache.lucene.search.TermQuery;
 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.index.IndexSettings;
+import org.elasticsearch.index.mapper.ContentPath;
 import org.elasticsearch.index.mapper.DateFieldMapper;
 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;
@@ -87,17 +91,16 @@
 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.Collection;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.function.BiFunction;
 import java.util.function.Consumer;
-import java.util.function.DoubleConsumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
@@ -740,41 +743,56 @@ public void testShortcutIsApplicable() {
                 )
             );
         }
-        assertNotNull(
+        for (DateFieldMapper.Resolution resolution : DateFieldMapper.Resolution.values()) {
+            assertNull(
+                MinAggregator.getPointReaderOrNull(
+                    mockSearchContext(new MatchAllDocsQuery()),
+                    mockAggregator(),
+                    mockDateValuesSourceConfig("number", true, resolution)
+                )
+            );
+            assertNull(
+                MinAggregator.getPointReaderOrNull(
+                    mockSearchContext(new TermQuery(new Term("foo", "bar"))),
+                    null,
+                    mockDateValuesSourceConfig("number", true, resolution)
+                )
+            );
+            assertNull(
+                MinAggregator.getPointReaderOrNull(
+                    mockSearchContext(null),
+                    mockAggregator(),
+                    mockDateValuesSourceConfig("number", true, resolution)
+                )
+            );
+            assertNull(
+                MinAggregator.getPointReaderOrNull(
+                    mockSearchContext(null),
+                    null,
+                    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(
             MinAggregator.getPointReaderOrNull(
                 mockSearchContext(new MatchAllDocsQuery()),
                 null,
-                mockDateValuesSourceConfig("number", true)
-            )
+                mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.MILLISECONDS)
+            ).apply(scratch), equalTo(expected.toEpochMilli())
         );
-        assertNull(
+        LongPoint.encodeDimension(DateFieldMapper.Resolution.NANOSECONDS.convert(expected), scratch, 0);
+        assertThat(
             MinAggregator.getPointReaderOrNull(
                 mockSearchContext(new MatchAllDocsQuery()),
-                mockAggregator(),
-                mockDateValuesSourceConfig("number", true)
-            )
-        );
-        assertNull(
-            MinAggregator.getPointReaderOrNull(
-                mockSearchContext(new TermQuery(new Term("foo", "bar"))),
                 null,
-                mockDateValuesSourceConfig("number", true)
-            )
-        );
-        assertNull(
-            MinAggregator.getPointReaderOrNull(
-                mockSearchContext(null),
-                mockAggregator(),
-                mockDateValuesSourceConfig("number", true)
-            )
-        );
-        assertNull(
-            MinAggregator.getPointReaderOrNull(
-                mockSearchContext(null),
-                null,
-                mockDateValuesSourceConfig("number", false)
-            )
+                mockDateValuesSourceConfig("number", true, DateFieldMapper.Resolution.NANOSECONDS)
+            ).apply(scratch), equalTo(expected.toEpochMilli())
         );
+
     }
 
     public void testMinShortcutRandom() throws Exception {
@@ -799,21 +817,6 @@ public void testMinShortcutRandom() throws Exception {
             (v) -> DoublePoint.decodeDimension(v, 0));
     }
 
-    private void testMinCase(IndexSearcher searcher,
-                                AggregationBuilder aggregationBuilder,
-                                MappedFieldType ft,
-                                DoubleConsumer testResult) throws IOException {
-        Collection<Query> queries = Arrays.asList(new MatchAllDocsQuery(), new DocValuesFieldExistsQuery(ft.name()));
-        for (Query query : queries) {
-            MinAggregator aggregator = createAggregator(query, aggregationBuilder, searcher, createIndexSettings(), ft);
-            aggregator.preCollection();
-            searcher.search(new MatchAllDocsQuery(), aggregator);
-            aggregator.postCollection();
-            InternalMin result = (InternalMin) aggregator.buildAggregation(0L);
-            testResult.accept(result.getValue());
-        }
-    }
-
     private void testMinShortcutCase(Supplier<Number> randomNumber,
                                         Function<Number, Field> pointFieldFunc,
                                         Function<byte[], Number> pointConvertFunc) throws IOException {
@@ -889,12 +892,17 @@ private ValuesSourceConfig<ValuesSource.Numeric> mockNumericValuesSourceConfig(S
         return config;
     }
 
-    private ValuesSourceConfig<ValuesSource.Numeric> mockDateValuesSourceConfig(String fieldName, boolean indexed) {
+    private ValuesSourceConfig<ValuesSource.Numeric> mockDateValuesSourceConfig(String fieldName, boolean indexed,
+            DateFieldMapper.Resolution resolution) {
         ValuesSourceConfig<ValuesSource.Numeric> config = mock(ValuesSourceConfig.class);
-        MappedFieldType ft = new DateFieldMapper.Builder(fieldName).fieldType();
-        ft.setName(fieldName);
-        ft.setIndexOptions(indexed ? IndexOptions.DOCS : IndexOptions.NONE);
-        ft.freeze();
+        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;
     }