huan233usc commented on code in PR #17106:
URL: https://github.com/apache/iceberg/pull/17106#discussion_r3533970605
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -897,15 +897,46 @@ private static Literal<?> castDefault(Literal<?>
defaultValue, Type type) {
throw new IllegalArgumentException(
String.format("Invalid default value for %s: %s (must be null)",
type, defaultValue));
} else if (defaultValue != null) {
+ // Check before conversion so non-finite double-to-float defaults fail
clearly instead of
+ // converting to AboveMax or BelowMin sentinels.
Review Comment:
Small nit: `NaN` doesn't actually convert to a sentinel (the range
comparisons are false, so it falls through to a real `Float.NaN`) — only
`±Infinity` does. Might be worth tweaking the wording.
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -897,15 +897,46 @@ private static Literal<?> castDefault(Literal<?>
defaultValue, Type type) {
throw new IllegalArgumentException(
String.format("Invalid default value for %s: %s (must be null)",
type, defaultValue));
} else if (defaultValue != null) {
+ // Check before conversion so non-finite double-to-float defaults fail
clearly instead of
+ // converting to AboveMax or BelowMin sentinels.
+ validateFloatingDefault(type, defaultValue, defaultValue);
Literal<?> typedDefault = defaultValue.to(type);
Preconditions.checkArgument(
typedDefault != null, "Cannot cast default value to %s: %s", type,
defaultValue);
+ validateFloatingDefault(type, typedDefault, defaultValue);
Review Comment:
A finite out-of-range double (e.g. `1e40`) for a float field converts to an
`aboveMax` sentinel, whose `value()` throws UOE — so this now rejects it as
"cannot cast", where it was accepted before. Since the goal is only non-finite
values, could we skip the check for sentinels?
##########
api/src/main/java/org/apache/iceberg/types/Types.java:
##########
@@ -897,15 +897,46 @@ private static Literal<?> castDefault(Literal<?>
defaultValue, Type type) {
throw new IllegalArgumentException(
String.format("Invalid default value for %s: %s (must be null)",
type, defaultValue));
} else if (defaultValue != null) {
+ // Check before conversion so non-finite double-to-float defaults fail
clearly instead of
+ // converting to AboveMax or BelowMin sentinels.
+ validateFloatingDefault(type, defaultValue, defaultValue);
Literal<?> typedDefault = defaultValue.to(type);
Preconditions.checkArgument(
typedDefault != null, "Cannot cast default value to %s: %s", type,
defaultValue);
+ validateFloatingDefault(type, typedDefault, defaultValue);
return typedDefault;
}
return null;
}
+ private static void validateFloatingDefault(
+ Type type, Literal<?> defaultValue, Literal<?> originalDefault) {
+ if (type.typeId() == Type.TypeID.FLOAT || type.typeId() ==
Type.TypeID.DOUBLE) {
+ Object value;
+ try {
+ value = defaultValue.value();
Review Comment:
Nit (non-blocking): this only catches `UnsupportedOperationException` — a
custom `Literal` throwing something else would slip past.
##########
api/src/test/java/org/apache/iceberg/types/TestTypes.java:
##########
@@ -215,6 +216,60 @@ public void testGeospatialTypeDefaultNormalization() {
.hasSameHashCodeAs(Types.GeographyType.crs84());
}
+ @Test
+ public void defaultValuesMustBeFiniteForFloatingPointTypes() {
Review Comment:
Both cases are same-type, so `to(type)` is a no-op and only the pre-check
runs. Could we add a cross-type case (double `NaN` default on a float field) to
cover the post-conversion path too?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
