featzhang commented on code in PR #28146:
URL: https://github.com/apache/flink/pull/28146#discussion_r3228492939
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/strategies/ToTimestampLtzTypeStrategy.java:
##########
@@ -66,19 +70,24 @@ public Optional<DataType> inferType(CallContext
callContext) {
}
break;
case 2:
- LogicalType secondType = argumentTypes.get(1).getLogicalType();
+ LogicalType secondType =
+
argumentTypes.get(PRECISION_OR_FORMAT_ARG).getLogicalType();
LogicalTypeRoot secondTypeRoot = secondType.getTypeRoot();
if (firstType.is(LogicalTypeFamily.NUMERIC)) {
if (secondTypeRoot != LogicalTypeRoot.INTEGER) {
throw new ValidationException(
"Unsupported argument type. "
+ "TO_TIMESTAMP_LTZ(<NUMERIC>,
<INTEGER>) requires the second argument to be <INTEGER>.");
}
- Optional<Integer> precisionOpt =
callContext.getArgumentValue(1, Integer.class);
- if (precisionOpt.isPresent()) {
- int precision = precisionOpt.get();
- validatePrecision(precision);
- outputPrecision = Math.max(precision,
DEFAULT_PRECISION);
+ if
(callContext.isArgumentLiteral(PRECISION_OR_FORMAT_ARG)) {
Review Comment:
The guard prevents the plan-time crash correctly. Nice fix.
One thought: could we add a plan-time warning when precision is non-literal?
Users might not realize they're losing sub-millisecond precision due to the
default TIMESTAMP_LTZ(3) output type.
Something like:
```java
if (!callContext.isArgumentLiteral(PRECISION_OR_FORMAT_ARG)) {
// LOG.warn("TO_TIMESTAMP_LTZ: non-literal precision defaults to 3...");
return DEFAULT_PRECISION;
}
```
The documentation you added is clear, but a warning might help users catch
unintended precision loss at development time. Not critical though.
--
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]
