twalthr commented on a change in pull request #17341:
URL: https://github.com/apache/flink/pull/17341#discussion_r717335148
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/BuiltInFunctionTestBase.java
##########
@@ -261,6 +248,14 @@ TestSpec withFunction(Class<? extends UserDefinedFunction>
functionClass) {
TestSpec testTableApiResult(
Expression expression, Object result, AbstractDataType<?>
dataType) {
+ return testTableApiResult(
+ new Expression[] {expression},
+ new Object[] {result},
+ new AbstractDataType<?>[] {dataType});
+ }
+
+ TestSpec testTableApiResult(
+ Expression[] expression, Object[] result,
AbstractDataType<?>[] dataType) {
Review comment:
nit: could we use `List` instead of arrays? I find `Arrays.asList` and
soon `List.of` nicer than the verbose array syntax.
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/BuiltInFunctionTestBase.java
##########
@@ -298,14 +298,52 @@ TestSpec testResult(
return testResult(expression, sqlExpression, result, dataType,
dataType);
}
+ TestSpec testResult(TableTestSpecColumn... tableTestSpecColumns) {
+ int cols = tableTestSpecColumns.length;
Review comment:
nit: we don't enforce using final in Flink but if you check the core,
you will see that most committers use final extensively to indicate
immutable/mutability in code. I would vote for having a conistent coding style
within our team. At least we should adapt to the coding style of the class.
When I read this and the following lines, it looks to me as if we would modify
the reference in the for loop again. Because all variables above are marked
`final`.
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/BuiltInFunctionTestBase.java
##########
@@ -298,14 +298,52 @@ TestSpec testResult(
return testResult(expression, sqlExpression, result, dataType,
dataType);
}
+ TestSpec testResult(TableTestSpecColumn... tableTestSpecColumns) {
+ int cols = tableTestSpecColumns.length;
+ Expression[] expressions = new Expression[cols];
+ String[] sqlExpressions = new String[cols];
+ Object[] results = new Object[cols];
+ AbstractDataType<?>[] tableApiDataTypes = new
AbstractDataType<?>[cols];
+ AbstractDataType<?>[] sqlDataTypes = new AbstractDataType<?>[cols];
+
+ for (int i = 0; i < cols; i++) {
+ TableTestSpecColumn tableTestSpecColumn =
tableTestSpecColumns[i];
+ expressions[i] = tableTestSpecColumn.tableApiExpression;
+ sqlExpressions[i] = tableTestSpecColumn.sqlExpression;
+ results[i] = tableTestSpecColumn.result;
+ tableApiDataTypes[i] = tableTestSpecColumn.tableApiDataType;
+ sqlDataTypes[i] = tableTestSpecColumn.sqlDataType;
+ }
+ return testResult(
+ expressions, sqlExpressions, results, tableApiDataTypes,
sqlDataTypes);
+ }
+
TestSpec testResult(
Expression expression,
String sqlExpression,
Object result,
AbstractDataType<?> tableApiDataType,
AbstractDataType<?> sqlDataType) {
+ return testResult(
+ new Expression[] {expression},
+ new String[] {sqlExpression},
+ new Object[] {result},
+ new AbstractDataType<?>[] {tableApiDataType},
+ new AbstractDataType<?>[] {sqlDataType});
+ }
+
+ TestSpec testResult(
+ Expression[] expression,
+ String[] sqlExpression,
+ Object[] result,
+ AbstractDataType<?>[] tableApiDataType,
+ AbstractDataType<?>[] sqlDataType) {
testItems.add(new TableApiResultTestItem(expression, result,
tableApiDataType));
- testItems.add(new SqlResultTestItem(sqlExpression, result,
sqlDataType));
+ StringJoiner sj = new StringJoiner(",");
+ for (String sql : sqlExpression) {
Review comment:
nit: use the Java streams API or `String.join` once we have lists
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/BuiltInFunctionTestBase.java
##########
@@ -383,18 +439,69 @@ public String toString() {
}
}
- private static class SqlErrorTestItem extends ErrorTestItem {
- final String expression;
+ private static class SqlErrorTestItem extends ErrorTestItem<String> {
private SqlErrorTestItem(
String expression, String errorMessage, boolean
expectedDuringValidation) {
- super(errorMessage, expectedDuringValidation);
- this.expression = expression;
+ super(expression, errorMessage, expectedDuringValidation);
+ }
+
+ @Override
+ String expression() {
+ return expression;
}
@Override
public String toString() {
return "[SQL] " + expression;
}
}
+
+ private static List<DataType> createDataTypes(
+ DataTypeFactory dataTypeFactory, AbstractDataType<?>[] dataTypes) {
+ return Stream.of(dataTypes)
+ .map(dataTypeFactory::createDataType)
+ .collect(Collectors.toList());
+ }
+
+ /** Helper POJO to store test parameters. */
+ public static class TableTestSpecColumn {
Review comment:
`Column` sounds very internal to this class. Tests don't know columns
but only test specs. How about `ResultSpec` to keep it short?
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/CoalesceFunctionITCase.java
##########
@@ -40,23 +41,28 @@
.onFieldsWithData(null, null, 1)
.andDataTypes(BIGINT().nullable(), INT().nullable(),
INT().notNull())
.testResult(
- coalesce($("f0"), $("f1")),
- "COALESCE(f0, f1)",
- null,
- BIGINT().nullable())
- .testResult(
- coalesce($("f0"), $("f2")),
- "COALESCE(f0, f2)",
- 1L,
- BIGINT().notNull())
- .testResult(
- coalesce($("f1"), $("f2")), "COALESCE(f1,
f2)", 1, INT().notNull())
- .testResult(
- coalesce($("f0"), 1),
- "COALESCE(f0, 1)",
- 1L,
- // In this case, the return type is not null
because we have a
- // constant in the function invocation
- BIGINT().notNull()));
+ of(
Review comment:
instead of using a static import in every test, we could simply provide
static methods in the upper class, called `resultItem(...)`
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/BuiltInFunctionTestBase.java
##########
@@ -383,18 +439,69 @@ public String toString() {
}
}
- private static class SqlErrorTestItem extends ErrorTestItem {
- final String expression;
+ private static class SqlErrorTestItem extends ErrorTestItem<String> {
private SqlErrorTestItem(
String expression, String errorMessage, boolean
expectedDuringValidation) {
- super(errorMessage, expectedDuringValidation);
- this.expression = expression;
+ super(expression, errorMessage, expectedDuringValidation);
+ }
+
+ @Override
+ String expression() {
+ return expression;
}
@Override
public String toString() {
return "[SQL] " + expression;
}
}
+
+ private static List<DataType> createDataTypes(
+ DataTypeFactory dataTypeFactory, AbstractDataType<?>[] dataTypes) {
+ return Stream.of(dataTypes)
+ .map(dataTypeFactory::createDataType)
+ .collect(Collectors.toList());
+ }
+
+ /** Helper POJO to store test parameters. */
+ public static class TableTestSpecColumn {
+
+ Expression tableApiExpression;
+ String sqlExpression;
+ Object result;
+ AbstractDataType<?> tableApiDataType;
+ AbstractDataType<?> sqlDataType;
+
+ public static TableTestSpecColumn of(
+ Expression tableApiExpression,
+ String sqlExpression,
+ Object result,
+ AbstractDataType<?> dataType) {
+ return of(tableApiExpression, sqlExpression, result, dataType,
dataType);
+ }
+
+ public static TableTestSpecColumn of(
+ Expression tableApiExpression,
+ String sqlExpression,
+ Object result,
+ AbstractDataType<?> tableApiDataType,
+ AbstractDataType<?> sqlQueryDataType) {
Review comment:
if Table API and SQL API share the same result it should in most cases
also have the same data type?
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/BuiltInFunctionTestBase.java
##########
@@ -150,7 +153,7 @@ private static void testResult(
assertEquals(
"Result of column [" + i + "] doesn't match.",
Review comment:
`of spec` btw I think it would be nice to also print the test item
summary to quickly know which one failed if there are many.
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/BuiltInFunctionTestBase.java
##########
@@ -383,18 +439,69 @@ public String toString() {
}
}
- private static class SqlErrorTestItem extends ErrorTestItem {
- final String expression;
+ private static class SqlErrorTestItem extends ErrorTestItem<String> {
private SqlErrorTestItem(
String expression, String errorMessage, boolean
expectedDuringValidation) {
- super(errorMessage, expectedDuringValidation);
- this.expression = expression;
+ super(expression, errorMessage, expectedDuringValidation);
+ }
+
+ @Override
+ String expression() {
+ return expression;
}
@Override
public String toString() {
return "[SQL] " + expression;
}
}
+
+ private static List<DataType> createDataTypes(
+ DataTypeFactory dataTypeFactory, AbstractDataType<?>[] dataTypes) {
+ return Stream.of(dataTypes)
+ .map(dataTypeFactory::createDataType)
+ .collect(Collectors.toList());
+ }
+
+ /** Helper POJO to store test parameters. */
+ public static class TableTestSpecColumn {
+
+ Expression tableApiExpression;
+ String sqlExpression;
+ Object result;
+ AbstractDataType<?> tableApiDataType;
+ AbstractDataType<?> sqlDataType;
+
+ public static TableTestSpecColumn of(
+ Expression tableApiExpression,
+ String sqlExpression,
+ Object result,
+ AbstractDataType<?> dataType) {
+ return of(tableApiExpression, sqlExpression, result, dataType,
dataType);
+ }
+
+ public static TableTestSpecColumn of(
+ Expression tableApiExpression,
+ String sqlExpression,
+ Object result,
+ AbstractDataType<?> tableApiDataType,
+ AbstractDataType<?> sqlQueryDataType) {
Review comment:
TBH this is a very special case and hopefully does not happen elsewhere.
we should not start having more of these special cases.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
