wenshao commented on code in PR #28828:
URL: https://github.com/apache/flink/pull/28828#discussion_r3696498485
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/utils/ValueDataTypeConverter.java:
##########
@@ -217,12 +217,12 @@ private static int fractionalSecondPrecision(int nanos) {
return String.format("%09d", nanos).replaceAll("0+$", "").length();
}
- private static int yearPrecision(int years) {
- return String.valueOf(years).length();
+ private static int yearPrecision(long years) {
+ return String.valueOf(Math.abs(years)).length();
}
Review Comment:
**[Suggestion]** The `Math.abs` fix in `yearPrecision` and `dayPrecision`
has no test coverage — all interval test inputs in this diff are positive, so
the `Math.abs` branch is never exercised. — Concrete cost: if a future change
removes `Math.abs`, `yearPrecision(-39)` would compute
`String.valueOf(-39).length()` = 3 (counting the minus sign), producing
`YEAR(3)` instead of the correct `YEAR(2)`, and no test would catch it.
Consider adding at least one negative-interval case to
`intervalLiteralTestCases()` in `ExpressionTest` (e.g., `Period.ofMonths(-470)`
expecting `YEAR(2)`) and one to `ValueDataTypeConverterTest` (e.g.,
`Duration.ofDays(-100)` expecting `DAY(3)`).
```suggestion
private static int yearPrecision(long years) {
return String.valueOf(Math.abs(years)).length();
}
private static int dayPrecision(long days) {
return String.valueOf(Math.abs(days)).length();
}
```
_— qwen3.8-max-preview via Qwen Code /review (v0.21.2)_
--
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]
