Skip to content

Commit 1f51f20

Browse files
polyfractalpolyfractal
authored
[Rollup] Validate timezones based on rules not string comparision (#36237)
The date_histogram internally converts obsolete timezones (such as "Canada/Mountain") into their modern equivalent ("America/Edmonton"). But rollup just stored the TZ as provided by the user. When checking the TZ for query validation we used a string comparison, which would fail due to the date_histo's upgrading behavior. Instead, we should convert both to a TimeZone object and check if their rules are compatible.
1 parent c31a5b1 commit 1f51f20

File tree

15 files changed

+645
-158
lines changed

15 files changed

+645
-158
lines changed

‎server/src/main/java/org/elasticsearch/common/time/DateUtils.java‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,127 @@ public static DateTimeZone zoneIdToDateTimeZone(ZoneId zoneId) {
6666
DEPRECATED_SHORT_TZ_IDS = tzs.keySet();
6767
}
6868

69+
// Map of deprecated timezones and their recommended new counterpart
70+
public static final Map<String, String> DEPRECATED_LONG_TIMEZONES;
71+
static {
72+
Map<String, String> tzs = new HashMap<>();
73+
tzs.put("Africa/Asmera","Africa/Nairobi");
74+
tzs.put("Africa/Timbuktu","Africa/Abidjan");
75+
tzs.put("America/Argentina/ComodRivadavia","America/Argentina/Catamarca");
76+
tzs.put("America/Atka","America/Adak");
77+
tzs.put("America/Buenos_Aires","America/Argentina/Buenos_Aires");
78+
tzs.put("America/Catamarca","America/Argentina/Catamarca");
79+
tzs.put("America/Coral_Harbour","America/Atikokan");
80+
tzs.put("America/Cordoba","America/Argentina/Cordoba");
81+
tzs.put("America/Ensenada","America/Tijuana");
82+
tzs.put("America/Fort_Wayne","America/Indiana/Indianapolis");
83+
tzs.put("America/Indianapolis","America/Indiana/Indianapolis");
84+
tzs.put("America/Jujuy","America/Argentina/Jujuy");
85+
tzs.put("America/Knox_IN","America/Indiana/Knox");
86+
tzs.put("America/Louisville","America/Kentucky/Louisville");
87+
tzs.put("America/Mendoza","America/Argentina/Mendoza");
88+
tzs.put("America/Montreal","America/Toronto");
89+
tzs.put("America/Porto_Acre","America/Rio_Branco");
90+
tzs.put("America/Rosario","America/Argentina/Cordoba");
91+
tzs.put("America/Santa_Isabel","America/Tijuana");
92+
tzs.put("America/Shiprock","America/Denver");
93+
tzs.put("America/Virgin","America/Port_of_Spain");
94+
tzs.put("Antarctica/South_Pole","Pacific/Auckland");
95+
tzs.put("Asia/Ashkhabad","Asia/Ashgabat");
96+
tzs.put("Asia/Calcutta","Asia/Kolkata");
97+
tzs.put("Asia/Chongqing","Asia/Shanghai");
98+
tzs.put("Asia/Chungking","Asia/Shanghai");
99+
tzs.put("Asia/Dacca","Asia/Dhaka");
100+
tzs.put("Asia/Harbin","Asia/Shanghai");
101+
tzs.put("Asia/Kashgar","Asia/Urumqi");
102+
tzs.put("Asia/Katmandu","Asia/Kathmandu");
103+
tzs.put("Asia/Macao","Asia/Macau");
104+
tzs.put("Asia/Rangoon","Asia/Yangon");
105+
tzs.put("Asia/Saigon","Asia/Ho_Chi_Minh");
106+
tzs.put("Asia/Tel_Aviv","Asia/Jerusalem");
107+
tzs.put("Asia/Thimbu","Asia/Thimphu");
108+
tzs.put("Asia/Ujung_Pandang","Asia/Makassar");
109+
tzs.put("Asia/Ulan_Bator","Asia/Ulaanbaatar");
110+
tzs.put("Atlantic/Faeroe","Atlantic/Faroe");
111+
tzs.put("Atlantic/Jan_Mayen","Europe/Oslo");
112+
tzs.put("Australia/ACT","Australia/Sydney");
113+
tzs.put("Australia/Canberra","Australia/Sydney");
114+
tzs.put("Australia/LHI","Australia/Lord_Howe");
115+
tzs.put("Australia/NSW","Australia/Sydney");
116+
tzs.put("Australia/North","Australia/Darwin");
117+
tzs.put("Australia/Queensland","Australia/Brisbane");
118+
tzs.put("Australia/South","Australia/Adelaide");
119+
tzs.put("Australia/Tasmania","Australia/Hobart");
120+
tzs.put("Australia/Victoria","Australia/Melbourne");
121+
tzs.put("Australia/West","Australia/Perth");
122+
tzs.put("Australia/Yancowinna","Australia/Broken_Hill");
123+
tzs.put("Brazil/Acre","America/Rio_Branco");
124+
tzs.put("Brazil/DeNoronha","America/Noronha");
125+
tzs.put("Brazil/East","America/Sao_Paulo");
126+
tzs.put("Brazil/West","America/Manaus");
127+
tzs.put("Canada/Atlantic","America/Halifax");
128+
tzs.put("Canada/Central","America/Winnipeg");
129+
tzs.put("Canada/East-Saskatchewan","America/Regina");
130+
tzs.put("Canada/Eastern","America/Toronto");
131+
tzs.put("Canada/Mountain","America/Edmonton");
132+
tzs.put("Canada/Newfoundland","America/St_Johns");
133+
tzs.put("Canada/Pacific","America/Vancouver");
134+
tzs.put("Canada/Yukon","America/Whitehorse");
135+
tzs.put("Chile/Continental","America/Santiago");
136+
tzs.put("Chile/EasterIsland","Pacific/Easter");
137+
tzs.put("Cuba","America/Havana");
138+
tzs.put("Egypt","Africa/Cairo");
139+
tzs.put("Eire","Europe/Dublin");
140+
tzs.put("Europe/Belfast","Europe/London");
141+
tzs.put("Europe/Tiraspol","Europe/Chisinau");
142+
tzs.put("GB","Europe/London");
143+
tzs.put("GB-Eire","Europe/London");
144+
tzs.put("Greenwich","Etc/GMT");
145+
tzs.put("Hongkong","Asia/Hong_Kong");
146+
tzs.put("Iceland","Atlantic/Reykjavik");
147+
tzs.put("Iran","Asia/Tehran");
148+
tzs.put("Israel","Asia/Jerusalem");
149+
tzs.put("Jamaica","America/Jamaica");
150+
tzs.put("Japan","Asia/Tokyo");
151+
tzs.put("Kwajalein","Pacific/Kwajalein");
152+
tzs.put("Libya","Africa/Tripoli");
153+
tzs.put("Mexico/BajaNorte","America/Tijuana");
154+
tzs.put("Mexico/BajaSur","America/Mazatlan");
155+
tzs.put("Mexico/General","America/Mexico_City");
156+
tzs.put("NZ","Pacific/Auckland");
157+
tzs.put("NZ-CHAT","Pacific/Chatham");
158+
tzs.put("Navajo","America/Denver");
159+
tzs.put("PRC","Asia/Shanghai");
160+
tzs.put("Pacific/Johnston","Pacific/Honolulu");
161+
tzs.put("Pacific/Ponape","Pacific/Pohnpei");
162+
tzs.put("Pacific/Samoa","Pacific/Pago_Pago");
163+
tzs.put("Pacific/Truk","Pacific/Chuuk");
164+
tzs.put("Pacific/Yap","Pacific/Chuuk");
165+
tzs.put("Poland","Europe/Warsaw");
166+
tzs.put("Portugal","Europe/Lisbon");
167+
tzs.put("ROC","Asia/Taipei");
168+
tzs.put("ROK","Asia/Seoul");
169+
tzs.put("Singapore","Asia/Singapore");
170+
tzs.put("Turkey","Europe/Istanbul");
171+
tzs.put("UCT","Etc/UCT");
172+
tzs.put("US/Alaska","America/Anchorage");
173+
tzs.put("US/Aleutian","America/Adak");
174+
tzs.put("US/Arizona","America/Phoenix");
175+
tzs.put("US/Central","America/Chicago");
176+
tzs.put("US/East-Indiana","America/Indiana/Indianapolis");
177+
tzs.put("US/Eastern","America/New_York");
178+
tzs.put("US/Hawaii","Pacific/Honolulu");
179+
tzs.put("US/Indiana-Starke","America/Indiana/Knox");
180+
tzs.put("US/Michigan","America/Detroit");
181+
tzs.put("US/Mountain","America/Denver");
182+
tzs.put("US/Pacific","America/Los_Angeles");
183+
tzs.put("US/Samoa","Pacific/Pago_Pago");
184+
tzs.put("Universal","Etc/UTC");
185+
tzs.put("W-SU","Europe/Moscow");
186+
tzs.put("Zulu","Etc/UTC");
187+
DEPRECATED_LONG_TIMEZONES = Collections.unmodifiableMap(tzs);
188+
}
189+
69190
public static ZoneId dateTimeZoneToZoneId(DateTimeZone timeZone) {
70191
if (timeZone == null) {
71192
return null;

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/rollup/job/DateHistogramGroupConfig.java‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
import java.io.IOException;
2626
import java.time.ZoneId;
27+
import java.time.ZoneOffset;
2728
import java.util.Map;
2829
import java.util.Objects;
2930

@@ -52,7 +53,8 @@ public class DateHistogramGroupConfig implements Writeable, ToXContentObject {
5253
private static final String FIELD = "field";
5354
public static final String TIME_ZONE = "time_zone";
5455
public static final String DELAY = "delay";
55-
private static final String DEFAULT_TIMEZONE = "UTC";
56+
public static final String DEFAULT_TIMEZONE = "UTC";
57+
public static final ZoneId DEFAULT_ZONEID_TIMEZONE = ZoneOffset.UTC;
5658
private static final ConstructingObjectParser<DateHistogramGroupConfig, Void> PARSER;
5759
static {
5860
PARSER = new ConstructingObjectParser<>(NAME, a ->
@@ -210,12 +212,12 @@ public boolean equals(final Object other) {
210212
return Objects.equals(interval, that.interval)
211213
&& Objects.equals(field, that.field)
212214
&& Objects.equals(delay, that.delay)
213-
&& Objects.equals(timeZone, that.timeZone);
215+
&& ZoneId.of(timeZone, ZoneId.SHORT_IDS).getRules().equals(ZoneId.of(that.timeZone, ZoneId.SHORT_IDS).getRules());
214216
}
215217

216218
@Override
217219
public int hashCode() {
218-
return Objects.hash(interval, field, delay, timeZone);
220+
return Objects.hash(interval, field, delay, ZoneId.of(timeZone));
219221
}
220222

221223
@Override
@@ -235,7 +237,7 @@ private static Rounding createRounding(final String expr, final String timeZone)
235237
} else {
236238
rounding = new Rounding.Builder(TimeValue.parseTimeValue(expr, "createRounding"));
237239
}
238-
rounding.timeZone(ZoneId.of(timeZone));
240+
rounding.timeZone(ZoneId.of(timeZone, ZoneId.SHORT_IDS));
239241
return rounding.build();
240242
}
241243
}

‎x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/datafeed/extractor/aggregation/RollupDataExtractorFactory.java‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@
2323
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
2424
import org.elasticsearch.xpack.ml.datafeed.extractor.DataExtractorFactory;
2525

26+
import java.time.ZoneId;
27+
import java.time.ZoneOffset;
2628
import java.util.ArrayList;
2729
import java.util.Arrays;
2830
import java.util.Collection;
@@ -124,7 +126,7 @@ private static boolean validInterval(long datafeedInterval, ParsedRollupCaps rol
124126
if (rollupJobGroupConfig.hasDatehistogram() == false) {
125127
return false;
126128
}
127-
if ("UTC".equalsIgnoreCase(rollupJobGroupConfig.getTimezone()) == false) {
129+
if (ZoneId.of(rollupJobGroupConfig.getTimezone()).getRules().equals(ZoneOffset.UTC.getRules()) == false) {
128130
return false;
129131
}
130132
try {

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

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps;
1818
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
1919

20+
import java.time.ZoneId;
2021
import java.util.ArrayList;
2122
import java.util.Comparator;
2223
import java.util.HashSet;
@@ -96,11 +97,13 @@ private static void checkDateHisto(DateHistogramAggregationBuilder source, List<
9697
if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
9798
DateHistogramInterval interval = new DateHistogramInterval((String)agg.get(RollupField.INTERVAL));
9899

99-
String thisTimezone = (String)agg.get(DateHistogramGroupConfig.TIME_ZONE);
100-
String sourceTimeZone = source.timeZone() == null ? "UTC" : source.timeZone().toString();
100+
ZoneId thisTimezone = ZoneId.of(((String) agg.get(DateHistogramGroupConfig.TIME_ZONE)), ZoneId.SHORT_IDS);
101+
ZoneId sourceTimeZone = source.timeZone() == null
102+
? DateHistogramGroupConfig.DEFAULT_ZONEID_TIMEZONE
103+
: ZoneId.of(source.timeZone().toString(), ZoneId.SHORT_IDS);
101104

102105
// Ensure we are working on the same timezone
103-
if (thisTimezone.equalsIgnoreCase(sourceTimeZone) == false) {
106+
if (thisTimezone.getRules().equals(sourceTimeZone.getRules()) == false) {
104107
continue;
105108
}
106109
if (source.dateHistogramInterval() != null) {

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

Lines changed: 14 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -11,8 +11,6 @@
1111
import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput;
1212
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
1313
import org.elasticsearch.common.io.stream.StreamInput;
14-
import org.elasticsearch.index.query.QueryBuilder;
15-
import org.elasticsearch.index.query.TermQueryBuilder;
1614
import org.elasticsearch.search.aggregations.AggregationBuilder;
1715
import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
1816
import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
@@ -22,8 +20,8 @@
2220
import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
2321
import org.elasticsearch.xpack.core.rollup.RollupField;
2422
import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
25-
import org.joda.time.DateTimeZone;
2623

24+
import java.time.ZoneId;
2725
import java.util.ArrayList;
2826
import java.util.Collections;
2927
import java.util.List;
@@ -47,7 +45,7 @@
4745
* }</pre>
4846
*
4947
*
50-
* The only publicly "consumable" API is {@link #translateAggregation(AggregationBuilder, List, NamedWriteableRegistry)}.
48+
* The only publicly "consumable" API is {@link #translateAggregation(AggregationBuilder, NamedWriteableRegistry)}.
5149
*/
5250
public class RollupRequestTranslator {
5351

@@ -116,26 +114,22 @@ public class RollupRequestTranslator {
116114
* relevant method below.
117115
*
118116
* @param source The source aggregation to translate into rollup-enabled version
119-
* @param filterConditions A list used to track any filter conditions that sub-aggs may
120-
* require.
121117
* @param registry Registry containing the various aggregations so that we can easily
122118
* deserialize into a stream for cloning
123119
* @return Returns the fully translated aggregation tree. Note that it returns a list instead
124120
* of a single AggBuilder, since some aggregations (e.g. avg) may result in two
125121
* translated aggs (sum + count)
126122
*/
127-
public static List<AggregationBuilder> translateAggregation(AggregationBuilder source,
128-
List<QueryBuilder> filterConditions,
129-
NamedWriteableRegistry registry) {
123+
public static List<AggregationBuilder> translateAggregation(AggregationBuilder source, NamedWriteableRegistry registry) {
130124

131125
if (source.getWriteableName().equals(DateHistogramAggregationBuilder.NAME)) {
132-
return translateDateHistogram((DateHistogramAggregationBuilder) source, filterConditions, registry);
126+
return translateDateHistogram((DateHistogramAggregationBuilder) source, registry);
133127
} else if (source.getWriteableName().equals(HistogramAggregationBuilder.NAME)) {
134-
return translateHistogram((HistogramAggregationBuilder) source, filterConditions, registry);
128+
return translateHistogram((HistogramAggregationBuilder) source, registry);
135129
} else if (RollupField.SUPPORTED_METRICS.contains(source.getWriteableName())) {
136130
return translateVSLeaf((ValuesSourceAggregationBuilder.LeafOnly)source, registry);
137131
} else if (source.getWriteableName().equals(TermsAggregationBuilder.NAME)) {
138-
return translateTerms((TermsAggregationBuilder)source, filterConditions, registry);
132+
return translateTerms((TermsAggregationBuilder)source, registry);
139133
} else {
140134
throw new IllegalArgumentException("Unable to translate aggregation tree into Rollup. Aggregation ["
141135
+ source.getName() + "] is of type [" + source.getClass().getSimpleName() + "] which is " +
@@ -195,22 +189,13 @@ public static List<AggregationBuilder> translateAggregation(AggregationBuilder s
195189
* <li>Field: `{timestamp field}.date_histogram._count`</li>
196190
* </ul>
197191
* </li>
198-
* <li>Add a filter condition:</li>
199-
* <li>
200-
* <ul>
201-
* <li>Query type: TermQuery</li>
202-
* <li>Field: `{timestamp_field}.date_histogram.interval`</li>
203-
* <li>Value: `{source interval}`</li>
204-
* </ul>
205-
* </li>
206192
* </ul>
207193
*
208194
*/
209195
private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggregationBuilder source,
210-
List<QueryBuilder> filterConditions,
211196
NamedWriteableRegistry registry) {
212197

213-
return translateVSAggBuilder(source, filterConditions, registry, () -> {
198+
return translateVSAggBuilder(source, registry, () -> {
214199
DateHistogramAggregationBuilder rolledDateHisto
215200
= new DateHistogramAggregationBuilder(source.getName());
216201

@@ -220,13 +205,9 @@ private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggr
220205
rolledDateHisto.interval(source.interval());
221206
}
222207

223-
String timezone = source.timeZone() == null ? DateTimeZone.UTC.toString() : source.timeZone().toString();
224-
filterConditions.add(new TermQueryBuilder(RollupField.formatFieldName(source,
225-
DateHistogramGroupConfig.TIME_ZONE), timezone));
208+
ZoneId timeZone = source.timeZone() == null ? DateHistogramGroupConfig.DEFAULT_ZONEID_TIMEZONE : source.timeZone();
209+
rolledDateHisto.timeZone(timeZone);
226210

227-
if (source.timeZone() != null) {
228-
rolledDateHisto.timeZone(source.timeZone());
229-
}
230211
rolledDateHisto.offset(source.offset());
231212
if (source.extendedBounds() != null) {
232213
rolledDateHisto.extendedBounds(source.extendedBounds());
@@ -248,14 +229,13 @@ private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggr
248229
* Notably, it adds a Sum metric to calculate the doc_count in each bucket.
249230
*
250231
* Conventions are identical to a date_histogram (excepting date-specific details), so see
251-
* {@link #translateDateHistogram(DateHistogramAggregationBuilder, List, NamedWriteableRegistry)} for
232+
* {@link #translateDateHistogram(DateHistogramAggregationBuilder, NamedWriteableRegistry)} for
252233
* a complete list of conventions, examples, etc
253234
*/
254235
private static List<AggregationBuilder> translateHistogram(HistogramAggregationBuilder source,
255-
List<QueryBuilder> filterConditions,
256236
NamedWriteableRegistry registry) {
257237

258-
return translateVSAggBuilder(source, filterConditions, registry, () -> {
238+
return translateVSAggBuilder(source, registry, () -> {
259239
HistogramAggregationBuilder rolledHisto
260240
= new HistogramAggregationBuilder(source.getName());
261241

@@ -328,10 +308,9 @@ private static List<AggregationBuilder> translateHistogram(HistogramAggregationB
328308
*
329309
*/
330310
private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder source,
331-
List<QueryBuilder> filterConditions,
332311
NamedWriteableRegistry registry) {
333312

334-
return translateVSAggBuilder(source, filterConditions, registry, () -> {
313+
return translateVSAggBuilder(source, registry, () -> {
335314
TermsAggregationBuilder rolledTerms
336315
= new TermsAggregationBuilder(source.getName(), source.valueType());
337316
rolledTerms.field(RollupField.formatFieldName(source, RollupField.VALUE));
@@ -359,8 +338,6 @@ private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder s
359338
* ValueSourceBuilder. This method is called by all the agg-specific methods (e.g. translateDateHistogram())
360339
*
361340
* @param source The source aggregation that we wish to translate
362-
* @param filterConditions A list of existing filter conditions, in case we need to add some
363-
* for this particular agg
364341
* @param registry Named registry for serializing leaf metrics. Not actually used by this method,
365342
* but is passed downwards for leaf usage
366343
* @param factory A factory closure that generates a new shallow clone of the `source`. E.g. if `source` is
@@ -371,15 +348,14 @@ private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder s
371348
* @return the translated multi-bucket ValueSourceAggBuilder
372349
*/
373350
private static <T extends ValuesSourceAggregationBuilder> List<AggregationBuilder>
374-
translateVSAggBuilder(ValuesSourceAggregationBuilder source, List<QueryBuilder> filterConditions,
375-
NamedWriteableRegistry registry, Supplier<T> factory) {
351+
translateVSAggBuilder(ValuesSourceAggregationBuilder source, NamedWriteableRegistry registry, Supplier<T> factory) {
376352

377353
T rolled = factory.get();
378354

379355
// Translate all subaggs and add to the newly translated agg
380356
// NOTE: using for loop instead of stream because compiler explodes with a bug :/
381357
for (AggregationBuilder subAgg : source.getSubAggregations()) {
382-
List<AggregationBuilder> translated = translateAggregation(subAgg, filterConditions, registry);
358+
List<AggregationBuilder> translated = translateAggregation(subAgg, registry);
383359
for (AggregationBuilder t : translated) {
384360
rolled.subAggregation(t);
385361
}

0 commit comments

Comments
 (0)