Display errors for invalid timezones in TIME_FORMAT (#11423)

Users sometimes make typos when picking timezones - like `America/Los Angeles`
instead of `America/Los_Angeles` instead of defaulting to UTC, this change
makes it so that an error is thrown instead notifying the user of their mistake.

core/src/main/java/org/apache/druid/java/util/common/DateTimes.java

@@ -19,6 +19,7 @@
 
 package org.apache.druid.java.util.common;
 
+import com.google.common.collect.ImmutableSet;
 import io.netty.util.SuppressForbidden;
 import org.joda.time.Chronology;
 import org.joda.time.DateTime;
@@ -28,6 +29,7 @@ import org.joda.time.chrono.ISOChronology;
 import org.joda.time.format.DateTimeFormatter;
 import org.joda.time.format.ISODateTimeFormat;
 
+import java.util.Set;
 import java.util.TimeZone;
 import java.util.regex.Pattern;
 
@@ -43,6 +45,8 @@ public final class DateTimes
       ISODateTimeFormat.dateTimeParser().withOffsetParsed()
   );
 
+  private static final Set<String> AVAILABLE_TIMEZONE_IDS = ImmutableSet.copyOf(TimeZone.getAvailableIDs());
+
   /**
    * This pattern aims to match strings, produced by {@link DateTime#toString()}. It's not rigorous: it could accept
    * some strings that couldn't be obtained by calling toString() on any {@link DateTime} object, and also it could
@@ -53,14 +57,28 @@ public final class DateTimes
       "[0-9]{4}-[01][0-9]-[0-3][0-9]T[0-2][0-9]:[0-5][0-9]:[0-5][0-9]\\.[0-9]{3}(Z|[+\\-][0-9]{2}(:[0-9]{2}))"
   );
 
-  @SuppressForbidden(reason = "DateTimeZone#forID")
   public static DateTimeZone inferTzFromString(String tzId)
+  {
+    return inferTzFromString(tzId, true);
+  }
+
+  /**
+   * @return The dateTimezone for the provided {@param tzId}. If {@param fallback} is true, the default timezone
+   * will be returned if the tzId does not match a supported timezone.
+   * @throws IllegalArgumentException if {@param fallback} is false and the provided tzId doesn't match a supported
+   * timezone.
+   */
+  @SuppressForbidden(reason = "DateTimeZone#forID")
+  public static DateTimeZone inferTzFromString(String tzId, boolean fallback) throws IllegalArgumentException
   {
     try {
       return DateTimeZone.forID(tzId);
     }
     catch (IllegalArgumentException e) {
       // also support Java timezone strings
+      if (!fallback && !AVAILABLE_TIMEZONE_IDS.contains(tzId)) {
+        throw e;
+      }
       return DateTimeZone.forTimeZone(TimeZone.getTimeZone(tzId));
     }
   }

core/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java

@@ -24,6 +24,8 @@ import org.joda.time.DateTimeZone;
 import org.junit.Assert;
 import org.junit.Test;
 
+import java.util.TimeZone;
+
 public class DateTimesTest
 {
   @Test
@@ -33,11 +35,53 @@ public class DateTimesTest
     DateTime dt2 = new DateTime(System.currentTimeMillis(), DateTimes.inferTzFromString("IST"));
     DateTime dt3 = new DateTime(System.currentTimeMillis(), DateTimeZone.forOffsetHoursMinutes(1, 30));
 
-    for (DateTime dt : new DateTime[] {dt1, dt2, dt3}) {
+    for (DateTime dt : new DateTime[]{dt1, dt2, dt3}) {
       Assert.assertTrue(DateTimes.COMMON_DATE_TIME_PATTERN.matcher(dt.toString()).matches());
     }
   }
 
+  @Test
+  public void testinferTzFromStringWithKnownTzId()
+  {
+    Assert.assertEquals(DateTimeZone.UTC, DateTimes.inferTzFromString("UTC"));
+  }
+
+  @Test
+  public void testinferTzFromStringWithOffset()
+  {
+    Assert.assertEquals(DateTimeZone.forOffsetHoursMinutes(10, 30), DateTimes.inferTzFromString("+1030"));
+  }
+
+  @Test
+  public void testinferTzFromStringWithJavaTimeZone()
+  {
+    Assert.assertEquals(
+        DateTimeZone.forTimeZone(TimeZone.getTimeZone("ACT")),
+        DateTimes.inferTzFromString("ACT")
+    );
+  }
+
+  @Test
+  public void testinferTzFromStringWithJavaTimeZoneAndNoFallback()
+  {
+    Assert.assertEquals(
+        DateTimeZone.forTimeZone(TimeZone.getTimeZone("ACT")),
+        DateTimes.inferTzFromString("ACT", false)
+    );
+  }
+
+  @Test
+  public void testinferTzFromStringWithUnknownTimeZoneShouldReturnUTC()
+  {
+    Assert.assertEquals(DateTimeZone.UTC, DateTimes.inferTzFromString("America/Unknown"));
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void testinferTzFromStringWithUnknownTimeZoneAndNoFallbackShouldThrowException()
+  {
+    Assert.assertEquals(DateTimeZone.getDefault(), DateTimes.inferTzFromString("America/Unknown", false));
+  }
+
   @Test
   public void testStringToDateTimeConversion()
   {

sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFormatOperatorConversion.java

@@ -29,6 +29,7 @@ import org.apache.calcite.sql.SqlOperator;
 import org.apache.calcite.sql.type.SqlTypeFamily;
 import org.apache.calcite.sql.type.SqlTypeName;
 import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.IAE;
 import org.apache.druid.segment.column.RowSignature;
 import org.apache.druid.sql.calcite.expression.DruidExpression;
 import org.apache.druid.sql.calcite.expression.Expressions;
@@ -81,7 +82,14 @@ public class TimeFormatOperatorConversion implements SqlOperatorConversion
     final DateTimeZone timeZone = OperatorConversions.getOperandWithDefault(
         call.getOperands(),
         2,
-        operand -> DateTimes.inferTzFromString(RexLiteral.stringValue(operand)),
+        operand -> {
+          try {
+            return DateTimes.inferTzFromString(RexLiteral.stringValue(operand), false);
+          }
+          catch (IllegalArgumentException e) {
+            throw new IAE(e.getMessage());
+          }
+        },
         plannerContext.getTimeZone()
     );
 

sql/src/test/java/org/apache/druid/sql/calcite/expression/TimeFormatOperatorConversionTest.java

@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.calcite.expression;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
+import org.apache.calcite.rex.RexNode;
+import org.apache.druid.java.util.common.DateTimes;
+import org.apache.druid.java.util.common.IAE;
+import org.apache.druid.segment.column.RowSignature;
+import org.apache.druid.segment.column.ValueType;
+import org.apache.druid.sql.calcite.expression.builtin.TimeFormatOperatorConversion;
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.annotation.Nullable;
+import java.util.Map;
+
+/**
+ * Tests for TIME_FORMAT
+ */
+public class TimeFormatOperatorConversionTest extends ExpressionTestBase
+{
+  private static final RowSignature ROW_SIGNATURE = RowSignature
+      .builder()
+      .add("t", ValueType.LONG)
+      .build();
+  private static final Map<String, Object> BINDINGS = ImmutableMap
+      .<String, Object>builder()
+      .put("t", DateTimes.of("2000-02-03T04:05:06").getMillis())
+      .build();
+
+  private TimeFormatOperatorConversion target;
+  private ExpressionTestHelper testHelper;
+
+  @Before
+  public void setUp()
+  {
+    target = new TimeFormatOperatorConversion();
+    testHelper = new ExpressionTestHelper(ROW_SIGNATURE, BINDINGS);
+  }
+
+  @Test
+  public void testConversionToUTC()
+  {
+    testExpression(
+        "2000-02-03 04:05:06",
+        "timestamp_format(\"t\",'yyyy-MM-dd HH:mm:ss','UTC')",
+        "yyyy-MM-dd HH:mm:ss",
+        "UTC"
+    );
+  }
+
+  @Test
+  public void testConversionWithDefaultShouldUseUTC()
+  {
+    testExpression(
+        "2000-02-03 04:05:06",
+        "timestamp_format(\"t\",'yyyy-MM-dd HH:mm:ss','UTC')",
+        "yyyy-MM-dd HH:mm:ss",
+        null
+    );
+  }
+
+  @Test
+  public void testConversionToTimezone()
+  {
+    testExpression(
+        "2000-02-02 20:05:06",
+        "timestamp_format(\"t\",'yyyy-MM-dd HH:mm:ss','America/Los_Angeles')",
+        "yyyy-MM-dd HH:mm:ss",
+        "America/Los_Angeles"
+    );
+  }
+
+  @Test(expected = IAE.class)
+  public void testConversionToUnknownTimezoneShouldThrowException()
+  {
+    testExpression(
+        "2000-02-02 20:05:06",
+        "timestamp_format(\"t\",'yyyy-MM-dd HH:mm:ss','America/NO_TZ')",
+        "yyyy-MM-dd HH:mm:ss",
+        "America/NO_TZ"
+    );
+  }
+
+  private void testExpression(
+      String expectedResult,
+      String expectedExpression,
+      String format,
+      @Nullable String timezone
+  )
+  {
+    ImmutableList.Builder<RexNode> exprsBuilder = ImmutableList
+        .<RexNode>builder()
+        .add(testHelper.makeInputRef("t"))
+        .add(testHelper.makeLiteral(format));
+    if (timezone != null) {
+      exprsBuilder.add(testHelper.makeLiteral(timezone));
+    }
+
+    testHelper.testExpression(
+        target.calciteOperator(),
+        exprsBuilder.build(),
+        DruidExpression.fromExpression(expectedExpression),
+        expectedResult
+    );
+  }
+}