Force selection of calendar or fixed intervals in date histo agg

client/rest-high-level/src/main/java/org/elasticsearch/client/rollup/job/config/DateHistogramGroupConfig.java

@@ -22,6 +22,7 @@
 import org.elasticsearch.client.ValidationException;
 import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.ParseField;
+import org.elasticsearch.common.unit.TimeValue;
 import org.elasticsearch.common.xcontent.ConstructingObjectParser;
 import org.elasticsearch.common.xcontent.ToXContentObject;
 import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -30,8 +31,11 @@
 import org.joda.time.DateTimeZone;
 
 import java.io.IOException;
+import java.util.Collections;
+import java.util.HashSet;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.Set;
 
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.constructorArg;
 import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;
@@ -59,14 +63,63 @@ public class DateHistogramGroupConfig implements Validatable, ToXContentObject {
     private static final String TIME_ZONE = "time_zone";
     private static final String DELAY = "delay";
     private static final String DEFAULT_TIMEZONE = "UTC";
+    private static final String CALENDAR_INTERVAL = "calendar_interval";
+    private static final String FIXED_INTERVAL = "fixed_interval";
+
+    // From DateHistogramAggregationBuilder in core, transplanted and modified to a set
+    // so we don't need to import a dependency on the class
+    private static final Set<String> DATE_FIELD_UNITS;
+    static {
+        Set<String> dateFieldUnits = new HashSet<>();
+        dateFieldUnits.add("year");
+        dateFieldUnits.add("1y");
+        dateFieldUnits.add("quarter");
+        dateFieldUnits.add("1q");
+        dateFieldUnits.add("month");
+        dateFieldUnits.add("1M");
+        dateFieldUnits.add("week");
+        dateFieldUnits.add("1w");
+        dateFieldUnits.add("day");
+        dateFieldUnits.add("1d");
+        dateFieldUnits.add("hour");
+        dateFieldUnits.add("1h");
+        dateFieldUnits.add("minute");
+        dateFieldUnits.add("1m");
+        dateFieldUnits.add("second");
+        dateFieldUnits.add("1s");
+        DATE_FIELD_UNITS = Collections.unmodifiableSet(dateFieldUnits);
+    }
 
     private static final ConstructingObjectParser<DateHistogramGroupConfig, Void> PARSER;
     static {
-        PARSER = new ConstructingObjectParser<>(NAME, true, a ->
-            new DateHistogramGroupConfig((String) a[0], (DateHistogramInterval) a[1], (DateHistogramInterval) a[2], (String) a[3]));
+        PARSER = new ConstructingObjectParser<>(NAME, true, a -> {
+            DateHistogramInterval oldInterval = (DateHistogramInterval) a[1];
+            DateHistogramInterval calendarInterval = (DateHistogramInterval) a[2];
+            DateHistogramInterval fixedInterval = (DateHistogramInterval) a[3];
+
+            if (oldInterval != null) {
+                if  (calendarInterval != null || fixedInterval != null) {
+                    throw new IllegalArgumentException("Cannot use [interval] with [fixed_interval] or [calendar_interval] " +
+                        "configuration options.");
+                }
+                return new DateHistogramGroupConfig((String) a[0], oldInterval, (DateHistogramInterval) a[4], (String) a[5]);
+            } else if (calendarInterval != null && fixedInterval == null) {
+                return new CalendarInterval((String) a[0], calendarInterval, (DateHistogramInterval) a[4], (String) a[5]);
+            } else if (calendarInterval == null && fixedInterval != null) {
+                return new FixedInterval((String) a[0], fixedInterval, (DateHistogramInterval) a[4], (String) a[5]);
+            } else if (calendarInterval != null && fixedInterval != null) {
+                throw new IllegalArgumentException("Cannot set both [fixed_interval] and [calendar_interval] at the same time");
+            } else {
+                throw new IllegalArgumentException("An interval is required.  Use [fixed_interval] or [calendar_interval].");
+            }
+        });
         PARSER.declareString(constructorArg(), new ParseField(FIELD));
-        PARSER.declareField(constructorArg(), p -> new DateHistogramInterval(p.text()), new ParseField(INTERVAL), ValueType.STRING);
-        PARSER.declareField(optionalConstructorArg(), p -> new DateHistogramInterval(p.text()), new ParseField(DELAY), ValueType.STRING);
+        PARSER.declareField(optionalConstructorArg(), p -> new DateHistogramInterval(p.text()), new ParseField(INTERVAL), ValueType.STRING);
+        PARSER.declareField(optionalConstructorArg(), p -> new DateHistogramInterval(p.text()),
+            new ParseField(CALENDAR_INTERVAL), ValueType.STRING);
+        PARSER.declareField(optionalConstructorArg(), p -> new DateHistogramInterval(p.text()),
+            new ParseField(FIXED_INTERVAL), ValueType.STRING);
+        PARSER.declareField(optionalConstructorArg(),  p -> new DateHistogramInterval(p.text()), new ParseField(DELAY), ValueType.STRING);
         PARSER.declareString(optionalConstructorArg(), new ParseField(TIME_ZONE));
     }
 
@@ -75,27 +128,81 @@ public class DateHistogramGroupConfig implements Validatable, ToXContentObject {
     private final DateHistogramInterval delay;
     private final String timeZone;
 
+    /**
+     * FixedInterval is a {@link DateHistogramGroupConfig} that uses a fixed time interval for rolling up data.
+     * The fixed time interval is one or multiples of SI units and has no calendar-awareness (e.g. doesn't account
+     * for leap corrections, does not have variable length months, etc).
+     *
+     * For calendar-aware rollups, use {@link CalendarInterval}
+     */
+    public static class FixedInterval extends DateHistogramGroupConfig {
+        public FixedInterval(String field, DateHistogramInterval interval) {
+            this(field, interval, null, null);
+        }
+
+        public FixedInterval(String field, DateHistogramInterval interval, DateHistogramInterval delay, String timeZone) {
+            super(field, interval, delay, timeZone);
+            // validate fixed time
+            TimeValue.parseTimeValue(interval.toString(), NAME + ".FixedInterval");
+        }
+    }
+
+    /**
+     * CalendarInterval is a {@link DateHistogramGroupConfig} that uses calendar-aware intervals for rolling up data.
+     * Calendar time intervals understand leap corrections and contextual differences in certain calendar units (e.g.
+     * months are variable length depending on the month).  Calendar units are only available in singular quantities:
+     * 1s, 1m, 1h, 1d, 1w, 1q, 1M, 1y
+     *
+     * For fixed time rollups, use {@link FixedInterval}
+     */
+    public static class CalendarInterval extends DateHistogramGroupConfig {
+        public CalendarInterval(String field, DateHistogramInterval interval) {
+            this(field, interval, null, null);
+
+        }
+
+        public CalendarInterval(String field, DateHistogramInterval interval, DateHistogramInterval delay, String timeZone) {
+            super(field, interval, delay, timeZone);
+            if (DATE_FIELD_UNITS.contains(interval.toString()) == false) {
+                throw new IllegalArgumentException("The supplied interval [" + interval +"] could not be parsed " +
+                    "as a calendar interval.");
+            }
+        }
+
+    }
+
     /**
      * Create a new {@link DateHistogramGroupConfig} using the given field and interval parameters.
+     *
+     * @deprecated Build a DateHistoConfig using {@link DateHistogramGroupConfig.CalendarInterval}
+     * or {@link DateHistogramGroupConfig.FixedInterval} instead
+     *
+     * @since 7.2.0
      */
+    @Deprecated
     public DateHistogramGroupConfig(final String field, final DateHistogramInterval interval) {
         this(field, interval, null, null);
     }
 
     /**
      * Create a new {@link DateHistogramGroupConfig} using the given configuration parameters.
      * <p>
-     * The {@code field} and {@code interval} are required to compute the date histogram for the rolled up documents.
-     * The {@code delay} is optional and can be set to {@code null}. It defines how long to wait before rolling up new documents.
-     * The {@code timeZone} is optional and can be set to {@code null}. When configured, the time zone value  is resolved using
-     * ({@link DateTimeZone#forID(String)} and must match a time zone identifier provided by the Joda Time library.
+     *     The {@code field} and {@code interval} are required to compute the date histogram for the rolled up documents.
+     *     The {@code delay} is optional and can be set to {@code null}. It defines how long to wait before rolling up new documents.
+     *     The {@code timeZone} is optional and can be set to {@code null}. When configured, the time zone value  is resolved using
+     *     ({@link DateTimeZone#forID(String)} and must match a time zone identifier provided by the Joda Time library.
      * </p>
-     *
-     * @param field    the name of the date field to use for the date histogram (required)
+     * @param field the name of the date field to use for the date histogram (required)
      * @param interval the interval to use for the date histogram (required)
-     * @param delay    the time delay (optional)
+     * @param delay the time delay (optional)
      * @param timeZone the id of time zone to use to calculate the date histogram (optional). When {@code null}, the UTC timezone is used.
+     *
+     * @deprecated Build a DateHistoConfig using {@link DateHistogramGroupConfig.CalendarInterval}
+     * or {@link DateHistogramGroupConfig.FixedInterval} instead
+     *
+     * @since 7.2.0
      */
+    @Deprecated
     public DateHistogramGroupConfig(final String field,
                                     final DateHistogramInterval interval,
                                     final @Nullable DateHistogramInterval delay,
@@ -153,7 +260,13 @@ public String getTimeZone() {
     public XContentBuilder toXContent(final XContentBuilder builder, final Params params) throws IOException {
         builder.startObject();
         {
-            builder.field(INTERVAL, interval.toString());
+            if (this.getClass().equals(CalendarInterval.class)) {
+                builder.field(CALENDAR_INTERVAL, interval.toString());
+            } else if (this.getClass().equals(FixedInterval.class)) {
+                builder.field(FIXED_INTERVAL, interval.toString());
+            } else {
+                builder.field(INTERVAL, interval.toString());
+            }
             builder.field(FIELD, field);
             if (delay != null) {
                 builder.field(DELAY, delay.toString());

client/rest-high-level/src/test/java/org/elasticsearch/client/RollupIT.java

@@ -152,7 +152,7 @@ public int indexDocs() throws Exception {
 
 
     public void testDeleteRollupJob() throws Exception {
-        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig("date", DateHistogramInterval.DAY));
+        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig.CalendarInterval("date", DateHistogramInterval.DAY));
         final List<MetricConfig> metrics = Collections.singletonList(new MetricConfig("value", SUPPORTED_METRICS));
         final TimeValue timeout = TimeValue.timeValueSeconds(randomIntBetween(30, 600));
         PutRollupJobRequest putRollupJobRequest =
@@ -174,7 +174,7 @@ public void testDeleteMissingRollupJob() {
 
     public void testPutStartAndGetRollupJob() throws Exception {
         // TODO expand this to also test with histogram and terms?
-        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig("date", DateHistogramInterval.DAY));
+        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig.CalendarInterval("date", DateHistogramInterval.DAY));
         final List<MetricConfig> metrics = Collections.singletonList(new MetricConfig("value", SUPPORTED_METRICS));
         final TimeValue timeout = TimeValue.timeValueSeconds(randomIntBetween(30, 600));
 
@@ -333,7 +333,7 @@ public void testGetRollupCaps() throws Exception {
         final String cron = "*/1 * * * * ?";
         final int pageSize = randomIntBetween(numDocs, numDocs * 10);
         // TODO expand this to also test with histogram and terms?
-        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig("date", DateHistogramInterval.DAY));
+        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig.CalendarInterval("date", DateHistogramInterval.DAY));
         final List<MetricConfig> metrics = Collections.singletonList(new MetricConfig("value", SUPPORTED_METRICS));
         final TimeValue timeout = TimeValue.timeValueSeconds(randomIntBetween(30, 600));
 
@@ -377,7 +377,7 @@ public void testGetRollupCaps() throws Exception {
                 case "delay":
                     assertThat(entry.getValue(), equalTo("foo"));
                     break;
-                case "interval":
+                case "calendar_interval":
                     assertThat(entry.getValue(), equalTo("1d"));
                     break;
                 case "time_zone":
@@ -445,7 +445,7 @@ public void testGetRollupIndexCaps() throws Exception {
         final String cron = "*/1 * * * * ?";
         final int pageSize = randomIntBetween(numDocs, numDocs * 10);
         // TODO expand this to also test with histogram and terms?
-        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig("date", DateHistogramInterval.DAY));
+        final GroupConfig groups = new GroupConfig(new DateHistogramGroupConfig.CalendarInterval("date", DateHistogramInterval.DAY));
         final List<MetricConfig> metrics = Collections.singletonList(new MetricConfig("value", SUPPORTED_METRICS));
         final TimeValue timeout = TimeValue.timeValueSeconds(randomIntBetween(30, 600));
 
@@ -489,7 +489,7 @@ public void testGetRollupIndexCaps() throws Exception {
                 case "delay":
                     assertThat(entry.getValue(), equalTo("foo"));
                     break;
-                case "interval":
+                case "calendar_interval":
                     assertThat(entry.getValue(), equalTo("1d"));
                     break;
                 case "time_zone":

client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java

@@ -399,8 +399,8 @@ public void onFailure(Exception e) {
     public void testGetRollupCaps() throws Exception {
         RestHighLevelClient client = highLevelClient();
 
-        DateHistogramGroupConfig dateHistogram =
-            new DateHistogramGroupConfig("timestamp", DateHistogramInterval.HOUR, new DateHistogramInterval("7d"), "UTC"); // <1>
+        DateHistogramGroupConfig dateHistogram = new DateHistogramGroupConfig.FixedInterval(
+            "timestamp", DateHistogramInterval.HOUR, new DateHistogramInterval("7d"), "UTC"); // <1>
         TermsGroupConfig terms = new TermsGroupConfig("hostname", "datacenter");
         HistogramGroupConfig histogram = new HistogramGroupConfig(5L, "load", "net_in", "net_out");
         GroupConfig groups = new GroupConfig(dateHistogram, histogram, terms);
@@ -473,7 +473,8 @@ public void testGetRollupCaps() throws Exception {
         // item represents a different aggregation that can be run against the "timestamp"
         // field, and any additional details specific to that agg (interval, etc)
         List<Map<String, Object>> timestampCaps = fieldCaps.get("timestamp").getAggs();
-        assert timestampCaps.get(0).toString().equals("{agg=date_histogram, delay=7d, interval=1h, time_zone=UTC}");
+        logger.error(timestampCaps.get(0).toString());
+        assert timestampCaps.get(0).toString().equals("{agg=date_histogram, fixed_interval=1h, delay=7d, time_zone=UTC}");
 
         // In contrast to the timestamp field, the temperature field has multiple aggs configured
         List<Map<String, Object>> temperatureCaps = fieldCaps.get("temperature").getAggs();
@@ -515,8 +516,8 @@ public void onFailure(Exception e) {
     public void testGetRollupIndexCaps() throws Exception {
         RestHighLevelClient client = highLevelClient();
 
-        DateHistogramGroupConfig dateHistogram =
-            new DateHistogramGroupConfig("timestamp", DateHistogramInterval.HOUR, new DateHistogramInterval("7d"), "UTC"); // <1>
+        DateHistogramGroupConfig dateHistogram = new DateHistogramGroupConfig.FixedInterval(
+            "timestamp", DateHistogramInterval.HOUR, new DateHistogramInterval("7d"), "UTC"); // <1>
         TermsGroupConfig terms = new TermsGroupConfig("hostname", "datacenter");
         HistogramGroupConfig histogram = new HistogramGroupConfig(5L, "load", "net_in", "net_out");
         GroupConfig groups = new GroupConfig(dateHistogram, histogram, terms);
@@ -587,7 +588,8 @@ public void testGetRollupIndexCaps() throws Exception {
         // item represents a different aggregation that can be run against the "timestamp"
         // field, and any additional details specific to that agg (interval, etc)
         List<Map<String, Object>> timestampCaps = fieldCaps.get("timestamp").getAggs();
-        assert timestampCaps.get(0).toString().equals("{agg=date_histogram, delay=7d, interval=1h, time_zone=UTC}");
+        logger.error(timestampCaps.get(0).toString());
+        assert timestampCaps.get(0).toString().equals("{agg=date_histogram, fixed_interval=1h, delay=7d, time_zone=UTC}");
 
         // In contrast to the timestamp field, the temperature field has multiple aggs configured
         List<Map<String, Object>> temperatureCaps = fieldCaps.get("temperature").getAggs();

client/rest-high-level/src/test/java/org/elasticsearch/client/ml/datafeed/DatafeedConfigTests.java

@@ -28,6 +28,7 @@
 import org.elasticsearch.index.query.QueryBuilders;
 import org.elasticsearch.search.aggregations.AggregationBuilders;
 import org.elasticsearch.search.aggregations.AggregatorFactories;
+import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
 import org.elasticsearch.search.aggregations.metrics.MaxAggregationBuilder;
 import org.elasticsearch.search.builder.SearchSourceBuilder.ScriptField;
 import org.elasticsearch.test.AbstractXContentTestCase;
@@ -79,7 +80,7 @@ public static DatafeedConfig.Builder createRandomBuilder() {
             aggHistogramInterval = aggHistogramInterval <= 0 ? 1 : aggHistogramInterval;
             MaxAggregationBuilder maxTime = AggregationBuilders.max("time").field("time");
             aggs.addAggregator(AggregationBuilders.dateHistogram("buckets")
-                .interval(aggHistogramInterval).subAggregation(maxTime).field("time"));
+                .fixedInterval(new DateHistogramInterval(aggHistogramInterval + "ms")).subAggregation(maxTime).field("time"));
             try {
                 builder.setAggregations(aggs);
             } catch (IOException e) {

client/rest-high-level/src/test/java/org/elasticsearch/client/rollup/GetRollupJobResponseTests.java

@@ -44,7 +44,7 @@ public void testFromXContent() throws IOException {
                 this::createTestInstance,
                 this::toXContent,
                 GetRollupJobResponse::fromXContent)
-                .supportsUnknownFields(true)
+                .supportsUnknownFields(false)
                 .randomFieldsExcludeFilter(field ->
                         field.endsWith("status.current_position"))
                 .test();

client/rest-high-level/src/test/java/org/elasticsearch/client/rollup/PutRollupJobRequestTests.java

@@ -49,7 +49,7 @@ protected PutRollupJobRequest doParseInstance(final XContentParser parser) throw
 
     @Override
     protected boolean supportsUnknownFields() {
-        return true;
+        return false;
     }
 
     public void testRequireConfiguration() {