Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RADON parses JSON integers as floats, rounding them to a different value #2315

Closed
tmpolaczyk opened this issue Nov 23, 2022 · 3 comments · Fixed by #2373
Closed

RADON parses JSON integers as floats, rounding them to a different value #2315

tmpolaczyk opened this issue Nov 23, 2022 · 3 comments · Fixed by #2373
Labels
breaking change ⚠️ Introduces breaking changes

Comments

@tmpolaczyk
Copy link
Contributor

So operators like StringParseJSONArray can only parse integers correctly up to MAX_SAFE_INTEGER, which is 2^53-1.

The following test shows how 2^53+1 is parsed as 2^53:

#[test]
fn test_parse_json_array_max_safe_integer_plus_2() {
    use crate::types::array::RadonArray;
    use crate::types::integer::RadonInteger;
    use crate::types::string::RadonString;

    // 2^53 + 1
    let input = RadonTypes::from(RadonString::from(r#"[9007199254740993]"#));
    let script = vec![(RadonOpCodes::StringParseJSONArray, None)];

    let output = execute_contextfree_radon_script(input, &script).unwrap();

    assert_eq!(
        output,
        RadonTypes::from(RadonArray::from(vec![RadonTypes::from(RadonInteger::from(
            9007199254740993
        ))]))
    );
}

Output:

thread 'script::tests::test_parse_json_array_biggish_integers' panicked at 'assertion failed: `(left == right)`
  left: `Array(RadonArray { value: [Integer(RadonInteger { value: 9007199254740992 })], is_homogeneous: true })`,
 right: `Array(RadonArray { value: [Integer(RadonInteger { value: 9007199254740993 })], is_homogeneous: true })`', rad/src/script.rs:607:9
@guidiaz
Copy link
Contributor

guidiaz commented Nov 23, 2022

Could we set the Json parser lib to use quadruple precision floats instead ? Or to only convert into double floats when there's a decimal dot and to an integer when there's not ?

@aesedepece
Copy link
Member

[...] Or to only convert into double floats when there's a decimal dot and to an integer when there's not ?

I believe that's what it's supposed to do 🤔

@tmpolaczyk
Copy link
Contributor Author

Yes, it converts to integer, but it converts the float to integer, therefore losing precision:

json::JsonValue::Number(value) => {
let (_, _, exponent) = value.as_parts();
let floating = f64::from(*value);
// Cast the float into an integer if it has no fractional part and its value will fit
// into the range of `i128` (38 is the biggest power of 10 that `i128` can safely hold)
if floating.fract() == 0.0 && exponent.unsigned_abs() < 38 {
// This cast is assumed to be safe as per the previous guard
Value::Integer(floating as i128)
} else {
Value::Float(floating)
}
}

The Number type used by the JSON parsing library is some weird emulated floating point type with 64 bit mantissa and 16 bit exponent, so maybe it is possible to support 64-bit integers using that library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change ⚠️ Introduces breaking changes
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants