Timm0 commented on code in PR #28437:
URL: https://github.com/apache/flink/pull/28437#discussion_r3705435584
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/TypeInferenceUtil.java:
##########
Review Comment:
Doesn't a change here mean a change in behavior when using `ARRAY<DECIMAL(2,
2)>` ? Previously it would have been just `NULL` and now it throws a
`ValidationException`?
##########
flink-table/flink-table-common/src/main/java/org/apache/flink/table/types/inference/TypeInferenceUtil.java:
##########
@@ -155,11 +158,59 @@ private static CallContext castArgumentsInternal(
"Invalid argument type at position %d. Data
type %s expected but %s passed.",
pos, expectedType, actualType));
}
+
+ // Beyond type-level castability, a constant DECIMAL literal must
also fit the
+ // expected type on the value level.
+ if (throwOnInferInputFailure) {
+ validateDecimalLiteralFitsExpectedType(callContext, pos,
expectedType);
+ }
}
return castCallContext;
}
+ /**
+ * Validates that a constant {@code DECIMAL} literal argument fits the
expected type on the
+ * value level and not only on the type level. For example, {@code
123.456} is implicitly
+ * castable to a {@code DECIMAL(2, 2)} but would otherwise be silently
reduced to {@code NULL}
+ * during constant folding instead of raising a type error.
+ *
+ * <p>Only {@code DECIMAL} is covered here; other value-level overflows
(e.g. {@code CHAR}
+ * overruns or out-of-range integer literals) are out of scope for this
check.
+ */
+ private static void validateDecimalLiteralFitsExpectedType(
+ CallContext callContext, int pos, DataType expectedType) {
+ if (!expectedType.getLogicalType().is(LogicalTypeRoot.DECIMAL)) {
Review Comment:
I was taking a look at the `LegacyTypeInfoDataTypeConverter` which is
deprecated, but there still seems to be some usage. `LegacyTypeInformationType`
can carry `DECIMAL` as a type root, when the type is `Types.BIG_DEC`. In that
case we would get an exception when casting at line 202. Could you check if
this is a case that can actually happen here? I think in any case asserting
here that `expectedType.getLogicalType() instanceof DecimalType decimalType`
should fix this.
--
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]
