Skip to content

Commit efa8202

Browse files
polyfractaljimczi
polyfractal
authored and
jimczi
committed
[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.
1 parent a763af9 commit efa8202

File tree

3 files changed

+396
-10
lines changed

3 files changed

+396
-10
lines changed

‎x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/rollup/job/DateHistoGroupConfigSerializingTests.java

+68
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,14 @@
77

88
import org.elasticsearch.action.ActionRequestValidationException;
99
import org.elasticsearch.action.fieldcaps.FieldCapabilities;
10+
import org.elasticsearch.common.io.stream.BytesStreamOutput;
11+
import org.elasticsearch.common.io.stream.StreamInput;
1012
import org.elasticsearch.common.io.stream.Writeable;
1113
import org.elasticsearch.common.xcontent.XContentParser;
1214
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
1315
import org.elasticsearch.test.AbstractSerializingTestCase;
1416
import org.elasticsearch.xpack.core.rollup.ConfigTestHelpers;
17+
import org.joda.time.DateTimeZone;
1518

1619
import java.io.IOException;
1720
import java.util.Collections;
@@ -142,4 +145,69 @@ public void testValidateMatchingField() throws IOException {
142145
config.validateMappings(responseMap, e);
143146
assertThat(e.validationErrors().size(), equalTo(0));
144147
}
148+
149+
public void testValidateWeek() {
150+
ActionRequestValidationException e = new ActionRequestValidationException();
151+
Map<String, Map<String, FieldCapabilities>> responseMap = new HashMap<>();
152+
153+
// Have to mock fieldcaps because the ctor's aren't public...
154+
FieldCapabilities fieldCaps = mock(FieldCapabilities.class);
155+
when(fieldCaps.isAggregatable()).thenReturn(true);
156+
responseMap.put("my_field", Collections.singletonMap("date", fieldCaps));
157+
158+
DateHistoGroupConfig config = new DateHistoGroupConfig.Builder()
159+
.setField("my_field")
160+
.setInterval(new DateHistogramInterval("1w"))
161+
.build();
162+
config.validateMappings(responseMap, e);
163+
assertThat(e.validationErrors().size(), equalTo(0));
164+
}
165+
166+
/**
167+
* Tests that a DateHistogramGroupConfig can be serialized/deserialized correctly after
168+
* the timezone was changed from DateTimeZone to String.
169+
*/
170+
public void testBwcSerialization() throws IOException {
171+
for (int runs = 0; runs < NUMBER_OF_TEST_RUNS; runs++) {
172+
final DateHistoGroupConfig reference = ConfigTestHelpers.getDateHisto().build();
173+
174+
final BytesStreamOutput out = new BytesStreamOutput();
175+
reference.writeTo(out);
176+
177+
// previous way to deserialize a DateHistogramGroupConfig
178+
final StreamInput in = out.bytes().streamInput();
179+
DateHistogramInterval interval = new DateHistogramInterval(in);
180+
String field = in.readString();
181+
DateHistogramInterval delay = in.readOptionalWriteable(DateHistogramInterval::new);
182+
DateTimeZone timeZone = in.readTimeZone();
183+
184+
assertEqualInstances(reference, new DateHistoGroupConfig.Builder()
185+
.setField(field)
186+
.setInterval(interval)
187+
.setDelay(delay)
188+
.setTimeZone(timeZone)
189+
.build());
190+
}
191+
192+
for (int runs = 0; runs < NUMBER_OF_TEST_RUNS; runs++) {
193+
final DateHistoGroupConfig config = ConfigTestHelpers.getDateHisto().build();
194+
195+
// previous way to serialize a DateHistogramGroupConfig
196+
final BytesStreamOutput out = new BytesStreamOutput();
197+
config.getInterval().writeTo(out);
198+
out.writeString(config.getField());
199+
out.writeOptionalWriteable(config.getDelay());
200+
out.writeTimeZone(config.getTimeZone());
201+
202+
final StreamInput in = out.bytes().streamInput();
203+
DateHistoGroupConfig deserialized = new DateHistoGroupConfig(in);
204+
205+
assertEqualInstances(new DateHistoGroupConfig.Builder()
206+
.setField(config.getField())
207+
.setInterval(config.getInterval())
208+
.setDelay(config.getDelay())
209+
.setTimeZone(config.getTimeZone())
210+
.build(), deserialized);
211+
}
212+
}
145213
}

‎x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/RollupJobIdentifierUtils.java

+69-10
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,11 @@
55
*/
66
package org.elasticsearch.xpack.rollup;
77

8+
import org.elasticsearch.common.rounding.DateTimeUnit;
89
import org.elasticsearch.common.unit.TimeValue;
910
import org.elasticsearch.search.aggregations.AggregationBuilder;
1011
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
12+
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramInterval;
1113
import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
1214
import org.elasticsearch.search.aggregations.bucket.terms.TermsAggregationBuilder;
1315
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
@@ -30,7 +32,7 @@
3032
*/
3133
public class RollupJobIdentifierUtils {
3234

33-
private static final Comparator<RollupJobCaps> COMPARATOR = RollupJobIdentifierUtils.getComparator();
35+
static final Comparator<RollupJobCaps> COMPARATOR = RollupJobIdentifierUtils.getComparator();
3436

3537
/**
3638
* Given the aggregation tree and a list of available job capabilities, this method will return a set
@@ -133,6 +135,57 @@ private static void checkDateHisto(DateHistogramAggregationBuilder source, List<
133135
}
134136
}
135137

138+
private static boolean isCalendarInterval(DateHistogramInterval interval) {
139+
return DateHistogramAggregationBuilder.DATE_FIELD_UNITS.containsKey(interval.toString());
140+
}
141+
142+
static boolean validateCalendarInterval(DateHistogramInterval requestInterval,
143+
DateHistogramInterval configInterval) {
144+
// Both must be calendar intervals
145+
if (isCalendarInterval(requestInterval) == false || isCalendarInterval(configInterval) == false) {
146+
return false;
147+
}
148+
149+
// The request must be gte the config. The CALENDAR_ORDERING map values are integers representing
150+
// relative orders between the calendar units
151+
DateTimeUnit requestUnit = DateHistogramAggregationBuilder.DATE_FIELD_UNITS.get(requestInterval.toString());
152+
long requestOrder = requestUnit.field(DateTimeZone.UTC).getDurationField().getUnitMillis();
153+
DateTimeUnit configUnit = DateHistogramAggregationBuilder.DATE_FIELD_UNITS.get(configInterval.toString());
154+
long configOrder = configUnit.field(DateTimeZone.UTC).getDurationField().getUnitMillis();
155+
156+
// All calendar units are multiples naturally, so we just care about gte
157+
return requestOrder >= configOrder;
158+
}
159+
160+
static boolean validateFixedInterval(DateHistogramInterval requestInterval,
161+
DateHistogramInterval configInterval) {
162+
// Neither can be calendar intervals
163+
if (isCalendarInterval(requestInterval) || isCalendarInterval(configInterval)) {
164+
return false;
165+
}
166+
167+
// Both are fixed, good to convert to millis now
168+
long configIntervalMillis = TimeValue.parseTimeValue(configInterval.toString(),
169+
"date_histo.config.interval").getMillis();
170+
long requestIntervalMillis = TimeValue.parseTimeValue(requestInterval.toString(),
171+
"date_histo.request.interval").getMillis();
172+
173+
// Must be a multiple and gte the config
174+
return requestIntervalMillis >= configIntervalMillis && requestIntervalMillis % configIntervalMillis == 0;
175+
}
176+
177+
static boolean validateFixedInterval(long requestInterval, DateHistogramInterval configInterval) {
178+
// config must not be a calendar interval
179+
if (isCalendarInterval(configInterval)) {
180+
return false;
181+
}
182+
long configIntervalMillis = TimeValue.parseTimeValue(configInterval.toString(),
183+
"date_histo.config.interval").getMillis();
184+
185+
// Must be a multiple and gte the config
186+
return requestInterval >= configIntervalMillis && requestInterval % configIntervalMillis == 0;
187+
}
188+
136189
/**
137190
* Find the set of histo's with the largest interval
138191
*/
@@ -247,8 +300,8 @@ private static Comparator<RollupJobCaps> getComparator() {
247300
return 0;
248301
}
249302

250-
TimeValue thisTime = null;
251-
TimeValue thatTime = null;
303+
long thisTime = Long.MAX_VALUE;
304+
long thatTime = Long.MAX_VALUE;
252305

253306
// histogram intervals are averaged and compared, with the idea that
254307
// a larger average == better, because it will generate fewer documents
@@ -265,7 +318,7 @@ private static Comparator<RollupJobCaps> getComparator() {
265318
for (RollupJobCaps.RollupFieldCaps fieldCaps : o1.getFieldCaps().values()) {
266319
for (Map<String, Object> agg : fieldCaps.getAggs()) {
267320
if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
268-
thisTime = TimeValue.parseTimeValue((String) agg.get(RollupField.INTERVAL), RollupField.INTERVAL);
321+
thisTime = getMillisFixedOrCalendar((String) agg.get(RollupField.INTERVAL));
269322
} else if (agg.get(RollupField.AGG).equals(HistogramAggregationBuilder.NAME)) {
270323
thisHistoWeights += (long) agg.get(RollupField.INTERVAL);
271324
counter += 1;
@@ -281,7 +334,7 @@ private static Comparator<RollupJobCaps> getComparator() {
281334
for (RollupJobCaps.RollupFieldCaps fieldCaps : o2.getFieldCaps().values()) {
282335
for (Map<String, Object> agg : fieldCaps.getAggs()) {
283336
if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
284-
thatTime = TimeValue.parseTimeValue((String) agg.get(RollupField.INTERVAL), RollupField.INTERVAL);
337+
thatTime = getMillisFixedOrCalendar((String) agg.get(RollupField.INTERVAL));
285338
} else if (agg.get(RollupField.AGG).equals(HistogramAggregationBuilder.NAME)) {
286339
thatHistoWeights += (long) agg.get(RollupField.INTERVAL);
287340
counter += 1;
@@ -292,13 +345,9 @@ private static Comparator<RollupJobCaps> getComparator() {
292345
}
293346
thatHistoWeights = counter == 0 ? 0 : thatHistoWeights / counter;
294347

295-
// DateHistos are mandatory so these should always be present no matter what
296-
assert thisTime != null;
297-
assert thatTime != null;
298-
299348
// Compare on date interval first
300349
// The "smaller" job is the one with the larger interval
301-
int timeCompare = thisTime.compareTo(thatTime);
350+
int timeCompare = Long.compare(thisTime, thatTime);
302351
if (timeCompare != 0) {
303352
return -timeCompare;
304353
}
@@ -330,4 +379,14 @@ private static Comparator<RollupJobCaps> getComparator() {
330379
// coverage
331380
};
332381
}
382+
383+
static long getMillisFixedOrCalendar(String value) {
384+
DateHistogramInterval interval = new DateHistogramInterval(value);
385+
if (isCalendarInterval(interval)) {
386+
DateTimeUnit intervalUnit = DateHistogramAggregationBuilder.DATE_FIELD_UNITS.get(interval.toString());
387+
return intervalUnit.field(DateTimeZone.UTC).getDurationField().getUnitMillis();
388+
} else {
389+
return TimeValue.parseTimeValue(value, "date_histo.comparator.interval").getMillis();
390+
}
391+
}
333392
}

0 commit comments

Comments
 (0)