From 5891dec9dcf1b2012eab325f29992f152c066b67 Mon Sep 17 00:00:00 2001 From: kasiafi <30203062+kasiafi@users.noreply.github.com> Date: Tue, 18 Jan 2022 15:44:09 +0100 Subject: [PATCH 1/5] Extract method for processing input string in test --- .../iterative/rule/TestSimplifyExpressions.java | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java index 123e80ad1d28..caef4dcb197f 100644 --- a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java +++ b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java @@ -244,13 +244,16 @@ public void testCastRealToBoundedVarchar() private static void assertSimplifies(String expression, String expected) { - ParsingOptions parsingOptions = new ParsingOptions(); - Expression actualExpression = rewriteIdentifiersToSymbolReferences(SQL_PARSER.createExpression(expression, parsingOptions)); - Expression expectedExpression = rewriteIdentifiersToSymbolReferences(SQL_PARSER.createExpression(expected, parsingOptions)); - Expression rewritten = rewrite(actualExpression, TEST_SESSION, new SymbolAllocator(booleanSymbolTypeMapFor(actualExpression)), PLANNER_CONTEXT, createTestingTypeAnalyzer(PLANNER_CONTEXT)); + Expression expectedExpression = normalize(rewriteIdentifiersToSymbolReferences(SQL_PARSER.createExpression(expected, new ParsingOptions()))); assertEquals( - normalize(rewritten), - normalize(expectedExpression)); + simplify(expression), + expectedExpression); + } + + private static Expression simplify(String expression) + { + Expression actualExpression = rewriteIdentifiersToSymbolReferences(SQL_PARSER.createExpression(expression, new ParsingOptions())); + return normalize(rewrite(actualExpression, TEST_SESSION, new SymbolAllocator(booleanSymbolTypeMapFor(actualExpression)), PLANNER_CONTEXT, createTestingTypeAnalyzer(PLANNER_CONTEXT))); } private static Map booleanSymbolTypeMapFor(Expression expression) From 168bc0f8f62153c265817039eeefa0d7e5c3c3b9 Mon Sep 17 00:00:00 2001 From: kasiafi <30203062+kasiafi@users.noreply.github.com> Date: Tue, 18 Jan 2022 15:46:42 +0100 Subject: [PATCH 2/5] Test casts of date to bounded varchar --- .../trino/sql/TestExpressionInterpreter.java | 11 +++++++++++ .../rule/TestSimplifyExpressions.java | 18 ++++++++++++++++++ .../io/trino/type/TestDateTimeOperators.java | 13 +++++++++++++ 3 files changed, 42 insertions(+) diff --git a/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java b/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java index 9e766646e332..13a3b6d32e16 100644 --- a/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java +++ b/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java @@ -833,6 +833,17 @@ public void testCastRealToBoundedVarchar() .hasMessage("Value 1200000.0 cannot be represented as varchar(5)"); } + @Test + public void testCastDateToBoundedVarchar() + { + assertEvaluatedEquals("CAST(DATE '2013-02-02' AS varchar(10))", "'2013-02-02'"); + assertEvaluatedEquals("CAST(DATE '-2013-02-02' AS varchar(50))", "'-2013-02-02'"); + + // incorrect behavior: the result value does not fit in the type + assertEvaluatedEquals("CAST(DATE '2013-02-02' AS varchar(9))", "'2013-02-02'"); + assertEvaluatedEquals("CAST(DATE '-2013-02-02' AS varchar(9))", "'-2013-02-02'"); + } + @Test public void testCastToBoolean() { diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java index caef4dcb197f..e0bbb653f177 100644 --- a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java +++ b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java @@ -44,6 +44,7 @@ import static io.trino.sql.planner.TypeAnalyzer.createTestingTypeAnalyzer; import static io.trino.sql.planner.iterative.rule.SimplifyExpressions.rewrite; import static java.util.stream.Collectors.toList; +import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.testng.Assert.assertEquals; public class TestSimplifyExpressions @@ -242,6 +243,23 @@ public void testCastRealToBoundedVarchar() assertSimplifies("CAST(REAL '12e2' AS varchar(3)) = '1200.0'", "CAST(REAL '12e2' AS varchar(3)) = '1200.0'"); } + @Test + public void testCastDateToBoundedVarchar() + { + // the varchar type length is enough to contain the date's representation + assertSimplifies("CAST(DATE '2013-02-02' AS varchar(10))", "'2013-02-02'"); + assertSimplifies("CAST(DATE '2013-02-02' AS varchar(50))", "CAST('2013-02-02' AS varchar(50))"); + + // the cast operator returns a value that is too long for the expected type ('2013-02-02' for varchar(3)) + // the LiteralEncoder detects the mismatch and fails + assertThatThrownBy(() -> simplify("CAST(DATE '2013-02-02' AS varchar(3))")) + .hasMessage("Value [2013-02-02] does not fit in type varchar(3)"); + + // the cast operator returns a value that is too long for the expected type ('2013-02-02' for varchar(3)) + // the value is nested in a comparison expression, so the mismatch is not detected by the LiteralEncoder + assertSimplifies("CAST(DATE '2013-02-02' AS varchar(3)) = '2013-02-02'", "true"); + } + private static void assertSimplifies(String expression, String expected) { Expression expectedExpression = normalize(rewriteIdentifiersToSymbolReferences(SQL_PARSER.createExpression(expected, new ParsingOptions()))); diff --git a/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java b/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java index e2a07063083d..49298c7593fb 100644 --- a/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java +++ b/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java @@ -32,6 +32,7 @@ import static io.trino.spi.type.TimestampType.createTimestampType; import static io.trino.spi.type.TimestampWithTimeZoneType.TIMESTAMP_TZ_MILLIS; import static io.trino.spi.type.VarcharType.VARCHAR; +import static io.trino.spi.type.VarcharType.createVarcharType; import static io.trino.testing.DateTimeTestingUtils.sqlTimestampOf; import static io.trino.testing.TestingSession.testSessionBuilder; import static org.joda.time.DateTimeZone.UTC; @@ -264,6 +265,18 @@ public void testDateCastFromVarchar() assertInvalidFunction("DATE '392251590-07-12'", INVALID_CAST_ARGUMENT, "Value cannot be cast to date: 392251590-07-12"); } + @Test + public void testDateCastToVarchar() + { + assertFunction("cast(DATE '2013-02-02' AS varchar)", VARCHAR, "2013-02-02"); + assertFunction("cast(DATE '13-2-2' AS varchar)", VARCHAR, "0013-02-02"); + assertFunction("cast(DATE '2013-02-02' AS varchar(50))", createVarcharType(50), "2013-02-02"); + assertFunction("cast(DATE '2013-02-02' AS varchar(10))", createVarcharType(10), "2013-02-02"); + + // cast operator returns a value that does not fit in the result type. this causes error in the LiteralEncoder + assertFunctionThrowsIncorrectly("cast(DATE '2013-02-02' AS varchar(9))", IllegalArgumentException.class, "Value .2013-02-02. does not fit in type varchar.9."); + } + private static SqlDate toDate(DateTime dateTime) { return new SqlDate((int) TimeUnit.MILLISECONDS.toDays(dateTime.getMillis())); From 3701f925dec9e8969e2f33e8cf0c8ef80b394108 Mon Sep 17 00:00:00 2001 From: kasiafi <30203062+kasiafi@users.noreply.github.com> Date: Wed, 19 Jan 2022 09:52:13 +0100 Subject: [PATCH 3/5] Fix cast from date to varchar Fail if the resulting string does not fit in the bounded length of the varchar type. --- .../src/main/java/io/trino/type/DateOperators.java | 12 ++++++++++-- .../java/io/trino/sql/TestExpressionInterpreter.java | 10 +++++++--- .../iterative/rule/TestSimplifyExpressions.java | 12 +++--------- .../java/io/trino/type/TestDateTimeOperators.java | 4 +--- 4 files changed, 21 insertions(+), 17 deletions(-) diff --git a/core/trino-main/src/main/java/io/trino/type/DateOperators.java b/core/trino-main/src/main/java/io/trino/type/DateOperators.java index 309c10f2dce4..2779a2a5a6c2 100644 --- a/core/trino-main/src/main/java/io/trino/type/DateOperators.java +++ b/core/trino-main/src/main/java/io/trino/type/DateOperators.java @@ -15,6 +15,7 @@ import io.airlift.slice.Slice; import io.trino.spi.TrinoException; +import io.trino.spi.function.LiteralParameter; import io.trino.spi.function.LiteralParameters; import io.trino.spi.function.ScalarFunction; import io.trino.spi.function.ScalarOperator; @@ -27,6 +28,7 @@ import static io.trino.spi.function.OperatorType.CAST; import static io.trino.util.DateTimeUtils.parseDate; import static io.trino.util.DateTimeUtils.printDate; +import static java.lang.String.format; public final class DateOperators { @@ -35,9 +37,15 @@ private DateOperators() {} @ScalarOperator(CAST) @LiteralParameters("x") @SqlType("varchar(x)") - public static Slice castToSlice(@SqlType(StandardTypes.DATE) long value) + public static Slice castToSlice(@LiteralParameter("x") long x, @SqlType(StandardTypes.DATE) long value) { - return utf8Slice(printDate((int) value)); + String stringValue = printDate((int) value); + // String is all-ASCII, so String.length() here returns actual code points count + if (stringValue.length() <= x) { + return utf8Slice(stringValue); + } + + throw new TrinoException(INVALID_CAST_ARGUMENT, format("Value %s cannot be represented as varchar(%s)", stringValue, x)); } @ScalarFunction("date") diff --git a/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java b/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java index 13a3b6d32e16..990cc7df39c6 100644 --- a/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java +++ b/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java @@ -839,9 +839,13 @@ public void testCastDateToBoundedVarchar() assertEvaluatedEquals("CAST(DATE '2013-02-02' AS varchar(10))", "'2013-02-02'"); assertEvaluatedEquals("CAST(DATE '-2013-02-02' AS varchar(50))", "'-2013-02-02'"); - // incorrect behavior: the result value does not fit in the type - assertEvaluatedEquals("CAST(DATE '2013-02-02' AS varchar(9))", "'2013-02-02'"); - assertEvaluatedEquals("CAST(DATE '-2013-02-02' AS varchar(9))", "'-2013-02-02'"); + // the result value does not fit in the type + assertTrinoExceptionThrownBy(() -> evaluate("CAST(DATE '2013-02-02' AS varchar(9))")) + .hasErrorCode(INVALID_CAST_ARGUMENT) + .hasMessage("Value 2013-02-02 cannot be represented as varchar(9)"); + assertTrinoExceptionThrownBy(() -> evaluate("CAST(DATE '-2013-02-02' AS varchar(9))")) + .hasErrorCode(INVALID_CAST_ARGUMENT) + .hasMessage("Value -2013-02-02 cannot be represented as varchar(9)"); } @Test diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java index e0bbb653f177..ba22c4ec617c 100644 --- a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java +++ b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java @@ -44,7 +44,6 @@ import static io.trino.sql.planner.TypeAnalyzer.createTestingTypeAnalyzer; import static io.trino.sql.planner.iterative.rule.SimplifyExpressions.rewrite; import static java.util.stream.Collectors.toList; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.testng.Assert.assertEquals; public class TestSimplifyExpressions @@ -250,14 +249,9 @@ public void testCastDateToBoundedVarchar() assertSimplifies("CAST(DATE '2013-02-02' AS varchar(10))", "'2013-02-02'"); assertSimplifies("CAST(DATE '2013-02-02' AS varchar(50))", "CAST('2013-02-02' AS varchar(50))"); - // the cast operator returns a value that is too long for the expected type ('2013-02-02' for varchar(3)) - // the LiteralEncoder detects the mismatch and fails - assertThatThrownBy(() -> simplify("CAST(DATE '2013-02-02' AS varchar(3))")) - .hasMessage("Value [2013-02-02] does not fit in type varchar(3)"); - - // the cast operator returns a value that is too long for the expected type ('2013-02-02' for varchar(3)) - // the value is nested in a comparison expression, so the mismatch is not detected by the LiteralEncoder - assertSimplifies("CAST(DATE '2013-02-02' AS varchar(3)) = '2013-02-02'", "true"); + // cast from date to varchar fails, so the expression is not modified + assertSimplifies("CAST(DATE '2013-02-02' AS varchar(3))", "CAST(DATE '2013-02-02' AS varchar(3))"); + assertSimplifies("CAST(DATE '2013-02-02' AS varchar(3)) = '2013-02-02'", "CAST(DATE '2013-02-02' AS varchar(3)) = '2013-02-02'"); } private static void assertSimplifies(String expression, String expected) diff --git a/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java b/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java index 49298c7593fb..aa54cba89ada 100644 --- a/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java +++ b/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java @@ -272,9 +272,7 @@ public void testDateCastToVarchar() assertFunction("cast(DATE '13-2-2' AS varchar)", VARCHAR, "0013-02-02"); assertFunction("cast(DATE '2013-02-02' AS varchar(50))", createVarcharType(50), "2013-02-02"); assertFunction("cast(DATE '2013-02-02' AS varchar(10))", createVarcharType(10), "2013-02-02"); - - // cast operator returns a value that does not fit in the result type. this causes error in the LiteralEncoder - assertFunctionThrowsIncorrectly("cast(DATE '2013-02-02' AS varchar(9))", IllegalArgumentException.class, "Value .2013-02-02. does not fit in type varchar.9."); + assertInvalidCast("cast(DATE '2013-02-02' AS varchar(9))", "Value 2013-02-02 cannot be represented as varchar(9)"); } private static SqlDate toDate(DateTime dateTime) From e3de11f3b3363c656a58fc79fb31836a9cb1c081 Mon Sep 17 00:00:00 2001 From: kasiafi <30203062+kasiafi@users.noreply.github.com> Date: Thu, 20 Jan 2022 11:35:51 +0100 Subject: [PATCH 4/5] Add comments showing deviation from the standard --- .../trino-main/src/main/java/io/trino/util/DateTimeUtils.java | 4 ++++ .../src/test/java/io/trino/sql/TestExpressionInterpreter.java | 1 + .../src/test/java/io/trino/type/TestDateTimeOperators.java | 1 + 3 files changed, 6 insertions(+) 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 a8643633230f..bb7e57c75303 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 @@ -61,6 +61,10 @@ private DateTimeUtils() {} public static int parseDate(String value) { + // in order to follow the standard, we should validate the value: + // - the required format is 'YYYY-MM-DD' + // - all components should be unsigned numbers + // https://github.com/trinodb/trino/issues/10677 return toIntExact(TimeUnit.MILLISECONDS.toDays(DATE_FORMATTER.parseMillis(value))); } diff --git a/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java b/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java index 990cc7df39c6..f2ec1e416956 100644 --- a/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java +++ b/core/trino-main/src/test/java/io/trino/sql/TestExpressionInterpreter.java @@ -837,6 +837,7 @@ public void testCastRealToBoundedVarchar() public void testCastDateToBoundedVarchar() { assertEvaluatedEquals("CAST(DATE '2013-02-02' AS varchar(10))", "'2013-02-02'"); + // according to the SQL standard, this literal is incorrect. Year should be unsigned. https://github.com/trinodb/trino/issues/10677 assertEvaluatedEquals("CAST(DATE '-2013-02-02' AS varchar(50))", "'-2013-02-02'"); // the result value does not fit in the type diff --git a/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java b/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java index aa54cba89ada..2d8c3a8d7cdb 100644 --- a/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java +++ b/core/trino-main/src/test/java/io/trino/type/TestDateTimeOperators.java @@ -269,6 +269,7 @@ public void testDateCastFromVarchar() public void testDateCastToVarchar() { assertFunction("cast(DATE '2013-02-02' AS varchar)", VARCHAR, "2013-02-02"); + // according to the SQL standard, this literal is incorrect. The required format is 'YYYY-MM-DD'. https://github.com/trinodb/trino/issues/10677 assertFunction("cast(DATE '13-2-2' AS varchar)", VARCHAR, "0013-02-02"); assertFunction("cast(DATE '2013-02-02' AS varchar(50))", createVarcharType(50), "2013-02-02"); assertFunction("cast(DATE '2013-02-02' AS varchar(10))", createVarcharType(10), "2013-02-02"); From 38e209ce028f133dcf471c314db7cf32b52c4bb9 Mon Sep 17 00:00:00 2001 From: kasiafi <30203062+kasiafi@users.noreply.github.com> Date: Thu, 20 Jan 2022 14:17:26 +0100 Subject: [PATCH 5/5] Use @Language annotation in test methods --- .../sql/planner/iterative/rule/TestSimplifyExpressions.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java index ba22c4ec617c..1d6770a2b503 100644 --- a/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java +++ b/core/trino-main/src/test/java/io/trino/sql/planner/iterative/rule/TestSimplifyExpressions.java @@ -24,6 +24,7 @@ import io.trino.sql.tree.ExpressionRewriter; import io.trino.sql.tree.ExpressionTreeRewriter; import io.trino.sql.tree.LogicalExpression; +import org.intellij.lang.annotations.Language; import org.testng.annotations.Test; import java.util.Comparator; @@ -254,7 +255,7 @@ public void testCastDateToBoundedVarchar() assertSimplifies("CAST(DATE '2013-02-02' AS varchar(3)) = '2013-02-02'", "CAST(DATE '2013-02-02' AS varchar(3)) = '2013-02-02'"); } - private static void assertSimplifies(String expression, String expected) + private static void assertSimplifies(@Language("SQL") String expression, @Language("SQL") String expected) { Expression expectedExpression = normalize(rewriteIdentifiersToSymbolReferences(SQL_PARSER.createExpression(expected, new ParsingOptions()))); assertEquals( @@ -262,7 +263,7 @@ private static void assertSimplifies(String expression, String expected) expectedExpression); } - private static Expression simplify(String expression) + private static Expression simplify(@Language("SQL") String expression) { Expression actualExpression = rewriteIdentifiersToSymbolReferences(SQL_PARSER.createExpression(expression, new ParsingOptions())); return normalize(rewrite(actualExpression, TEST_SESSION, new SymbolAllocator(booleanSymbolTypeMapFor(actualExpression)), PLANNER_CONTEXT, createTestingTypeAnalyzer(PLANNER_CONTEXT)));