Repository navigation
fix(spec): read float JSON values like iceberg-java - #3476
Open
spicysharkco wants to merge 1 commit into
Open
spicysharkco wants to merge 1 commit into
spicysharkco wants to merge 1 commit into
Conversation
The Float branch of RawLiteral::try_into rejected a double whose distance to the narrowed f32 exceeded the absolute f32::EPSILON, so valid values such as 100.1 and 123.456 failed. iceberg-java narrows with floatValue() and does no precision check; do the same.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
What changes are included in this PR?
RawLiteral::try_intoforPrimitiveType::Floatrejected a double when(f64::from(v as f32) - v).abs() > f32::EPSILON. That is an absolute bound, so valid floats such as100.1and123.456failed withUnable to convert raw literal (double) fail convert to type float for: type mismatch. This path reads float partition values indeserialize_data_file_from_jsonand floatDatumvalues.The check is replaced by a plain
v as f32. That is what iceberg-java does:SingleValueParser.fromJsonchecksisFloatingPointNumber()for FLOAT and returnsfloatValue()(SingleValueParser.java L73-L79). It is also whatLiteral::try_from_jsonalready does for field defaults. A double with no exact f32 now narrows to the nearest one:16777217.0gives16777216.0, where main returned an error.Doubles outside the f32 range (for example
1e39, which becomes infinity) behave as on main and asfloatValue(). That is the subject of #3333 and is not touched here.Notes for reviewers:
test_raw_double_to_float_precision_lossalso expects an error for123.456789and123456789.0, which this PR accepts. If this lands first, fix(spec): reject float overflow in raw literals #3333 would need its own overflow check and that test would change.null) is left open. Returning an error fromLiteral::try_into_jsonwould makeFrom<NestedField> for SerdeNestedFieldpanic atdatatypes.rs:649-650(.expectontry_into_json) for a field whose default is NaN or infinite. Java'sSingleValueParser.toJsonwrites quoted"NaN"and"Infinity"strings by default (JacksonQUOTE_NON_NUMERIC_NUMBERS), which its ownfromJsoncannot read back. Which behaviour do you want here?Are these changes tested?
Unit tests in
crates/iceberg/src/spec/values/tests.rs, which fail on current main and pass with this change:raw_literal_json_number_to_float:100.1,123.456,-123.456,f32::MAX,f32::MINand the smallest subnormal, parsed withserde_json::from_str::<RawLiteral>, convert to exactly that f32.raw_literal_avro_double_to_float: the same values throughValue::Double, both as the widened f32 and as the shortest decimal parsed to f64.raw_literal_double_without_exact_float_narrows_to_nearest:16777217.0becomes16777216.0, and1.0000000001becomes1.0.