enhancement(codecs): Support protobuf encoding integers as floats #19527
Conversation
…oubles Rust's conversion functionality allows converting i64 to f32 and f64 with high precision for all possible i64 values. We can leverage this convenience for protobuf encoding.
| @@ -96,6 +96,8 @@ fn convert_value_raw( | |||
| } | |||
| (Value::Float(f), Kind::Double) => Ok(prost_reflect::Value::F64(f.into_inner())), | |||
| (Value::Float(f), Kind::Float) => Ok(prost_reflect::Value::F32(f.into_inner() as f32)), | |||
| (Value::Integer(i), Kind::Double) => Ok(prost_reflect::Value::F64(i as f64)), | |||
| (Value::Integer(i), Kind::Float) => Ok(prost_reflect::Value::F32(i as f32)), | |||
There was a problem hiding this comment.
Even it's a "high precision conversion", the farther you get from 0 the more problematic it becomes. I am not fully convinced we want to introduce this. Is the motivation to avoid using a remap transform? (that might also have problems with very large integers)
Perhaps if we document very clearly the caveats especially when it comes to large i64 we can justify introducing this for convenience. Users who know their data can leverage it.
There was a problem hiding this comment.
@pront could you clarify "the farther you get from 0 the more problematic it becomes"? I added tests in this PR specifically to see if that was the case.
The motivation is to avoid using a remap to transform, particularly when the conversion to integer is the result of another parser and not a remap itself, e.g. the JSON parser deciding that the number it parsed is an integer instead of a float.
There was a problem hiding this comment.
I see now, this use case makes sense.
There was a problem hiding this comment.
The problem is that integer values larger than the number of mantissa bits will have the low-order bits cleared.
There was a problem hiding this comment.
The problem is that integer values larger than the number of mantissa bits will have the low-order bits cleared.
This is what I was referring to as problematic. These conversions are lossy and that would need to be documented. VRL also performs lossy conversions, see here.
| let message = encode_message( | ||
| &test_message_descriptor("Floats"), | ||
| Value::Object(BTreeMap::from([ | ||
| ("d".into(), Value::Integer(9223372036854775807)), |
|
There are a few failing checks. I also left one new comment. |
| ])), | ||
| ) | ||
| .unwrap(); | ||
| assert_eq!(Some(9.223372036854775807e18), mfield!(message, "d").as_f64()); |
There was a problem hiding this comment.
Unfortunately, this doesn't actually test what it appears to: 9.223372036854775807e18 (AKA 9223372036854775807.0) silently converts to 9.223372036854776e+18 which its internal representation. Run this Rust Playground to demonstrate.
| ) | ||
| .unwrap(); | ||
| assert_eq!(Some(9.223372036854775807e18), mfield!(message, "d").as_f64()); | ||
| assert_eq!(Some(9.223372036854775807e18), mfield!(message, "f").as_f32()); |
There was a problem hiding this comment.
Similarly, this silently converts to 9.223372e18 internally.
| @@ -96,6 +96,8 @@ fn convert_value_raw( | |||
| } | |||
| (Value::Float(f), Kind::Double) => Ok(prost_reflect::Value::F64(f.into_inner())), | |||
| (Value::Float(f), Kind::Float) => Ok(prost_reflect::Value::F32(f.into_inner() as f32)), | |||
| (Value::Integer(i), Kind::Double) => Ok(prost_reflect::Value::F64(i as f64)), | |||
| (Value::Integer(i), Kind::Float) => Ok(prost_reflect::Value::F32(i as f32)), | |||
There was a problem hiding this comment.
The problem is that integer values larger than the number of mantissa bits will have the low-order bits cleared.
thomasqueirozb
added
the
meta: awaiting author
pront
force-pushed
the
master
branch
4 times, most recently
from
1720078 to
ffe54be
Compare
Rust's conversion functionality allows converting i64 to f32 and f64 with high precision for all possible i64 values. We can leverage this convenience to avoid users having to remap types using VRL.