You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2021/10/28 10:53:32 UTC
[GitHub] [flink] twalthr commented on a change in pull request #17579: [FLINK-24454][table-planner][tests] Consolidate cast tests
twalthr commented on a change in pull request #17579:
URL: https://github.com/apache/flink/pull/17579#discussion_r738258450
##########
File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CastFunctionITCase.java
##########
@@ -191,149 +210,159 @@ protected Configuration configuration() {
.failTableApi(FLOAT(), -123.456)
.failTableApi(DOUBLE(), 0)
// Not supported - no fix
- // DATE
- // TIME
- // TIMESTAMP
+ .fail(DATE(), LocalDate.of(2021, 10, 26))
+ .fail(TIME(), LocalTime.parse("12:34:56.123"))
+ .fail(TIMESTAMP(), LocalDateTime.parse("2021-09-24T12:34:56.123456"))
// TIMESTAMP_WITH_TIME_ZONE
- // TIMESTAMP_LTZ
- // INTERVAL_YEAR_MONTH
- // INTERVAL_DAY_TIME
- // ARRAY
+ .fail(TIMESTAMP_LTZ(), fromLocalTZ("2021-09-24T18:34:56.123456"))
+ .fail(INTERVAL(YEAR(), MONTH()), Period.of(10, 4, 0))
+ .fail(INTERVAL(DAY(), SECOND()), Duration.ofHours(12))
+ .fail(ARRAY(INT()), new int[] {0, 1, 2})
// MULTISET
// MAP
// ROW
// RAW
.build(),
CastTestSpecBuilder.testCastTo(BINARY(2))
+ .fromCase(BINARY(5), null, null)
.fromCase(CHAR(3), "foo", new byte[] {102, 111, 111})
.fromCase(VARCHAR(5), "Flink", new byte[] {70, 108, 105, 110, 107})
// https://issues.apache.org/jira/browse/FLINK-24419 - not trimmed to 2
// bytes
.fromCase(STRING(), "Apache", new byte[] {65, 112, 97, 99, 104, 101})
// Not supported - no fix
- // BOOLEAN
+ .fail(BOOLEAN(), randomBoolean())
+ //
.fromCase(BINARY(2), new byte[] {0, 1}, new byte[] {0, 1})
.fromCase(VARBINARY(3), new byte[] {0, 1, 2}, new byte[] {0, 1, 2})
.fromCase(BYTES(), new byte[] {0, 1, 2, 3, 4}, new byte[] {0, 1, 2, 3, 4})
// Not supported - no fix
- // DECIMAL
- // TINYINT
- // SMALLINT
- // INT
- // BIGINT
- // FLOAT
- // DOUBLE
- // DATE
- // TIME
- // TIMESTAMP
+ .fail(DECIMAL(5, 3), 12.345)
+ .fail(TINYINT(), (byte) 5)
+ .fail(SMALLINT(), (short) 12)
+ .fail(INT(), 122345)
+ .fail(BIGINT(), 12345678912L)
+ .fail(FLOAT(), 1.2345)
+ .fail(DOUBLE(), 1.23456789)
+ .fail(DATE(), LocalDate.of(2021, 10, 26))
+ .fail(TIME(), LocalTime.parse("12:34:56.123"))
+ .fail(TIMESTAMP(), LocalDateTime.parse("2021-09-24T12:34:56.123456"))
// TIMESTAMP_WITH_TIME_ZONE
- // TIMESTAMP_LTZ
- // INTERVAL_YEAR_MONTH
- // INTERVAL_DAY_TIME
- // ARRAY
+ .fail(TIMESTAMP_LTZ(), fromLocalTZ("2021-09-24T18:34:56.123456"))
+ .fail(INTERVAL(YEAR(), MONTH()), Period.of(10, 4, 0))
+ .fail(INTERVAL(DAY(), SECOND()), Duration.ofHours(12))
+ .fail(ARRAY(INT()), new int[] {0, 1, 2})
Review comment:
btw have you thought about introducing a default value for every type in this test. I think reading `DEFAULT_INT` or `DEFAULT_ARRAY_INT` would improve the readability. One can still add non default values for specific tests that test e.g. boundaries.
##########
File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CastFunctionITCase.java
##########
@@ -930,16 +1066,44 @@ private TestSpec build() {
int idxOffset = 0;
for (int i = 0; i < columnData.size(); i++) {
String colName = "f" + i;
+ LogicalType colType = columnTypes.get(i).getLogicalType();
+ String errorMsg;
switch (testTypes.get(i)) {
case ERROR_TABLE_API:
- testSpec.testTableApiValidationError(
- $(colName).cast(targetType),
- String.format(
- "Invalid function call:%ncast("
- + columnTypes.get(i).toString()
- + ", "
- + targetType.toString()
- + ")"));
+ if (isTimestampLtzToNumeric(colType, targetType.getLogicalType())) {
Review comment:
maybe move those in a dedicated method for `specific errors` or `hint errors`?
##########
File path: flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/expressions/ScalarOperatorsTest.scala
##########
@@ -144,14 +144,10 @@ class ScalarOperatorsTest extends ScalarOperatorsTestBase {
testSqlApi("CASE WHEN 'a'='a' THEN 1 END", "1")
testSqlApi("CASE 2 WHEN 1 THEN 'a' WHEN 2 THEN 'bcd' END", "bcd")
testSqlApi("CASE 1 WHEN 1 THEN 'a' WHEN 2 THEN 'bcd' END", "a")
- testSqlApi("CASE 1 WHEN 1 THEN CAST ('a' as varchar(1)) WHEN 2 THEN " +
Review comment:
I would still keep this. As we had issue with `org.apache.calcite.rel.type.RelDataTypeSystem#shouldConvertRaggedUnionTypesToVarying`
##########
File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CastFunctionITCase.java
##########
@@ -966,4 +1130,21 @@ private static Instant fromLocalToUTC(LocalDateTime localDateTime) {
private static Instant fromLocalTZ(String str) {
return LocalDateTime.parse(str).toInstant(TEST_OFFSET);
}
+
+ private static boolean randomBoolean() {
Review comment:
randomness in tests is never a good idea
##########
File path: flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/logical/utils/LogicalTypeCasts.java
##########
@@ -184,17 +184,17 @@
castTo(DATE)
.implicitFrom(DATE, TIMESTAMP_WITHOUT_TIME_ZONE)
- .explicitFromFamily(TIMESTAMP, CHARACTER_STRING, BINARY_STRING)
Review comment:
can we put such change in `table-common` into separate commits. this change should be visible in the log afterwards.
##########
File path: flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/MiscFunctionsITCase.java
##########
@@ -46,7 +46,9 @@
"INT NOT NULL",
DataTypes.STRING())
.testTableApiValidationError(
- call("TYPEOF", $("f0"), $("f2")), "Invalid input arguments.")
+ call("TYPEOF", $("f0"), $("f2")),
Review comment:
hotfix commit? not really related to the JIRA issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org