API: Reject non-finite floating defaults#17106
Conversation
Float and double field defaults are stored in schema metadata using Iceberg's JSON single-value serialization. A NaN or +/-Infinity default can currently be set through NestedField default validation even though those values are not portable JSON numbers and cannot safely round-trip through metadata. Reject non-finite floating-point initial and write defaults during NestedField default validation so invalid defaults fail before schema metadata is written.
huan233usc
left a comment
huan233usc
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left some comments.
| Literal<?> typedDefault = defaultValue.to(type); | ||
| Preconditions.checkArgument( | ||
| typedDefault != null, "Cannot cast default value to %s: %s", type, defaultValue); | ||
| validateFloatingDefault(type, typedDefault, defaultValue); |
There was a problem hiding this 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?
| 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. |
There was a problem hiding this 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.
| if (type.typeId() == Type.TypeID.FLOAT || type.typeId() == Type.TypeID.DOUBLE) { | ||
| Object value; | ||
| try { | ||
| value = defaultValue.value(); |
There was a problem hiding this comment.
Nit (non-blocking): this only catches UnsupportedOperationException — a custom Literal throwing something else would slip past.
| } | ||
|
|
||
| @Test | ||
| public void defaultValuesMustBeFiniteForFloatingPointTypes() { |
There was a problem hiding this 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?
2 participants
Float and double field defaults are stored in schema metadata using Iceberg's JSON single-value serialization. A NaN or +/-Infinity default can currently be set through NestedField default validation even though those values are not portable JSON numbers and cannot safely round-trip through metadata.
Reject non-finite floating-point initial and write defaults during NestedField default validation so invalid defaults fail before schema metadata is written.