fix: reject non-finite floating-point default values#810
Conversation
evindj
left a comment
There was a problem hiding this comment.
This looks good to me. For my own understanding how is the caller supposed to handle this error?
| // it as `null`, which reads back as an absent default. Reject it so the default is not | ||
| // silently lost when the metadata round-trips. | ||
| const auto& literal_value = value.value(); | ||
| const bool non_finite = (std::holds_alternative<float>(literal_value) && |
There was a problem hiding this comment.
You mentioned NaN in the PR description but doesn't seem to handle that, is that intentional?
There was a problem hiding this comment.
It is handled by following std::isfinite returns false for both NaN and infinity, so a NaN default hits the same non_finite branch and is rejected. The test ValidateRejectsNonFiniteFloatingDefault covers NaN, +Inf and -Inf explicitly.
|
I think java impl has the same issue, created a PR to strict the validation apache/iceberg#17106 |
|
@evindj thanks for the review! |
What
A
float/doublecolumn default ofNaNor±InfinitypassesSchemaField::Validatebut cannot survive a metadata round-trip: the JSON serializer emits a non-finite number asnull, which reads back as an absent default. The declared default is silently lost (and, because the key is present but null, the reader then errors on it).How
ValidateDefaultnow rejects a non-finite floating default with a clear "value must be finite" error, so an unrepresentable default is caught up front rather than silently dropped on serialization.The check uses
std::isfinite, which is false for bothNaNand±Infinity— so a single condition covers all three cases (there is no separateIsNaNbranch becauseisfinitealready rejects NaN).Testing
SchemaFieldTest.ValidateRejectsNonFiniteFloatingDefaultexplicitly coversNaN,+Infand-Inf, andValidateAcceptsFiniteFloatingDefaultcovers a normal value; verified fail-without / pass-with. Fullschema_testpasses (551 tests).