[Rollup] Validate timezones based on rules not string comparision

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

@@ -58,6 +58,127 @@ public static DateTimeZone zoneIdToDateTimeZone(ZoneId zoneId) {
         DEPRECATED_SHORT_TZ_IDS = tzs.keySet();
     }
 
+    // Map of deprecated timezones and their recommended new counterpart
+    public static final Map<String, String> DEPRECATED_LONG_TIMEZONES;
+    static {
+        Map<String, String> tzs = new HashMap<>();
+        tzs.put("Africa/Asmera","Africa/Nairobi");
+        tzs.put("Africa/Timbuktu","Africa/Abidjan");
+        tzs.put("America/Argentina/ComodRivadavia","America/Argentina/Catamarca");
+        tzs.put("America/Atka","America/Adak");
+        tzs.put("America/Buenos_Aires","America/Argentina/Buenos_Aires");
+        tzs.put("America/Catamarca","America/Argentina/Catamarca");
+        tzs.put("America/Coral_Harbour","America/Atikokan");
+        tzs.put("America/Cordoba","America/Argentina/Cordoba");
+        tzs.put("America/Ensenada","America/Tijuana");
+        tzs.put("America/Fort_Wayne","America/Indiana/Indianapolis");
+        tzs.put("America/Indianapolis","America/Indiana/Indianapolis");
+        tzs.put("America/Jujuy","America/Argentina/Jujuy");
+        tzs.put("America/Knox_IN","America/Indiana/Knox");
+        tzs.put("America/Louisville","America/Kentucky/Louisville");
+        tzs.put("America/Mendoza","America/Argentina/Mendoza");
+        tzs.put("America/Montreal","America/Toronto");
+        tzs.put("America/Porto_Acre","America/Rio_Branco");
+        tzs.put("America/Rosario","America/Argentina/Cordoba");
+        tzs.put("America/Santa_Isabel","America/Tijuana");
+        tzs.put("America/Shiprock","America/Denver");
+        tzs.put("America/Virgin","America/Port_of_Spain");
+        tzs.put("Antarctica/South_Pole","Pacific/Auckland");
+        tzs.put("Asia/Ashkhabad","Asia/Ashgabat");
+        tzs.put("Asia/Calcutta","Asia/Kolkata");
+        tzs.put("Asia/Chongqing","Asia/Shanghai");
+        tzs.put("Asia/Chungking","Asia/Shanghai");
+        tzs.put("Asia/Dacca","Asia/Dhaka");
+        tzs.put("Asia/Harbin","Asia/Shanghai");
+        tzs.put("Asia/Kashgar","Asia/Urumqi");
+        tzs.put("Asia/Katmandu","Asia/Kathmandu");
+        tzs.put("Asia/Macao","Asia/Macau");
+        tzs.put("Asia/Rangoon","Asia/Yangon");
+        tzs.put("Asia/Saigon","Asia/Ho_Chi_Minh");
+        tzs.put("Asia/Tel_Aviv","Asia/Jerusalem");
+        tzs.put("Asia/Thimbu","Asia/Thimphu");
+        tzs.put("Asia/Ujung_Pandang","Asia/Makassar");
+        tzs.put("Asia/Ulan_Bator","Asia/Ulaanbaatar");
+        tzs.put("Atlantic/Faeroe","Atlantic/Faroe");
+        tzs.put("Atlantic/Jan_Mayen","Europe/Oslo");
+        tzs.put("Australia/ACT","Australia/Sydney");
+        tzs.put("Australia/Canberra","Australia/Sydney");
+        tzs.put("Australia/LHI","Australia/Lord_Howe");
+        tzs.put("Australia/NSW","Australia/Sydney");
+        tzs.put("Australia/North","Australia/Darwin");
+        tzs.put("Australia/Queensland","Australia/Brisbane");
+        tzs.put("Australia/South","Australia/Adelaide");
+        tzs.put("Australia/Tasmania","Australia/Hobart");
+        tzs.put("Australia/Victoria","Australia/Melbourne");
+        tzs.put("Australia/West","Australia/Perth");
+        tzs.put("Australia/Yancowinna","Australia/Broken_Hill");
+        tzs.put("Brazil/Acre","America/Rio_Branco");
+        tzs.put("Brazil/DeNoronha","America/Noronha");
+        tzs.put("Brazil/East","America/Sao_Paulo");
+        tzs.put("Brazil/West","America/Manaus");
+        tzs.put("Canada/Atlantic","America/Halifax");
+        tzs.put("Canada/Central","America/Winnipeg");
+        tzs.put("Canada/East-Saskatchewan","America/Regina");
+        tzs.put("Canada/Eastern","America/Toronto");
+        tzs.put("Canada/Mountain","America/Edmonton");
+        tzs.put("Canada/Newfoundland","America/St_Johns");
+        tzs.put("Canada/Pacific","America/Vancouver");
+        tzs.put("Canada/Yukon","America/Whitehorse");
+        tzs.put("Chile/Continental","America/Santiago");
+        tzs.put("Chile/EasterIsland","Pacific/Easter");
+        tzs.put("Cuba","America/Havana");
+        tzs.put("Egypt","Africa/Cairo");
+        tzs.put("Eire","Europe/Dublin");
+        tzs.put("Europe/Belfast","Europe/London");
+        tzs.put("Europe/Tiraspol","Europe/Chisinau");
+        tzs.put("GB","Europe/London");
+        tzs.put("GB-Eire","Europe/London");
+        tzs.put("Greenwich","Etc/GMT");
+        tzs.put("Hongkong","Asia/Hong_Kong");
+        tzs.put("Iceland","Atlantic/Reykjavik");
+        tzs.put("Iran","Asia/Tehran");
+        tzs.put("Israel","Asia/Jerusalem");
+        tzs.put("Jamaica","America/Jamaica");
+        tzs.put("Japan","Asia/Tokyo");
+        tzs.put("Kwajalein","Pacific/Kwajalein");
+        tzs.put("Libya","Africa/Tripoli");
+        tzs.put("Mexico/BajaNorte","America/Tijuana");
+        tzs.put("Mexico/BajaSur","America/Mazatlan");
+        tzs.put("Mexico/General","America/Mexico_City");
+        tzs.put("NZ","Pacific/Auckland");
+        tzs.put("NZ-CHAT","Pacific/Chatham");
+        tzs.put("Navajo","America/Denver");
+        tzs.put("PRC","Asia/Shanghai");
+        tzs.put("Pacific/Johnston","Pacific/Honolulu");
+        tzs.put("Pacific/Ponape","Pacific/Pohnpei");
+        tzs.put("Pacific/Samoa","Pacific/Pago_Pago");
+        tzs.put("Pacific/Truk","Pacific/Chuuk");
+        tzs.put("Pacific/Yap","Pacific/Chuuk");
+        tzs.put("Poland","Europe/Warsaw");
+        tzs.put("Portugal","Europe/Lisbon");
+        tzs.put("ROC","Asia/Taipei");
+        tzs.put("ROK","Asia/Seoul");
+        tzs.put("Singapore","Asia/Singapore");
+        tzs.put("Turkey","Europe/Istanbul");
+        tzs.put("UCT","Etc/UCT");
+        tzs.put("US/Alaska","America/Anchorage");
+        tzs.put("US/Aleutian","America/Adak");
+        tzs.put("US/Arizona","America/Phoenix");
+        tzs.put("US/Central","America/Chicago");
+        tzs.put("US/East-Indiana","America/Indiana/Indianapolis");
+        tzs.put("US/Eastern","America/New_York");
+        tzs.put("US/Hawaii","Pacific/Honolulu");
+        tzs.put("US/Indiana-Starke","America/Indiana/Knox");
+        tzs.put("US/Michigan","America/Detroit");
+        tzs.put("US/Mountain","America/Denver");
+        tzs.put("US/Pacific","America/Los_Angeles");
+        tzs.put("US/Samoa","Pacific/Pago_Pago");
+        tzs.put("Universal","Etc/UTC");
+        tzs.put("W-SU","Europe/Moscow");
+        tzs.put("Zulu","Etc/UTC");
+        DEPRECATED_LONG_TIMEZONES = Collections.unmodifiableMap(tzs);
+    }
+
     public static ZoneId dateTimeZoneToZoneId(DateTimeZone timeZone) {
         if (timeZone == null) {
             return null;

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

@@ -24,6 +24,7 @@
 
 import java.io.IOException;
 import java.time.ZoneId;
+import java.time.ZoneOffset;
 import java.util.Map;
 import java.util.Objects;
 
@@ -52,7 +53,8 @@ public class DateHistogramGroupConfig implements Writeable, ToXContentObject {
     private static final String FIELD = "field";
     public static final String TIME_ZONE = "time_zone";
     public static final String DELAY = "delay";
-    private static final String DEFAULT_TIMEZONE = "UTC";
+    public static final String DEFAULT_TIMEZONE = "UTC";
+    public static final ZoneId DEFAULT_ZONEID_TIMEZONE = ZoneOffset.UTC;
     private static final ConstructingObjectParser<DateHistogramGroupConfig, Void> PARSER;
     static {
         PARSER = new ConstructingObjectParser<>(NAME, a ->
@@ -210,12 +212,12 @@ public boolean equals(final Object other) {
         return Objects.equals(interval, that.interval)
             && Objects.equals(field, that.field)
             && Objects.equals(delay, that.delay)
-            && Objects.equals(timeZone, that.timeZone);
+            && ZoneId.of(timeZone, ZoneId.SHORT_IDS).getRules().equals(ZoneId.of(that.timeZone, ZoneId.SHORT_IDS).getRules());
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(interval, field, delay, timeZone);
+        return Objects.hash(interval, field, delay, ZoneId.of(timeZone));
     }
 
     @Override
@@ -235,7 +237,7 @@ private static Rounding createRounding(final String expr, final String timeZone)
         } else {
             rounding = new Rounding.Builder(TimeValue.parseTimeValue(expr, "createRounding"));
         }
-        rounding.timeZone(ZoneId.of(timeZone));
+        rounding.timeZone(ZoneId.of(timeZone, ZoneId.SHORT_IDS));
         return rounding.build();
     }
 }

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

@@ -22,6 +22,8 @@
 import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
 import org.elasticsearch.xpack.ml.datafeed.extractor.DataExtractorFactory;
 
+import java.time.ZoneId;
+import java.time.ZoneOffset;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -119,7 +121,7 @@ private static boolean validInterval(long datafeedInterval, ParsedRollupCaps rol
         if (rollupJobGroupConfig.hasDatehistogram() == false) {
             return false;
         }
-        if ("UTC".equalsIgnoreCase(rollupJobGroupConfig.getTimezone()) == false) {
+        if (ZoneId.of(rollupJobGroupConfig.getTimezone()).getRules().equals(ZoneOffset.UTC.getRules()) == false) {
             return false;
         }
         try {

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

@@ -17,6 +17,7 @@
 import org.elasticsearch.xpack.core.rollup.action.RollupJobCaps;
 import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
 
+import java.time.ZoneId;
 import java.util.ArrayList;
 import java.util.Comparator;
 import java.util.HashSet;
@@ -96,11 +97,13 @@ private static void checkDateHisto(DateHistogramAggregationBuilder source, List<
                     if (agg.get(RollupField.AGG).equals(DateHistogramAggregationBuilder.NAME)) {
                         DateHistogramInterval interval = new DateHistogramInterval((String)agg.get(RollupField.INTERVAL));
 
-                        String thisTimezone  = (String)agg.get(DateHistogramGroupConfig.TIME_ZONE);
-                        String sourceTimeZone = source.timeZone() == null ? "UTC" : source.timeZone().toString();
+                        ZoneId thisTimezone = ZoneId.of(((String) agg.get(DateHistogramGroupConfig.TIME_ZONE)), ZoneId.SHORT_IDS);
+                        ZoneId sourceTimeZone = source.timeZone() == null
+                            ? DateHistogramGroupConfig.DEFAULT_ZONEID_TIMEZONE
+                            : ZoneId.of(source.timeZone().toString(), ZoneId.SHORT_IDS);
 
                         // Ensure we are working on the same timezone
-                        if (thisTimezone.equalsIgnoreCase(sourceTimeZone) == false) {
+                        if (thisTimezone.getRules().equals(sourceTimeZone.getRules()) == false) {
                             continue;
                         }
                         if (source.dateHistogramInterval() != null) {

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

@@ -11,8 +11,6 @@
 import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput;
 import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
 import org.elasticsearch.common.io.stream.StreamInput;
-import org.elasticsearch.index.query.QueryBuilder;
-import org.elasticsearch.index.query.TermQueryBuilder;
 import org.elasticsearch.search.aggregations.AggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.histogram.DateHistogramAggregationBuilder;
 import org.elasticsearch.search.aggregations.bucket.histogram.HistogramAggregationBuilder;
@@ -21,8 +19,6 @@
 import org.elasticsearch.search.aggregations.metrics.SumAggregationBuilder;
 import org.elasticsearch.search.aggregations.support.ValuesSourceAggregationBuilder;
 import org.elasticsearch.xpack.core.rollup.RollupField;
-import org.elasticsearch.xpack.core.rollup.job.DateHistogramGroupConfig;
-import org.joda.time.DateTimeZone;
 
 import java.util.ArrayList;
 import java.util.Collections;
@@ -47,7 +43,7 @@
  * }</pre>
  *
  *
- * The only publicly "consumable" API is {@link #translateAggregation(AggregationBuilder, List, NamedWriteableRegistry)}.
+ * The only publicly "consumable" API is {@link #translateAggregation(AggregationBuilder, NamedWriteableRegistry)}.
  */
 public class RollupRequestTranslator {
 
@@ -116,26 +112,22 @@ public class RollupRequestTranslator {
      * relevant method below.
      *
      * @param source           The source aggregation to translate into rollup-enabled version
-     * @param filterConditions A list used to track any filter conditions that sub-aggs may
-     *                         require.
      * @param registry  Registry containing the various aggregations so that we can easily
      *                  deserialize into a stream for cloning
      * @return  Returns the fully translated aggregation tree. Note that it returns a list instead
      *          of a single AggBuilder, since some aggregations (e.g. avg) may result in two
      *          translated aggs (sum + count)
      */
-    public static List<AggregationBuilder> translateAggregation(AggregationBuilder source,
-                                                                List<QueryBuilder> filterConditions,
-                                                                NamedWriteableRegistry registry) {
+    public static List<AggregationBuilder> translateAggregation(AggregationBuilder source, NamedWriteableRegistry registry) {
 
         if (source.getWriteableName().equals(DateHistogramAggregationBuilder.NAME)) {
-            return translateDateHistogram((DateHistogramAggregationBuilder) source, filterConditions, registry);
+            return translateDateHistogram((DateHistogramAggregationBuilder) source, registry);
         } else if (source.getWriteableName().equals(HistogramAggregationBuilder.NAME)) {
-            return translateHistogram((HistogramAggregationBuilder) source, filterConditions, registry);
+            return translateHistogram((HistogramAggregationBuilder) source, registry);
         } else if (RollupField.SUPPORTED_METRICS.contains(source.getWriteableName())) {
             return translateVSLeaf((ValuesSourceAggregationBuilder.LeafOnly)source, registry);
         } else if (source.getWriteableName().equals(TermsAggregationBuilder.NAME)) {
-            return translateTerms((TermsAggregationBuilder)source, filterConditions, registry);
+            return translateTerms((TermsAggregationBuilder)source, registry);
         } else {
             throw new IllegalArgumentException("Unable to translate aggregation tree into Rollup.  Aggregation ["
                     + source.getName() + "] is of type [" + source.getClass().getSimpleName() + "] which is " +
@@ -195,22 +187,13 @@ public static List<AggregationBuilder> translateAggregation(AggregationBuilder s
      *             <li>Field: `{timestamp field}.date_histogram._count`</li>
      *         </ul>
      *     </li>
-     *     <li>Add a filter condition:</li>
-     *     <li>
-     *         <ul>
-     *             <li>Query type: TermQuery</li>
-     *             <li>Field: `{timestamp_field}.date_histogram.interval`</li>
-     *             <li>Value: `{source interval}`</li>
-     *         </ul>
-     *     </li>
      * </ul>
      *
      */
     private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggregationBuilder source,
-                                                                   List<QueryBuilder> filterConditions,
                                                                    NamedWriteableRegistry registry) {
 
-        return translateVSAggBuilder(source, filterConditions, registry, () -> {
+        return translateVSAggBuilder(source, registry, () -> {
             DateHistogramAggregationBuilder rolledDateHisto
                     = new DateHistogramAggregationBuilder(source.getName());
 
@@ -220,10 +203,6 @@ private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggr
                 rolledDateHisto.interval(source.interval());
             }
 
-            String timezone = source.timeZone() == null ? DateTimeZone.UTC.toString() : source.timeZone().toString();
-            filterConditions.add(new TermQueryBuilder(RollupField.formatFieldName(source,
-                DateHistogramGroupConfig.TIME_ZONE), timezone));
-
             rolledDateHisto.offset(source.offset());
             if (source.extendedBounds() != null) {
                 rolledDateHisto.extendedBounds(source.extendedBounds());
@@ -245,14 +224,13 @@ private static List<AggregationBuilder> translateDateHistogram(DateHistogramAggr
      * Notably, it adds a Sum metric to calculate the doc_count in each bucket.
      *
      * Conventions are identical to a date_histogram (excepting date-specific details), so see
-     * {@link #translateDateHistogram(DateHistogramAggregationBuilder, List, NamedWriteableRegistry)} for
+     * {@link #translateDateHistogram(DateHistogramAggregationBuilder, NamedWriteableRegistry)} for
      * a complete list of conventions, examples, etc
      */
     private static List<AggregationBuilder> translateHistogram(HistogramAggregationBuilder source,
-                                                               List<QueryBuilder> filterConditions,
                                                                NamedWriteableRegistry registry) {
 
-        return translateVSAggBuilder(source, filterConditions, registry, () -> {
+        return translateVSAggBuilder(source, registry, () -> {
             HistogramAggregationBuilder rolledHisto
                     = new HistogramAggregationBuilder(source.getName());
 
@@ -325,10 +303,9 @@ private static List<AggregationBuilder> translateHistogram(HistogramAggregationB
      *
      */
     private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder source,
-                                                           List<QueryBuilder> filterConditions,
                                                            NamedWriteableRegistry registry) {
 
-        return translateVSAggBuilder(source, filterConditions, registry, () -> {
+        return translateVSAggBuilder(source, registry, () -> {
             TermsAggregationBuilder rolledTerms
                     = new TermsAggregationBuilder(source.getName(), source.valueType());
             rolledTerms.field(RollupField.formatFieldName(source, RollupField.VALUE));
@@ -356,8 +333,6 @@ private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder s
      * ValueSourceBuilder.  This method is called by all the agg-specific methods (e.g. translateDateHistogram())
      *
      * @param source The source aggregation that we wish to translate
-     * @param filterConditions A list of existing filter conditions, in case we need to add some
-     *                         for this particular agg
      * @param registry Named registry for serializing leaf metrics.  Not actually used by this method,
      *                 but is passed downwards for leaf usage
      * @param factory A factory closure that generates a new shallow clone of the `source`. E.g. if `source` is
@@ -368,15 +343,14 @@ private static List<AggregationBuilder> translateTerms(TermsAggregationBuilder s
      * @return the translated multi-bucket ValueSourceAggBuilder
      */
     private static <T extends ValuesSourceAggregationBuilder> List<AggregationBuilder>
-        translateVSAggBuilder(ValuesSourceAggregationBuilder source, List<QueryBuilder> filterConditions,
-                          NamedWriteableRegistry registry, Supplier<T> factory) {
+        translateVSAggBuilder(ValuesSourceAggregationBuilder source, NamedWriteableRegistry registry, Supplier<T> factory) {
 
         T rolled = factory.get();
 
         // Translate all subaggs and add to the newly translated agg
         // NOTE: using for loop instead of stream because compiler explodes with a bug :/
         for (AggregationBuilder subAgg : source.getSubAggregations()) {
-            List<AggregationBuilder> translated = translateAggregation(subAgg, filterConditions, registry);
+            List<AggregationBuilder> translated = translateAggregation(subAgg, registry);
             for (AggregationBuilder t : translated) {
                 rolled.subAggregation(t);
             }