From 49e8732e4fa8756c3c0e8f1d4e00a8bb9eba81fd Mon Sep 17 00:00:00 2001 From: Suneet Saldanha Date: Fri, 9 Jul 2021 06:07:13 -0700 Subject: [PATCH] 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. --- .../druid/java/util/common/DateTimes.java | 20 ++- .../druid/java/util/common/DateTimesTest.java | 46 ++++++- .../builtin/TimeFormatOperatorConversion.java | 10 +- .../TimeFormatOperatorConversionTest.java | 126 ++++++++++++++++++ 4 files changed, 199 insertions(+), 3 deletions(-) create mode 100644 sql/src/test/java/org/apache/druid/sql/calcite/expression/TimeFormatOperatorConversionTest.java 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 c1718bd1d78..09ac52279c4 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.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 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)); } } 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 15f30336c85..fd3d1ab8c97 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.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() { 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 e44734f84ba..8afeb803181 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.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() ); 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 00000000000..36a8bb05ace --- /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 BINDINGS = ImmutableMap + .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 exprsBuilder = ImmutableList + .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 + ); + } +}