From c0087ad012fc5199a9486e3c37e470d384108e73 Mon Sep 17 00:00:00 2001 From: Zachary Tong Date: Mon, 3 Sep 2018 04:49:19 -0400 Subject: [PATCH] [Rollup] Fix Caps Comparator to handle calendar/fixed time (#33336) The comparator used TimeValue parsing, which meant it couldn't handle calendar time. This fixes the comparator to handle either (and potentially mixed). The mixing shouldn't be an issue since the validation code upstream will prevent it, but was simplest to allow the comparator to handle both. --- ...eHistogramGroupConfigSerializingTests.java | 14 ++ .../rollup/RollupJobIdentifierUtils.java | 60 ++++----- .../rollup/RollupJobIdentifierUtilTests.java | 124 ++++++++++++++++++ 3 files changed, 160 insertions(+), 38 deletions(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfigSerializingTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfigSerializingTests.java index 6b8846def7284..415e1a00a60cf 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfigSerializingTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfigSerializingTests.java @@ -125,6 +125,20 @@ public void testValidateMatchingField() { assertThat(e.validationErrors().size(), equalTo(0)); } + public void testValidateWeek() { + ActionRequestValidationException e = new ActionRequestValidationException(); + Map> 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. diff --git a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java index 8537f2b6a38b4..232034177e87b 100644 --- a/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java +++ b/x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java @@ -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; @@ -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; @@ -33,30 +32,7 @@ */ public class RollupJobIdentifierUtils { - private static final Comparator COMPARATOR = RollupJobIdentifierUtils.getComparator(); - - public static final Map CALENDAR_ORDERING; - - static { - Map 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 COMPARATOR = RollupJobIdentifierUtils.getComparator(); /** * Given the aggregation tree and a list of available job capabilities, this method will return a set @@ -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; @@ -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(), @@ -326,8 +304,8 @@ private static Comparator 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 @@ -344,7 +322,7 @@ private static Comparator getComparator() { for (RollupJobCaps.RollupFieldCaps fieldCaps : o1.getFieldCaps().values()) { for (Map 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; @@ -360,7 +338,7 @@ private static Comparator getComparator() { for (RollupJobCaps.RollupFieldCaps fieldCaps : o2.getFieldCaps().values()) { for (Map 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; @@ -371,13 +349,9 @@ private static Comparator 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; } @@ -409,4 +383,14 @@ private static Comparator 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(); + } + } } diff --git a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java index c23151c4c6afe..54cab648a20a2 100644 --- a/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java +++ b/x-pack/plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtilTests.java @@ -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; @@ -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 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); @@ -577,6 +583,124 @@ public void testValidateCalendarInterval() { assertFalse(valid); } + public void testComparatorMixed() { + int numCaps = randomIntBetween(1, 10); + List 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 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 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 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 singletonSet(RollupJobCaps cap) { Set caps = new HashSet<>(); caps.add(cap);