Handle after key with formatted date histogram + address reviews

jimczi · jimczi · commit 2051507d65ce · 2018-01-22T17:20:53.000+01:00
diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/search.aggregation/230_composite.yml
@@ -241,7 +241,7 @@ setup:
 "Composite aggregation with format":
   - skip:
       version: " - 6.99.99"
-      reason:  this uses a new option (format) added 7.0.0
+      reason:  this uses a new option (format) added in 7.0.0
 
   - do:
       search:
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesComparator.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesComparator.java
@@ -56,7 +56,7 @@ final class CompositeValuesComparator {
                 if (vs.isFloatingPoint()) {
                     arrays[i] = CompositeValuesSource.wrapDouble(vs, size, reverseMul);
                 } else {
-                    arrays[i] = CompositeValuesSource.wrapLong(vs, size, reverseMul);
+                    arrays[i] = CompositeValuesSource.wrapLong(vs, sources[i].format(), size, reverseMul);
                 }
             }
         }
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSource.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeValuesSource.java
@@ -23,8 +23,10 @@
 import org.apache.lucene.index.SortedSetDocValues;
 import org.apache.lucene.search.LeafCollector;
 import org.apache.lucene.util.BytesRef;
+import org.elasticsearch.common.joda.FormatDateTimeFormatter;
 import org.elasticsearch.index.fielddata.SortedBinaryDocValues;
 import org.elasticsearch.index.fielddata.SortedNumericDoubleValues;
+import org.elasticsearch.search.DocValueFormat;
 import org.elasticsearch.search.aggregations.support.ValuesSource;
 import org.elasticsearch.search.sort.SortOrder;
 
@@ -96,8 +98,9 @@ interface Collector {
     /**
      * Creates a {@link CompositeValuesSource} that generates long values.
      */
-    static CompositeValuesSource<ValuesSource.Numeric, Long> wrapLong(ValuesSource.Numeric vs, int size, int reverseMul) {
-        return new LongValuesSource(vs, size, reverseMul);
+    static CompositeValuesSource<ValuesSource.Numeric, Long> wrapLong(ValuesSource.Numeric vs, DocValueFormat format,
+                                                                      int size, int reverseMul) {
+        return new LongValuesSource(vs, format, size, reverseMul);
     }
 
     /**
@@ -273,9 +276,12 @@ Collector getLeafCollector(LeafReaderContext context, Collector next) throws IOE
      */
     private static class LongValuesSource extends CompositeValuesSource<ValuesSource.Numeric, Long> {
         private final long[] values;
+        // handles "format" for date histogram source
+        private final DocValueFormat format;
 
-        LongValuesSource(ValuesSource.Numeric vs, int size, int reverseMul) {
+        LongValuesSource(ValuesSource.Numeric vs, DocValueFormat format, int size, int reverseMul) {
             super(vs, size, reverseMul);
+            this.format = format;
             this.values = new long[size];
         }
 
@@ -304,7 +310,11 @@ void setTop(Comparable<?> value) {
             if (value instanceof Number) {
                 topValue = ((Number) value).longValue();
             } else {
-                topValue = Long.parseLong(value.toString());
+                // for date histogram source with "format", the after value is formatted
+                // as a string so we need to retrieve the original value in milliseconds.
+                topValue = format.parseLong(value.toString(), false, () -> {
+                    throw new IllegalArgumentException("now() is not supported in [after] key");
+                });
             }
         }
 
diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java b/server/src/main/java/org/elasticsearch/search/aggregations/bucket/composite/InternalComposite.java
@@ -37,6 +37,7 @@
 import java.util.AbstractSet;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
@@ -68,11 +69,11 @@ public InternalComposite(StreamInput in) throws IOException {
         super(in);
         this.size = in.readVInt();
         this.sourceNames = in.readList(StreamInput::readString);
-        if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
-            this.formats = in.readNamedWriteableList(DocValueFormat.class);
-        } else {
-            this.formats = new ArrayList<>(sourceNames.size());
-            for (int i = 0; i < sourceNames.size(); i++) {
+        this.formats = new ArrayList<>(sourceNames.size());
+        for (int i = 0; i < sourceNames.size(); i++) {
+            if (in.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
+                formats.add(in.readNamedWriteable(DocValueFormat.class));
+            } else {
                 formats.add(DocValueFormat.RAW);
             }
         }
@@ -85,7 +86,9 @@ protected void doWriteTo(StreamOutput out) throws IOException {
         out.writeVInt(size);
         out.writeStringList(sourceNames);
         if (out.getVersion().onOrAfter(Version.V_7_0_0_alpha1)) {
-            out.writeNamedWriteableList(formats);
+            for (DocValueFormat format : formats) {
+                out.writeNamedWriteable(format);
+            }
         }
         out.writeIntArray(reverseMuls);
         out.writeList(buckets);
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/CompositeAggregatorTests.java
@@ -39,6 +39,7 @@
 import org.apache.lucene.util.LuceneTestCase;
 import org.apache.lucene.util.NumericUtils;
 import org.apache.lucene.util.TestUtil;
+import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.index.Index;
 import org.elasticsearch.index.IndexSettings;
@@ -68,6 +69,9 @@
 import java.util.function.Consumer;
 import java.util.function.Supplier;
 
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.instanceOf;
+
 public class CompositeAggregatorTests extends AggregatorTestCase {
     private static MappedFieldType[] FIELD_TYPES;
 
@@ -800,7 +804,7 @@ public void testWithDateHistogramAndFormat() throws IOException {
                     .dateHistogramInterval(DateHistogramInterval.days(1))
                     .format("yyyy-MM-dd");
                 return new CompositeAggregationBuilder("name", Collections.singletonList(histo))
-                    .aggregateAfter(createAfterKey("date", 1474329600000L));
+                    .aggregateAfter(createAfterKey("date", "2016-09-20"));
 
             }, (result) -> {
                 assertEquals(2, result.getBuckets().size());
@@ -812,6 +816,38 @@ public void testWithDateHistogramAndFormat() throws IOException {
         );
     }
 
+    public void testThatDateHistogramFailsFormatAfter() throws IOException {
+        ElasticsearchParseException exc = expectThrows(ElasticsearchParseException.class,
+            () -> testSearchCase(new MatchAllDocsQuery(), null, Collections.emptyList(),
+                () -> {
+                    DateHistogramValuesSourceBuilder histo = new DateHistogramValuesSourceBuilder("date")
+                        .field("date")
+                        .dateHistogramInterval(DateHistogramInterval.days(1))
+                        .format("yyyy-MM-dd");
+                    return new CompositeAggregationBuilder("name", Collections.singletonList(histo))
+                        .aggregateAfter(createAfterKey("date", "now"));
+                },
+                (result) -> {}
+        ));
+        assertThat(exc.getCause(), instanceOf(IllegalArgumentException.class));
+        assertThat(exc.getCause().getMessage(), containsString("now() is not supported in [after] key"));
+
+        exc = expectThrows(ElasticsearchParseException.class,
+            () -> testSearchCase(new MatchAllDocsQuery(), null, Collections.emptyList(),
+                () -> {
+                    DateHistogramValuesSourceBuilder histo = new DateHistogramValuesSourceBuilder("date")
+                        .field("date")
+                        .dateHistogramInterval(DateHistogramInterval.days(1))
+                        .format("yyyy-MM-dd");
+                    return new CompositeAggregationBuilder("name", Collections.singletonList(histo))
+                        .aggregateAfter(createAfterKey("date", "1474329600000"));
+                },
+                (result) -> {}
+            ));
+        assertThat(exc.getCause(), instanceOf(IllegalArgumentException.class));
+        assertThat(exc.getCause().getMessage(), containsString("Parse failure"));
+    }
+
     public void testWithDateHistogramAndTimeZone() throws IOException {
         final List<Map<String, List<Object>>> dataset = new ArrayList<>();
         dataset.addAll(
diff --git a/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/InternalCompositeTests.java b/server/src/test/java/org/elasticsearch/search/aggregations/bucket/composite/InternalCompositeTests.java
@@ -21,13 +21,15 @@
 
 import org.apache.lucene.util.BytesRef;
 import org.elasticsearch.common.io.stream.Writeable;
+import org.elasticsearch.common.joda.Joda;
 import org.elasticsearch.common.util.BigArrays;
 import org.elasticsearch.search.DocValueFormat;
 import org.elasticsearch.search.aggregations.InternalAggregation;
 import org.elasticsearch.search.aggregations.InternalAggregations;
 import org.elasticsearch.search.aggregations.ParsedAggregation;
 import org.elasticsearch.search.aggregations.pipeline.PipelineAggregator;
 import org.elasticsearch.test.InternalMultiBucketAggregationTestCase;
+import org.joda.time.DateTimeZone;
 import org.junit.After;
 
 import java.io.IOException;
@@ -52,6 +54,20 @@ public class InternalCompositeTests extends InternalMultiBucketAggregationTestCa
     private int[] types;
     private int size;
 
+    private static DocValueFormat randomDocValueFormat(boolean isLong) {
+        if (isLong) {
+            // we use specific format only for date histogram on a long/date field
+            if (randomBoolean()) {
+                return new DocValueFormat.DateTime(Joda.forPattern("epoch_second"), DateTimeZone.forOffsetHours(1));
+            } else {
+                return DocValueFormat.RAW;
+            }
+        } else {
+            // and the raw format for the other types
+            return DocValueFormat.RAW;
+        }
+    }
+
     @Override
     public void setUp() throws Exception {
         super.setUp();
@@ -63,17 +79,18 @@ public void setUp() throws Exception {
         types = new int[numFields];
         for (int i = 0; i < numFields; i++) {
             sourceNames.add("field_" + i);
-            formats.add(DocValueFormat.RAW);
             reverseMuls[i] = randomBoolean() ? 1 : -1;
-            types[i] = randomIntBetween(0, 2);
+            int type = randomIntBetween(0, 2);
+            types[i] = type;
+            formats.add(randomDocValueFormat(type == 0));
         }
     }
 
     @Override
     @After
     public void tearDown() throws Exception {
         super.tearDown();
-        sourceNames= null;
+        sourceNames = null;
         formats = null;
         reverseMuls = null;
         types = null;

Original file line number	Diff line number	Diff line change
`@@ -56,7 +56,7 @@ final class CompositeValuesComparator {`
`56`	`56`	`if (vs.isFloatingPoint()) {`
`57`	`57`	`arrays[i] = CompositeValuesSource.wrapDouble(vs, size, reverseMul);`
`58`	`58`	`} else {`
`59`		`- arrays[i] = CompositeValuesSource.wrapLong(vs, size, reverseMul);`
	`59`	`+ arrays[i] = CompositeValuesSource.wrapLong(vs, sources[i].format(), size, reverseMul);`
`60`	`60`	`}`
`61`	`61`	`}`
`62`	`62`	`}`