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

Jump to bottom

[Rollup] Fix Caps Comparator to handle calendar/fixed time #33336

Merged
merged 1 commit into from
Sep 3, 2018
Merged

[Rollup] Fix Caps Comparator to handle calendar/fixed time #33336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions ...ava/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfigSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,20 @@ public void testValidateMatchingField() {
assertThat(e.validationErrors().size(), equalTo(0));
}

public void testValidateWeek() {
ActionRequestValidationException e = new ActionRequestValidationException();
Map<String, Map<String, FieldCapabilities>> responseMap = new HashMap<>();

// Have to mock fieldcaps because the ctor's aren't public...
FieldCapabilities fieldCaps = mock(FieldCapabilities.class);
when(fieldCaps.isAggregatable()).thenReturn(true);
responseMap.put("my_field", Collections.singletonMap("date", fieldCaps));

DateHistogramGroupConfig config = new DateHistogramGroupConfig("my_field", new DateHistogramInterval("1w"), null, null);
config.validateMappings(responseMap, e);
assertThat(e.validationErrors().size(), equalTo(0));
}

/**
* Tests that a DateHistogramGroupConfig can be serialized/deserialized correctly after
* the timezone was changed from DateTimeZone to String.
Expand Down
60 changes: 22 additions & 38 deletions .../plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.rollup;

import org.elasticsearch.common.rounding.DateTimeUnit;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.search.aggregations.AggregationBuilder;
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
Expand All @@ -18,9 +19,7 @@
import org.joda.time.DateTimeZone;

import java.util.ArrayList;
import java.util.Collections;
import java.util.Comparator;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand All @@ -33,30 +32,7 @@
*/
public class RollupJobIdentifierUtils {

private static final Comparator<RollupJobCaps> COMPARATOR = RollupJobIdentifierUtils.getComparator();

public static final Map<String, Integer> CALENDAR_ORDERING;

static {
Map<String, Integer> dateFieldUnits = new HashMap<>(16);
dateFieldUnits.put("year", 8);
dateFieldUnits.put("1y", 8);
dateFieldUnits.put("quarter", 7);
dateFieldUnits.put("1q", 7);
dateFieldUnits.put("month", 6);
dateFieldUnits.put("1M", 6);
dateFieldUnits.put("week", 5);
dateFieldUnits.put("1w", 5);
dateFieldUnits.put("day", 4);
dateFieldUnits.put("1d", 4);
dateFieldUnits.put("hour", 3);
dateFieldUnits.put("1h", 3);
dateFieldUnits.put("minute", 2);
dateFieldUnits.put("1m", 2);
dateFieldUnits.put("second", 1);
dateFieldUnits.put("1s", 1);
CALENDAR_ORDERING = Collections.unmodifiableMap(dateFieldUnits);
}
static final Comparator<RollupJobCaps> COMPARATOR = RollupJobIdentifierUtils.getComparator();

/**
* Given the aggregation tree and a list of available job capabilities, this method will return a set
Expand Down Expand Up @@ -176,8 +152,10 @@ static boolean validateCalendarInterval(DateHistogramInterval requestInterval,

// The request must be gte the config. The CALENDAR_ORDERING map values are integers representing
// relative orders between the calendar units
int requestOrder = CALENDAR_ORDERING.getOrDefault(requestInterval.toString(), Integer.MAX_VALUE);
int configOrder = CALENDAR_ORDERING.getOrDefault(configInterval.toString(), Integer.MAX_VALUE);
DateTimeUnit requestUnit = DateHistogramAggregationBuilder.DATE_FIELD_UNITS.get(requestInterval.toString());
long requestOrder = requestUnit.field(DateTimeZone.UTC).getDurationField().getUnitMillis();
DateTimeUnit configUnit = DateHistogramAggregationBuilder.DATE_FIELD_UNITS.get(configInterval.toString());
long configOrder = configUnit.field(DateTimeZone.UTC).getDurationField().getUnitMillis();

// All calendar units are multiples naturally, so we just care about gte
return requestOrder >= configOrder;
Expand All @@ -190,7 +168,7 @@ static boolean validateFixedInterval(DateHistogramInterval requestInterval,
return false;
}

// Both are fixed, good to conver to millis now
// Both are fixed, good to convert to millis now
long configIntervalMillis = TimeValue.parseTimeValue(configInterval.toString(),
"date_histo.config.interval").getMillis();
long requestIntervalMillis = TimeValue.parseTimeValue(requestInterval.toString(),
Expand Down Expand Up @@ -326,8 +304,8 @@ private static Comparator<RollupJobCaps> getComparator() {
return 0;
}

TimeValue thisTime = null;
TimeValue thatTime = null;
long thisTime = Long.MAX_VALUE;
long thatTime = Long.MAX_VALUE;

// histogram intervals are averaged and compared, with the idea that
// a larger average == better, because it will generate fewer documents
Expand All @@ -344,7 +322,7 @@ private static Comparator<RollupJobCaps> getComparator() {
for (RollupJobCaps.RollupFieldCaps fieldCaps : o1.getFieldCaps().values()) {
for (Map<String, Object> agg : fieldCaps.getAggs()) {
if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
thisTime = TimeValue.parseTimeValue((String) agg.get(RollupField.INTERVAL), RollupField.INTERVAL);
thisTime = getMillisFixedOrCalendar((String) agg.get(RollupField.INTERVAL));
} else if (agg.get(RollupField.AGG).equals(HistogramAggregationBuilder.NAME)) {
thisHistoWeights += (long) agg.get(RollupField.INTERVAL);
counter += 1;
Expand All @@ -360,7 +338,7 @@ private static Comparator<RollupJobCaps> getComparator() {
for (RollupJobCaps.RollupFieldCaps fieldCaps : o2.getFieldCaps().values()) {
for (Map<String, Object> agg : fieldCaps.getAggs()) {
if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
thatTime = TimeValue.parseTimeValue((String) agg.get(RollupField.INTERVAL), RollupField.INTERVAL);
thatTime = getMillisFixedOrCalendar((String) agg.get(RollupField.INTERVAL));
} else if (agg.get(RollupField.AGG).equals(HistogramAggregationBuilder.NAME)) {
thatHistoWeights += (long) agg.get(RollupField.INTERVAL);
counter += 1;
Expand All @@ -371,13 +349,9 @@ private static Comparator<RollupJobCaps> getComparator() {
}
thatHistoWeights = counter == 0 ? 0 : thatHistoWeights / counter;

// DateHistos are mandatory so these should always be present no matter what
assert thisTime != null;
assert thatTime != null;

// Compare on date interval first
// The "smaller" job is the one with the larger interval
int timeCompare = thisTime.compareTo(thatTime);
int timeCompare = Long.compare(thisTime, thatTime);
if (timeCompare != 0) {
return -timeCompare;
}
Expand Down Expand Up @@ -409,4 +383,14 @@ private static Comparator<RollupJobCaps> getComparator() {
// coverage
};
}

static long getMillisFixedOrCalendar(String value) {
DateHistogramInterval interval = new DateHistogramInterval(value);
if (isCalendarInterval(interval)) {
DateTimeUnit intervalUnit = DateHistogramAggregationBuilder.DATE_FIELD_UNITS.get(interval.toString());
return intervalUnit.field(DateTimeZone.UTC).getDurationField().getUnitMillis();
} else {
return TimeValue.parseTimeValue(value, "date_histo.comparator.interval").getMillis();
}
}
}
124 changes: 124 additions & 0 deletions ...gin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.elasticsearch.search.aggregations.metrics.sum.SumAggregationBuilder;
import org.elasticsearch.search.aggregations.support.ValueType;
import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.rollup.RollupField;
import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps;
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
import org.elasticsearch.xpack.core.rollup.job.GroupConfig;
Expand All @@ -24,17 +25,22 @@
import org.elasticsearch.xpack.core.rollup.job.TermsGroupConfig;
import org.joda.time.DateTimeZone;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.greaterThanOrEqualTo;

public class RollupJobIdentifierUtilTests extends ESTestCase {

private static final List<String> UNITS = new ArrayList<>(DateHistogramAggregationBuilder.DATE_FIELD_UNITS.keySet());

public void testOneMatch() {
final GroupConfig group = new GroupConfig(new DateHistogramGroupConfig("foo", new DateHistogramInterval("1h")));
final RollupJobConfig job = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, group, emptyList(), null);
Expand Down Expand Up @@ -577,6 +583,124 @@ public void testValidateCalendarInterval() {
assertFalse(valid);
}

public void testComparatorMixed() {
int numCaps = randomIntBetween(1, 10);
List<RollupJobCaps> caps = new ArrayList<>(numCaps);

for (int i = 0; i < numCaps; i++) {
DateHistogramInterval interval = getRandomInterval();
GroupConfig group = new GroupConfig(new DateHistogramGroupConfig("foo", interval));
RollupJobConfig job = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, group, emptyList(), null);
RollupJobCaps cap = new RollupJobCaps(job);
caps.add(cap);
}

caps.sort(RollupJobIdentifierUtils.COMPARATOR);

// This only tests for calendar/fixed ordering, ignoring the other criteria
for (int i = 1; i < numCaps; i++) {
RollupJobCaps a = caps.get(i - 1);
RollupJobCaps b = caps.get(i);
long aMillis = getMillis(a);
long bMillis = getMillis(b);

assertThat(aMillis, greaterThanOrEqualTo(bMillis));

}
}

public void testComparatorFixed() {
int numCaps = randomIntBetween(1, 10);
List<RollupJobCaps> caps = new ArrayList<>(numCaps);

for (int i = 0; i < numCaps; i++) {
DateHistogramInterval interval = getRandomFixedInterval();
GroupConfig group = new GroupConfig(new DateHistogramGroupConfig("foo", interval));
RollupJobConfig job = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, group, emptyList(), null);
RollupJobCaps cap = new RollupJobCaps(job);
caps.add(cap);
}

caps.sort(RollupJobIdentifierUtils.COMPARATOR);

// This only tests for fixed ordering, ignoring the other criteria
for (int i = 1; i < numCaps; i++) {
RollupJobCaps a = caps.get(i - 1);
RollupJobCaps b = caps.get(i);
long aMillis = getMillis(a);
long bMillis = getMillis(b);

assertThat(aMillis, greaterThanOrEqualTo(bMillis));

}
}

public void testComparatorCalendar() {
int numCaps = randomIntBetween(1, 10);
List<RollupJobCaps> caps = new ArrayList<>(numCaps);

for (int i = 0; i < numCaps; i++) {
DateHistogramInterval interval = getRandomCalendarInterval();
GroupConfig group = new GroupConfig(new DateHistogramGroupConfig("foo", interval));
RollupJobConfig job = new RollupJobConfig("foo", "index", "rollup", "*/5 * * * * ?", 10, group, emptyList(), null);
RollupJobCaps cap = new RollupJobCaps(job);
caps.add(cap);
}

caps.sort(RollupJobIdentifierUtils.COMPARATOR);

// This only tests for calendar ordering, ignoring the other criteria
for (int i = 1; i < numCaps; i++) {
RollupJobCaps a = caps.get(i - 1);
RollupJobCaps b = caps.get(i);
long aMillis = getMillis(a);
long bMillis = getMillis(b);

assertThat(aMillis, greaterThanOrEqualTo(bMillis));

}
}

private static long getMillis(RollupJobCaps cap) {
for (RollupJobCaps.RollupFieldCaps fieldCaps : cap.getFieldCaps().values()) {
for (Map<String, Object> agg : fieldCaps.getAggs()) {
if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
return RollupJobIdentifierUtils.getMillisFixedOrCalendar((String) agg.get(RollupField.INTERVAL));
}
}
}
return Long.MAX_VALUE;
}

private static DateHistogramInterval getRandomInterval() {
if (randomBoolean()) {
return getRandomFixedInterval();
}
return getRandomCalendarInterval();
}

private static DateHistogramInterval getRandomFixedInterval() {
int value = randomIntBetween(1, 1000);
String unit;
int randomValue = randomInt(4);
if (randomValue == 0) {
unit = "ms";
} else if (randomValue == 1) {
unit = "s";
} else if (randomValue == 2) {
unit = "m";
} else if (randomValue == 3) {
unit = "h";
} else {
unit = "d";
}
return new DateHistogramInterval(Integer.toString(value) + unit);
}

private static DateHistogramInterval getRandomCalendarInterval() {
return new DateHistogramInterval(UNITS.get(randomIntBetween(0, UNITS.size()-1)));
}

private Set<RollupJobCaps> singletonSet(RollupJobCaps cap) {
Set<RollupJobCaps> caps = new HashSet<>();
caps.add(cap);
Expand Down