tsreaper commented on a change in pull request #16740:
URL: https://github.com/apache/flink/pull/16740#discussion_r684875945
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/sql/FlinkSqlOperatorTable.java
##########
@@ -224,6 +224,15 @@ public void lookupOperatorOverloads(
OperandTypes.or(OperandTypes.NUMERIC_INTEGER,
OperandTypes.NUMERIC),
SqlFunctionCategory.NUMERIC);
+ public static final SqlFunction TRUNCATE =
+ new SqlFunction(
+ "TRUNCATE",
+ SqlKind.OTHER_FUNCTION,
+ FlinkReturnTypes.ROUND_FUNCTION_NULLABLE,
Review comment:
If you look into this method you'll find that it is calling
`LogicalTypeMerging#findRoundDecimalType`. In that method there is a line
stating that
```java
// NOTE: rounding may increase the digits by 1, therefore we need +1 on
precisions.
return new DecimalType(false, 1 + precision - scale + round, round);
```
However for `truncate` function the number of digits will not increase, thus
`FlinkReturnTypes.ROUND_FUNCTION_NULLABLE` is not the best choice to use here.
What I'll suggest is that you create a new method in `LogicalTypeMerging`
called `findTruncateDecimalType`. This method and `findRoundDecimalType` can
together reuse some code. Then you might want to create
`FlinkReturnTypes.TRUNCATE_FUNCTION_NULLABLE` which will also reuse a lot of
code with `ROUND_FUNCTION_NULLABLE`.
##########
File path:
flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/functions/MathFunctionsITCase.java
##########
@@ -115,6 +115,14 @@
$("f0").round(2),
"ROUND(f0, 2)",
new BigDecimal("12345.12"),
- DataTypes.DECIMAL(8, 2).notNull()));
+ DataTypes.DECIMAL(8, 2).notNull()),
+ TestSpec.forFunction(BuiltInFunctionDefinitions.TRUNCATE)
+ .onFieldsWithData(new BigDecimal("123.456"))
+ // TRUNCATE(DECIMAL(6, 3) NOT NULL, 2) => DECIMAL(6,
2) NOT NULL
+ .testResult(
+ $("f0").truncate(2),
+ "TRUNCATE(f0, 2)",
+ new BigDecimal("123.45"),
+ DataTypes.DECIMAL(6, 2).notNull()));
Review comment:
This `MathFunctionITCase`, as stated in the java docs, is for
`BuiltInFunctionDefinitions`. If you follow the usage of
`BuiltinFunctionDefinitions` you'll see that it is converted to
`FlinkSqlOperatorTable.TRUNCATE`. For Flink SQL scalar functions we always add
tests in `ScalarFunctionsTest`. Please add your tests there.
A thorough test for a function should include all data types it supported as
well as their corresponding null values. The tests in
`ScalarFunctionTest#testTruncate` may not be complete so please complete them
with all supported data types. See `FlinkSqlOperatorTable.TRUNCATE` to get all
its supported types.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
