Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML-DataFrame] add support for fixed_interval, calendar_interval, remove interval #42427

Merged
merged 14 commits into from
May 24, 2019
Merged

[ML-DataFrame] add support for fixed_interval, calendar_interval, remove interval #42427

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
11d0717
add support for fixed_interval, calendar_interval, remove interval
May 22, 2019
5eb3528
adapt HLRC
May 23, 2019
45fd6fd
checkstyle
May 23, 2019
dddfdf3
add a hlrc to server test
May 23, 2019
5fa114b
adapt yml test
May 23, 2019
a304e26
improve naming and doc
May 23, 2019
c4ccab5
improve interface and add test code for hlrc to server
May 23, 2019
e707c2a
address review comments
May 23, 2019
2647c8e
repair merge conflict
May 23, 2019
c75f7f9
fix date patterns
May 24, 2019
27104d2
address review comments
May 24, 2019
61660c1
remove assert for warning
May 24, 2019
0016ef3
improve exception message
May 24, 2019
42a3981
use constants
May 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
231 changes: 164 additions & 67 deletions ...in/java/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@
package org.elasticsearch.client.dataframe.transforms.pivot;

import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.xcontent.ConstructingObjectParser;
import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.ToXContentFragment;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand All @@ -31,7 +31,11 @@
import java.io.IOException;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashSet;
import java.util.Objects;
import java.util.Set;

import static org.elasticsearch.common.xcontent.ConstructingObjectParser.optionalConstructorArg;

Expand All @@ -43,32 +47,164 @@ public class DateHistogramGroupSource extends SingleGroupSource implements ToXCo
private static final ParseField TIME_ZONE = new ParseField("time_zone");
private static final ParseField FORMAT = new ParseField("format");

// 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 = Collections.unmodifiableSet(new HashSet<>(Arrays.asList(
"year",
"1y",
"quarter",
"1q",
"month",
"1M",
"week",
"1w",
"day",
"1d",
"hour",
"1h",
"minute",
"1m",
"second",
"1s")));

/**
* Interval can be specified in 2 ways:
*
* fixed_interval fixed intervals like 1h, 1m, 1d
* calendar_interval calendar aware intervals like 1M, 1Y, ...
*
* Note: data frames do not support the deprecated interval option
*/
public interface Interval extends ToXContentFragment {
String getName();
DateHistogramInterval getInterval();
}

public static class FixedInterval implements Interval {
private static final String NAME = "fixed_interval";
private final DateHistogramInterval interval;

public FixedInterval(DateHistogramInterval interval) {
this.interval = interval;
}

@Override
public String getName() {
return NAME;
}

@Override
public DateHistogramInterval getInterval() {
return interval;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field(NAME);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nit: I've just realized it can be compressed to one line using:
return builder.field(NAME, interval);

This call uses the following method, seems this is exactly what is needed here:
public XContentBuilder field(String name, ToXContent value) throws IOException;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way it's done at the moment the params are preserved. It doesn't matter now but in the future it could make a difference.

interval.toXContent(builder, params);
return builder;
}

@Override
public boolean equals(Object other) {
if (this == other) {
hendrikmuhs marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

if (other == null || getClass() != other.getClass()) {
return false;
}

final FixedInterval that = (FixedInterval) other;
return Objects.equals(this.interval, that.interval);
}

@Override
public int hashCode() {
return Objects.hash(interval);
}
}

public static class CalendarInterval implements Interval {
private static final String NAME = "calendar_interval";
private final DateHistogramInterval interval;

public CalendarInterval(DateHistogramInterval interval) {
this.interval = interval;
if (DATE_FIELD_UNITS.contains(interval.toString()) == false) {
przemekwitek marked this conversation as resolved.
Show resolved Hide resolved
throw new IllegalArgumentException("The supplied interval [" + interval + "] could not be parsed " +
"as a calendar interval.");
}
}

@Override
public String getName() {
return NAME;
}

@Override
public DateHistogramInterval getInterval() {
return interval;
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.field(NAME);
interval.toXContent(builder, params);
return builder;
}

@Override
public boolean equals(Object other) {
if (this == other) {
return true;
}

if (other == null || getClass() != other.getClass()) {
return false;
}

final CalendarInterval that = (CalendarInterval) other;
return Objects.equals(this.interval, that.interval);
}

@Override
public int hashCode() {
return Objects.hash(interval);
}
}

private static final ConstructingObjectParser<DateHistogramGroupSource, Void> PARSER =
new ConstructingObjectParser<>("date_histogram_group_source",
true,
(args) -> {
String field = (String)args[0];
long interval = 0;
DateHistogramInterval dateHistogramInterval = null;
if (args[1] instanceof Long) {
interval = (Long)args[1];
String fixedInterval = (String) args[1];
String calendarInterval = (String) args[2];

Interval interval = null;

if (fixedInterval != null && calendarInterval != null) {
hendrikmuhs marked this conversation as resolved.
Show resolved Hide resolved
throw new IllegalArgumentException("You must specify either fixed_interval or calendar_interval, found both");
} else if (fixedInterval != null) {
interval = new FixedInterval(new DateHistogramInterval(fixedInterval));
} else if (calendarInterval != null) {
interval = new CalendarInterval(new DateHistogramInterval(calendarInterval));
} else {
dateHistogramInterval = (DateHistogramInterval) args[1];
throw new IllegalArgumentException("You must specify either fixed_interval or calendar_interval, found none");
}
ZoneId zoneId = (ZoneId) args[2];
String format = (String) args[3];
return new DateHistogramGroupSource(field, interval, dateHistogramInterval, format, zoneId);

ZoneId zoneId = (ZoneId) args[3];
String format = (String) args[4];
return new DateHistogramGroupSource(field, interval, format, zoneId);
});

static {
PARSER.declareString(optionalConstructorArg(), FIELD);
PARSER.declareField(optionalConstructorArg(), p -> {
if (p.currentToken() == XContentParser.Token.VALUE_NUMBER) {
return p.longValue();
} else {
return new DateHistogramInterval(p.text());
}
}, HistogramGroupSource.INTERVAL, ObjectParser.ValueType.LONG);

PARSER.declareString(optionalConstructorArg(), new ParseField(FixedInterval.NAME));
PARSER.declareString(optionalConstructorArg(), new ParseField(CalendarInterval.NAME));

PARSER.declareField(optionalConstructorArg(), p -> {
if (p.currentToken() == XContentParser.Token.VALUE_STRING) {
return ZoneId.of(p.text());
Expand All @@ -84,15 +220,13 @@ public static DateHistogramGroupSource fromXContent(final XContentParser parser)
return PARSER.apply(parser, null);
}

private final long interval;
private final DateHistogramInterval dateHistogramInterval;
private final Interval interval;
private final String format;
private final ZoneId timeZone;

DateHistogramGroupSource(String field, long interval, DateHistogramInterval dateHistogramInterval, String format, ZoneId timeZone) {
DateHistogramGroupSource(String field, Interval interval, String format, ZoneId timeZone) {
super(field);
this.interval = interval;
this.dateHistogramInterval = dateHistogramInterval;
this.format = format;
this.timeZone = timeZone;
}
Expand All @@ -102,14 +236,10 @@ public Type getType() {
return Type.DATE_HISTOGRAM;
}

public long getInterval() {
public Interval getInterval() {
return interval;
}

public DateHistogramInterval getDateHistogramInterval() {
return dateHistogramInterval;
}

public String getFormat() {
return format;
}
Expand All @@ -124,11 +254,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
if (field != null) {
builder.field(FIELD.getPreferredName(), field);
}
if (dateHistogramInterval == null) {
builder.field(HistogramGroupSource.INTERVAL.getPreferredName(), interval);
} else {
builder.field(HistogramGroupSource.INTERVAL.getPreferredName(), dateHistogramInterval.toString());
}
interval.toXContent(builder, params);
if (timeZone != null) {
builder.field(TIME_ZONE.getPreferredName(), timeZone.toString());
}
Expand All @@ -152,15 +278,14 @@ public boolean equals(Object other) {
final DateHistogramGroupSource that = (DateHistogramGroupSource) other;

return Objects.equals(this.field, that.field) &&
przemekwitek marked this conversation as resolved.
Show resolved Hide resolved
Objects.equals(interval, that.interval) &&
Objects.equals(dateHistogramInterval, that.dateHistogramInterval) &&
Objects.equals(timeZone, that.timeZone) &&
Objects.equals(format, that.format);
Objects.equals(this.interval, that.interval) &&
Objects.equals(this.timeZone, that.timeZone) &&
Objects.equals(this.format, that.format);
}

@Override
public int hashCode() {
return Objects.hash(field, interval, dateHistogramInterval, timeZone, format);
return Objects.hash(field, interval, timeZone, format);
}

public static Builder builder() {
Expand All @@ -170,8 +295,7 @@ public static Builder builder() {
public static class Builder {

private String field;
private long interval = 0;
private DateHistogramInterval dateHistogramInterval;
private Interval interval;
private String format;
private ZoneId timeZone;

Expand All @@ -187,41 +311,14 @@ public Builder setField(String field) {

/**
* Set the interval for the DateHistogram grouping
* @param interval the time interval in milliseconds
* @param interval a fixed or calendar interval
* @return the {@link Builder} with the interval set.
*/
public Builder setInterval(long interval) {
if (interval < 1) {
throw new IllegalArgumentException("[interval] must be greater than or equal to 1.");
}
public Builder setInterval(Interval interval) {
this.interval = interval;
return this;
}

/**
* Set the interval for the DateHistogram grouping
* @param timeValue The time value to use as the interval
* @return the {@link Builder} with the interval set.
*/
public Builder setInterval(TimeValue timeValue) {
return setInterval(timeValue.getMillis());
}

/**
* Sets the interval of the DateHistogram grouping
*
* If this DateHistogramInterval is set, it supersedes the #{@link DateHistogramGroupSource#getInterval()}
* @param dateHistogramInterval the DateHistogramInterval to set
* @return The {@link Builder} with the dateHistogramInterval set.
*/
public Builder setDateHistgramInterval(DateHistogramInterval dateHistogramInterval) {
if (dateHistogramInterval == null) {
throw new IllegalArgumentException("[dateHistogramInterval] must not be null");
}
this.dateHistogramInterval = dateHistogramInterval;
return this;
}

/**
* Set the optional String formatting for the time interval.
* @param format The format of the output for the time interval key
Expand All @@ -243,7 +340,7 @@ public Builder setTimeZone(ZoneId timeZone) {
}

public DateHistogramGroupSource build() {
return new DateHistogramGroupSource(field, interval, dateHistogramInterval, format, timeZone);
return new DateHistogramGroupSource(field, interval, format, timeZone);
}
}
}
17 changes: 11 additions & 6 deletions ...va/org/elasticsearch/client/dataframe/transforms/pivot/DateHistogramGroupSourceTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,20 @@

public class DateHistogramGroupSourceTests extends AbstractXContentTestCase<DateHistogramGroupSource> {

public static DateHistogramGroupSource.Interval randomDateHistogramInterval() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the name "randomInterval" sound better now?

if (randomBoolean()) {
return new DateHistogramGroupSource.FixedInterval(new DateHistogramInterval(randomPositiveTimeValue()));
} else {
return new DateHistogramGroupSource.CalendarInterval(new DateHistogramInterval(randomTimeValue(1, 1, "m", "h", "d", "w")));
}
}

public static DateHistogramGroupSource randomDateHistogramGroupSource() {
String field = randomAlphaOfLengthBetween(1, 20);
boolean setInterval = randomBoolean();
return new DateHistogramGroupSource(field,
setInterval ? randomLongBetween(1, 10_000) : 0,
setInterval ? null : randomFrom(DateHistogramInterval.days(10),
DateHistogramInterval.minutes(1), DateHistogramInterval.weeks(1)),
randomBoolean() ? randomAlphaOfLength(10) : null,
randomBoolean() ? randomZone() : null);
randomDateHistogramInterval(),
randomBoolean() ? randomAlphaOfLength(10) : null,
randomBoolean() ? randomZone() : null);
}

@Override
Expand Down
Loading