[VARIANT] impl Display for VariantDecimalXX#7785
Conversation
| // fall back to floating point | ||
| let result = integer as f64 / divisor as f64; | ||
| write!(json_buffer, "{}", result)?; | ||
| return Ok(()); |
There was a problem hiding this comment.
My original suggestion to use floating point was for the serde_json::Value::Number code path, which can only support i64, u64, and f64 values. Normal to-string can use arbitrary precision -- let the reader be the one to lose information (if it must).
There was a problem hiding this comment.
got it! this makes more sense, thank you
| macro_rules! format_decimal { | ||
| ($f:expr, $integer:expr, $scale:expr, $int_type:ty) => {{ | ||
| let integer = if $scale == 0 { | ||
| $integer | ||
| } else { | ||
| let divisor = (10 as $int_type).pow($scale as u32); |
There was a problem hiding this comment.
I was sorely tempted to just define a helper function that takes i128, since it would also produce correct output for all narrower integer types. But 128-bit division is vastly more expensive than 32- or 64-bit division, and the narrower types usually produce quotient and remainder from a single machine instruction.
If we think that performance difference doesn't matter, then we should use the helper function instead for simplicity.
There was a problem hiding this comment.
until we have benchmarks I don't think we can tell. This seems like a good improvement to me simply from a reusability perspective (I would liked to have a display impl for Variant in other times too)
|
This is great! thank you @scovich !! |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @scovich and @carpecodeum for the review
I agree this is a nice improvement
| macro_rules! format_decimal { | ||
| ($f:expr, $integer:expr, $scale:expr, $int_type:ty) => {{ | ||
| let integer = if $scale == 0 { | ||
| $integer | ||
| } else { | ||
| let divisor = (10 as $int_type).pow($scale as u32); |
There was a problem hiding this comment.
until we have benchmarks I don't think we can tell. This seems like a good improvement to me simply from a reusability perspective (I would liked to have a display impl for Variant in other times too)
|
I merged up to resolve a conflict with this PR |
|
The clippy failures are not related. See |
|
In order to keep the code moving here, I pushed up a change to fix clippy only for variant to this PR. Once the CI has passed I will merge it in |
3 participants
Which issue does this PR close?
Rationale for this change
Follow-up to #7670, which accidentally introduced a lossy to-string conversion for variant decimals.
What changes are included in this PR?
Use integer + string operations to convert decimal values to string, instead of floating point that could lose precision.
Also, the
VariantDecimalXXstructs nowimpl Display, which greatly simplifies the to-json path. A new (private) macro encapsulates the to-string logic, since it's syntactically identical for all three decimal sizes.Are these changes tested?
Yes, new unit tests added.
Are there any user-facing changes?
The
VariantDecimalXXstructs nowimpl Display