From 37b020780f6b723b6ad739d495423f788bed20bc Mon Sep 17 00:00:00 2001 From: takezoe Date: Sun, 11 Aug 2024 12:26:14 +0900 Subject: [PATCH 1/2] Improve messages for INTERVAL literal semantic errors --- .../sql/analyzer/ExpressionAnalyzer.java | 3 + .../java/io/trino/util/DateTimeUtils.java | 65 ++++++++--------- .../scalar/interval/TestIntervalDayTime.java | 69 +++++++++++++++++-- .../interval/TestIntervalYearMonth.java | 46 +++++++++++-- 4 files changed, 141 insertions(+), 42 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java b/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java index 27da38e1a50cc..c36ac0478a58f 100644 --- a/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java +++ b/core/trino-main/src/main/java/io/trino/sql/analyzer/ExpressionAnalyzer.java @@ -1259,6 +1259,9 @@ protected Type visitIntervalLiteral(IntervalLiteral node, Context context) try { literalInterpreter.evaluate(node, type); } + catch (TrinoException e) { + throw new TrinoException(e::getErrorCode, extractLocation(node), e.getMessage(), e); + } catch (RuntimeException e) { throw semanticException(INVALID_LITERAL, node, e, "'%s' is not a valid INTERVAL literal", node.getValue()); } diff --git a/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java b/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java index 6ff34ff15c20a..0ad21a79d984e 100644 --- a/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java +++ b/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java @@ -44,7 +44,7 @@ import java.util.stream.Stream; import static com.google.common.base.Preconditions.checkArgument; -import static io.trino.spi.StandardErrorCode.INVALID_FUNCTION_ARGUMENT; +import static io.trino.spi.StandardErrorCode.INVALID_LITERAL; import static io.trino.sql.tree.IntervalLiteral.IntervalField; import static io.trino.util.DateTimeZoneIndex.getChronology; import static io.trino.util.DateTimeZoneIndex.packDateTimeWithZone; @@ -212,48 +212,46 @@ public static long convertToTimestampWithTimeZone(TimeZoneKey timeZoneKey, Strin public static long parseDayTimeInterval(String value, IntervalField startField, Optional endField) { - IntervalField end = endField.orElse(startField); - try { - if (startField == IntervalField.DAY && end == IntervalField.SECOND) { + if (startField == IntervalField.DAY && endField.isEmpty()) { + return parsePeriodMillis(INTERVAL_DAY_FORMATTER, value); + } + if (startField == IntervalField.DAY && endField.get() == IntervalField.SECOND) { return parsePeriodMillis(INTERVAL_DAY_SECOND_FORMATTER, value); } - if (startField == IntervalField.DAY && end == IntervalField.MINUTE) { + if (startField == IntervalField.DAY && endField.get() == IntervalField.MINUTE) { return parsePeriodMillis(INTERVAL_DAY_MINUTE_FORMATTER, value); } - if (startField == IntervalField.DAY && end == IntervalField.HOUR) { + if (startField == IntervalField.DAY && endField.get() == IntervalField.HOUR) { return parsePeriodMillis(INTERVAL_DAY_HOUR_FORMATTER, value); } - if (startField == IntervalField.DAY && end == IntervalField.DAY) { - return parsePeriodMillis(INTERVAL_DAY_FORMATTER, value); - } - if (startField == IntervalField.HOUR && end == IntervalField.SECOND) { + if (startField == IntervalField.HOUR && endField.isEmpty()) { + return parsePeriodMillis(INTERVAL_HOUR_FORMATTER, value); + } + if (startField == IntervalField.HOUR && endField.get() == IntervalField.SECOND) { return parsePeriodMillis(INTERVAL_HOUR_SECOND_FORMATTER, value); } - if (startField == IntervalField.HOUR && end == IntervalField.MINUTE) { + if (startField == IntervalField.HOUR && endField.get() == IntervalField.MINUTE) { return parsePeriodMillis(INTERVAL_HOUR_MINUTE_FORMATTER, value); } - if (startField == IntervalField.HOUR && end == IntervalField.HOUR) { - return parsePeriodMillis(INTERVAL_HOUR_FORMATTER, value); - } - if (startField == IntervalField.MINUTE && end == IntervalField.SECOND) { - return parsePeriodMillis(INTERVAL_MINUTE_SECOND_FORMATTER, value); - } - if (startField == IntervalField.MINUTE && end == IntervalField.MINUTE) { + if (startField == IntervalField.MINUTE && endField.isEmpty()) { return parsePeriodMillis(INTERVAL_MINUTE_FORMATTER, value); } + if (startField == IntervalField.MINUTE && endField.get() == IntervalField.SECOND) { + return parsePeriodMillis(INTERVAL_MINUTE_SECOND_FORMATTER, value); + } - if (startField == IntervalField.SECOND && end == IntervalField.SECOND) { + if (startField == IntervalField.SECOND && endField.isEmpty()) { return parsePeriodMillis(INTERVAL_SECOND_FORMATTER, value); } } catch (IllegalArgumentException e) { - throw invalidInterval(e, value, startField, end); + throw invalidInterval(e, value, startField, endField.orElse(startField)); } - throw new IllegalArgumentException("Invalid day second interval qualifier: " + startField + " to " + end); + throw invalidQualifier(startField, endField.orElse(startField)); } private static long parsePeriodMillis(PeriodFormatter periodFormatter, String value) @@ -269,25 +267,23 @@ private static long parsePeriodMillis(PeriodFormatter periodFormatter, String va public static long parseYearMonthInterval(String value, IntervalField startField, Optional endField) { - IntervalField end = endField.orElse(startField); - try { - if (startField == IntervalField.YEAR && end == IntervalField.MONTH) { - return parsePeriodMonths(value, INTERVAL_YEAR_MONTH_FORMATTER); - } - if (startField == IntervalField.YEAR && end == IntervalField.YEAR) { + if (startField == IntervalField.YEAR && endField.isEmpty()) { return parsePeriodMonths(value, INTERVAL_YEAR_FORMATTER); } + if (startField == IntervalField.YEAR && endField.get() == IntervalField.MONTH) { + return parsePeriodMonths(value, INTERVAL_YEAR_MONTH_FORMATTER); + } - if (startField == IntervalField.MONTH && end == IntervalField.MONTH) { + if (startField == IntervalField.MONTH && endField.isEmpty()) { return parsePeriodMonths(value, INTERVAL_MONTH_FORMATTER); } } catch (IllegalArgumentException e) { - throw invalidInterval(e, value, startField, end); + throw invalidInterval(e, value, startField, endField.orElse(startField)); } - throw new IllegalArgumentException("Invalid year month interval qualifier: " + startField + " to " + end); + throw invalidQualifier(startField, endField.orElse(startField)); } private static long parsePeriodMonths(String value, PeriodFormatter periodFormatter) @@ -325,7 +321,12 @@ private static TrinoException invalidInterval(Throwable throwable, String value, else { message = format("Invalid INTERVAL %s TO %s value: %s", startField, endField, value); } - return new TrinoException(INVALID_FUNCTION_ARGUMENT, message, throwable); + return new TrinoException(INVALID_LITERAL, message, throwable); + } + + private static TrinoException invalidQualifier(IntervalField startField, IntervalField endField) + { + throw new TrinoException(INVALID_LITERAL, "Invalid interval qualifier: " + startField + " TO " + endField); } private static PeriodFormatter cretePeriodFormatter(IntervalField startField, IntervalField endField) @@ -351,7 +352,7 @@ private static PeriodFormatter cretePeriodFormatter(IntervalField startField, In builder.appendMonths(); parsers.add(builder.toParser()); if (endField != IntervalField.MONTH) { - throw new IllegalArgumentException("Invalid interval qualifier: " + startField + " to " + endField); + throw invalidQualifier(startField, endField); } break; diff --git a/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalDayTime.java b/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalDayTime.java index 15ae5b26bce07..9e74e2e353564 100644 --- a/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalDayTime.java +++ b/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalDayTime.java @@ -138,20 +138,79 @@ public void testLiterals() assertThat(assertions.expression("INTERVAL '32' SECOND")) .isEqualTo(interval(0, 0, 0, 32, 0)); + // Invalid literals assertThatThrownBy(assertions.expression("INTERVAL '12X' DAY")::evaluate) - .hasMessage("line 1:12: '12X' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL DAY value: 12X"); assertThatThrownBy(assertions.expression("INTERVAL '12 10' DAY")::evaluate) - .hasMessage("line 1:12: '12 10' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL DAY value: 12 10"); assertThatThrownBy(assertions.expression("INTERVAL '12 X' DAY TO HOUR")::evaluate) - .hasMessage("line 1:12: '12 X' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL DAY TO HOUR value: 12 X"); assertThatThrownBy(assertions.expression("INTERVAL '12 -10' DAY TO HOUR")::evaluate) - .hasMessage("line 1:12: '12 -10' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL DAY TO HOUR value: 12 -10"); assertThatThrownBy(assertions.expression("INTERVAL '--12 -10' DAY TO HOUR")::evaluate) - .hasMessage("line 1:12: '--12 -10' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL DAY TO HOUR value: --12 -10"); + + // Invalid qualifiers (DAY TO xxx) + assertThatThrownBy(assertions.expression("INTERVAL '12' DAY TO DAY")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: DAY TO DAY"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' DAY TO YEAR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: DAY TO YEAR"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' DAY TO MONTH")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: DAY TO MONTH"); + + // Invalid qualifiers (HOUR TO xxx) + assertThatThrownBy(assertions.expression("INTERVAL '10' HOUR TO HOUR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: HOUR TO HOUR"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' HOUR TO YEAR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: HOUR TO YEAR"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' HOUR TO MONTH")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: HOUR TO MONTH"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' HOUR TO DAY")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: HOUR TO DAY"); + + // Invalid qualifiers (MINUTE TO xxx) + assertThatThrownBy(assertions.expression("INTERVAL '45' MINUTE TO MINUTE")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MINUTE TO MINUTE"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' MINUTE TO YEAR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MINUTE TO YEAR"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' MINUTE TO MONTH")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MINUTE TO MONTH"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' MINUTE TO DAY")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MINUTE TO DAY"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' MINUTE TO HOUR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MINUTE TO HOUR"); + + // Invalid qualifiers (SECOND TO xxx) + assertThatThrownBy(assertions.expression("INTERVAL '32' SECOND TO SECOND")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: SECOND TO SECOND"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' SECOND TO YEAR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: SECOND TO YEAR"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' SECOND TO MONTH")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: SECOND TO MONTH"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' SECOND TO DAY")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: SECOND TO DAY"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' SECOND TO HOUR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: SECOND TO HOUR"); + + assertThatThrownBy(assertions.expression("INTERVAL '12-10' SECOND TO MINUTE")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: SECOND TO MINUTE"); } private static SqlIntervalDayTime interval(int day, int hour, int minute, int second, int milliseconds) diff --git a/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalYearMonth.java b/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalYearMonth.java index e0fe9c19bd6d0..6224dd0263aea 100644 --- a/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalYearMonth.java +++ b/core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalYearMonth.java @@ -66,20 +66,56 @@ public void testLiterals() assertThat(assertions.expression("INTERVAL '32767-32767' YEAR TO MONTH")) .isEqualTo(interval(32767, 32767)); + // Invalid literals assertThatThrownBy(assertions.expression("INTERVAL '124X' YEAR")::evaluate) - .hasMessage("line 1:12: '124X' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL YEAR value: 124X"); assertThatThrownBy(assertions.expression("INTERVAL '124-30' YEAR")::evaluate) - .hasMessage("line 1:12: '124-30' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL YEAR value: 124-30"); assertThatThrownBy(assertions.expression("INTERVAL '124-X' YEAR TO MONTH")::evaluate) - .hasMessage("line 1:12: '124-X' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL YEAR TO MONTH value: 124-X"); assertThatThrownBy(assertions.expression("INTERVAL '124--30' YEAR TO MONTH")::evaluate) - .hasMessage("line 1:12: '124--30' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL YEAR TO MONTH value: 124--30"); assertThatThrownBy(assertions.expression("INTERVAL '--124--30' YEAR TO MONTH")::evaluate) - .hasMessage("line 1:12: '--124--30' is not a valid INTERVAL literal"); + .hasMessage("line 1:12: Invalid INTERVAL YEAR TO MONTH value: --124--30"); + + // Invalid qualifiers (YEAR TO xxx) + assertThatThrownBy(assertions.expression("INTERVAL '32767' YEAR TO YEAR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: YEAR TO YEAR"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' YEAR TO DAY")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: YEAR TO DAY"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' YEAR TO HOUR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: YEAR TO HOUR"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' YEAR TO MINUTE")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: YEAR TO MINUTE"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' YEAR TO SECOND")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: YEAR TO SECOND"); + + // Invalid qualifiers (MONTH TO xxx) + assertThatThrownBy(assertions.expression("INTERVAL '30' MONTH TO MONTH")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MONTH TO MONTH"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' MONTH TO YEAR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MONTH TO YEAR"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' MONTH TO DAY")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MONTH TO DAY"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' MONTH TO HOUR")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MONTH TO HOUR"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' MONTH TO MINUTE")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MONTH TO MINUTE"); + + assertThatThrownBy(assertions.expression("INTERVAL '124-30' MONTH TO SECOND")::evaluate) + .hasMessage("line 1:12: Invalid interval qualifier: MONTH TO SECOND"); } private static SqlIntervalYearMonth interval(int year, int month) From 6a6a3ac480178f780e9e815e661b86c239f159eb Mon Sep 17 00:00:00 2001 From: takezoe Date: Sat, 28 Sep 2024 11:15:04 +0900 Subject: [PATCH 2/2] Fix typo in DateTimeUtils --- .../java/io/trino/util/DateTimeUtils.java | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java b/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java index 0ad21a79d984e..b364630f4a4e9 100644 --- a/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java +++ b/core/trino-main/src/main/java/io/trino/util/DateTimeUtils.java @@ -191,24 +191,24 @@ public static long convertToTimestampWithTimeZone(TimeZoneKey timeZoneKey, Strin private static final int SECOND_FIELD = 6; private static final int MILLIS_FIELD = 7; - private static final PeriodFormatter INTERVAL_DAY_SECOND_FORMATTER = cretePeriodFormatter(IntervalField.DAY, IntervalField.SECOND); - private static final PeriodFormatter INTERVAL_DAY_MINUTE_FORMATTER = cretePeriodFormatter(IntervalField.DAY, IntervalField.MINUTE); - private static final PeriodFormatter INTERVAL_DAY_HOUR_FORMATTER = cretePeriodFormatter(IntervalField.DAY, IntervalField.HOUR); - private static final PeriodFormatter INTERVAL_DAY_FORMATTER = cretePeriodFormatter(IntervalField.DAY, IntervalField.DAY); + private static final PeriodFormatter INTERVAL_DAY_SECOND_FORMATTER = createPeriodFormatter(IntervalField.DAY, IntervalField.SECOND); + private static final PeriodFormatter INTERVAL_DAY_MINUTE_FORMATTER = createPeriodFormatter(IntervalField.DAY, IntervalField.MINUTE); + private static final PeriodFormatter INTERVAL_DAY_HOUR_FORMATTER = createPeriodFormatter(IntervalField.DAY, IntervalField.HOUR); + private static final PeriodFormatter INTERVAL_DAY_FORMATTER = createPeriodFormatter(IntervalField.DAY, IntervalField.DAY); - private static final PeriodFormatter INTERVAL_HOUR_SECOND_FORMATTER = cretePeriodFormatter(IntervalField.HOUR, IntervalField.SECOND); - private static final PeriodFormatter INTERVAL_HOUR_MINUTE_FORMATTER = cretePeriodFormatter(IntervalField.HOUR, IntervalField.MINUTE); - private static final PeriodFormatter INTERVAL_HOUR_FORMATTER = cretePeriodFormatter(IntervalField.HOUR, IntervalField.HOUR); + private static final PeriodFormatter INTERVAL_HOUR_SECOND_FORMATTER = createPeriodFormatter(IntervalField.HOUR, IntervalField.SECOND); + private static final PeriodFormatter INTERVAL_HOUR_MINUTE_FORMATTER = createPeriodFormatter(IntervalField.HOUR, IntervalField.MINUTE); + private static final PeriodFormatter INTERVAL_HOUR_FORMATTER = createPeriodFormatter(IntervalField.HOUR, IntervalField.HOUR); - private static final PeriodFormatter INTERVAL_MINUTE_SECOND_FORMATTER = cretePeriodFormatter(IntervalField.MINUTE, IntervalField.SECOND); - private static final PeriodFormatter INTERVAL_MINUTE_FORMATTER = cretePeriodFormatter(IntervalField.MINUTE, IntervalField.MINUTE); + private static final PeriodFormatter INTERVAL_MINUTE_SECOND_FORMATTER = createPeriodFormatter(IntervalField.MINUTE, IntervalField.SECOND); + private static final PeriodFormatter INTERVAL_MINUTE_FORMATTER = createPeriodFormatter(IntervalField.MINUTE, IntervalField.MINUTE); - private static final PeriodFormatter INTERVAL_SECOND_FORMATTER = cretePeriodFormatter(IntervalField.SECOND, IntervalField.SECOND); + private static final PeriodFormatter INTERVAL_SECOND_FORMATTER = createPeriodFormatter(IntervalField.SECOND, IntervalField.SECOND); - private static final PeriodFormatter INTERVAL_YEAR_MONTH_FORMATTER = cretePeriodFormatter(IntervalField.YEAR, IntervalField.MONTH); - private static final PeriodFormatter INTERVAL_YEAR_FORMATTER = cretePeriodFormatter(IntervalField.YEAR, IntervalField.YEAR); + private static final PeriodFormatter INTERVAL_YEAR_MONTH_FORMATTER = createPeriodFormatter(IntervalField.YEAR, IntervalField.MONTH); + private static final PeriodFormatter INTERVAL_YEAR_FORMATTER = createPeriodFormatter(IntervalField.YEAR, IntervalField.YEAR); - private static final PeriodFormatter INTERVAL_MONTH_FORMATTER = cretePeriodFormatter(IntervalField.MONTH, IntervalField.MONTH); + private static final PeriodFormatter INTERVAL_MONTH_FORMATTER = createPeriodFormatter(IntervalField.MONTH, IntervalField.MONTH); public static long parseDayTimeInterval(String value, IntervalField startField, Optional endField) { @@ -329,7 +329,7 @@ private static TrinoException invalidQualifier(IntervalField startField, Interva throw new TrinoException(INVALID_LITERAL, "Invalid interval qualifier: " + startField + " TO " + endField); } - private static PeriodFormatter cretePeriodFormatter(IntervalField startField, IntervalField endField) + private static PeriodFormatter createPeriodFormatter(IntervalField startField, IntervalField endField) { if (endField == null) { endField = startField;