Skip to content

Commit

Permalink
Force selection of calendar or fixed intervals in date histo agg (ela…
Browse files Browse the repository at this point in the history 
…stic#33727)

The date_histogram accepts an interval which can be either a calendar 
interval (DST-aware, leap seconds, arbitrary length of months, etc) or 
fixed interval (strict multiples of SI units). Unfortunately this is inferred
by first trying to parse as a calendar interval, then falling back to fixed
if that fails.

This leads to confusing arrangement where `1d` == calendar, but 
`2d` == fixed.  And if you want a day of fixed time, you have to 
specify `24h` (e.g. the next smallest unit).  This arrangement is very
error-prone for users.

This PR adds `calendar_interval` and `fixed_interval` parameters to any
code that uses intervals (date_histogram, rollup, composite, datafeed, etc).
Calendar only accepts calendar intervals, fixed accepts any combination of
units (meaning `1d` can be used to specify `24h` in fixed time), and both
are mutually exclusive.  

The old interval behavior is deprecated and will throw a deprecation warning.
It is also mutually exclusive with the two new parameters. In the future the 
old dual-purpose interval will be removed.

The change applies to both REST and java clients.
  • Loading branch information
@polyfractal
polyfractal authored and Gurkan Kaymak committed May 27, 2019
1 parent 4ded96a commit 041f4d6
Show file tree
Hide file tree
Showing 108 changed files with 3,268 additions and 670 deletions.
137 changes: 125 additions & 12 deletions ...el/src/main/java/org/elasticsearch/client/rollup/job/config/DateHistogramGroupConfig.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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));
}

Expand All @@ -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,
Expand Down Expand Up @@ -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());
Expand Down
12 changes: 6 additions & 6 deletions client/rest-high-level/src/test/java/org/elasticsearch/client/RollupIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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));

Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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":
Expand Down Expand Up @@ -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));

Expand Down Expand Up @@ -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":
Expand Down
14 changes: 8 additions & 6 deletions ...igh-level/src/test/java/org/elasticsearch/client/documentation/RollupDocumentationIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand Down
3 changes: 2 additions & 1 deletion ...st-high-level/src/test/java/org/elasticsearch/client/ml/datafeed/DatafeedConfigTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
2 changes: 1 addition & 1 deletion ...t-high-level/src/test/java/org/elasticsearch/client/rollup/GetRollupJobResponseTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion ...st-high-level/src/test/java/org/elasticsearch/client/rollup/PutRollupJobRequestTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ protected PutRollupJobRequest doParseInstance(final XContentParser parser) throw

@Override
protected boolean supportsUnknownFields() {
return true;
return false;
}

public void testRequireConfiguration() {
Expand Down
Loading

0 comments on commit 041f4d6

Please sign in to comment.