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.
diff --git a/core/src/main/java/org/apache/druid/java/util/common/DateTimes.java b/core/src/main/java/org/apache/druid/java/util/common/DateTimes.java
index c1718bd..09ac522 100644
--- a/core/src/main/java/org/apache/druid/java/util/common/DateTimes.java
+++ b/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.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 @@
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 @@
"[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));
}
}
diff --git a/core/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java b/core/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java
index 15f3033..fd3d1ab 100644
--- a/core/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java
+++ b/core/src/test/java/org/apache/druid/java/util/common/DateTimesTest.java
@@ -24,6 +24,8 @@
import org.junit.Assert;
import org.junit.Test;
+import java.util.TimeZone;
+
public class DateTimesTest
{
@Test
@@ -33,12 +35,54 @@
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()
{
String seconds = "2018-01-30T06:00:00";
diff --git a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFormatOperatorConversion.java b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFormatOperatorConversion.java
index e44734f..8afeb80 100644
--- a/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFormatOperatorConversion.java
+++ b/sql/src/main/java/org/apache/druid/sql/calcite/expression/builtin/TimeFormatOperatorConversion.java
@@ -29,6 +29,7 @@
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 @@
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()
);
diff --git a/sql/src/test/java/org/apache/druid/sql/calcite/expression/TimeFormatOperatorConversionTest.java b/sql/src/test/java/org/apache/druid/sql/calcite/expression/TimeFormatOperatorConversionTest.java
new file mode 100644
index 0000000..36a8bb0
--- /dev/null
+++ b/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
+ );
+ }
+}
