GH-38395: [Go] fix rounding errors in decimal256 string functions #38426
Conversation
|
|
github-actions
bot
added
Component: Go
awaiting committer review
github-actions
bot
added
awaiting changes
| if out.Signbit() { | ||
| out.Sub(out, pt5) | ||
| } else { | ||
| out.Add(out, pt5) | ||
| } |
There was a problem hiding this comment.
This is done because the round is a "round up", right?
There was a problem hiding this comment.
more accurately it's a round away from zero
There was a problem hiding this comment.
So -7.0 will become -7.0-0.5=-7.5 and round to -8.0?
There was a problem hiding this comment.
Not quite, check the comment above the spot: we truncate the value to get an integer. So -7.0 with a scale of 0 becomes -7.5 and then we truncate it to -7. -7.5 becomes -8.0 and then we truncate it to -8.
A better example would be if we're given the value -0.75 with a scale of 1: The scale of 1 means that our final Decimal value is going to be the integer -8 because of rounding.
There was a problem hiding this comment.
Thank you for the explanation!
| // Since we're going to truncate this to get an integer, we need to round | ||
| // the value instead because of edge cases so that we match how other implementations | ||
| // (e.g. C++) handles Decimal values. So if we're negative we'll subtract 0.5 and if | ||
| // we're positive we'll add 0.5. | ||
| p := (&big.Float{}).SetFloat64(float64PowersOfTen[scale+38]) | ||
| p := (&big.Float{}).SetInt(scaleMultipliers[scale].BigInt()) |
There was a problem hiding this comment.
A-ha! I was reviewing commit by commit :)
github-actions
bot
added
awaiting change review
github-actions
bot
added
awaiting merge
…ns (apache#38426) ### Rationale for this change Slight modifications to the `ToString` and `FromString` methods of decimal 256 to ensure proper accurate and proper conversion given precision and scale. ### Are these changes tested? Yes * Closes: apache#38395 Authored-by: Matt Topol <[email protected]> Signed-off-by: Matt Topol <[email protected]>
|
After merging your PR, Conbench analyzed the 5 benchmarking runs that have been run so far on merge-commit 9e2d4aa. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them. |
…ns (apache#38426) ### Rationale for this change Slight modifications to the `ToString` and `FromString` methods of decimal 256 to ensure proper accurate and proper conversion given precision and scale. ### Are these changes tested? Yes * Closes: apache#38395 Authored-by: Matt Topol <[email protected]> Signed-off-by: Matt Topol <[email protected]>
…ns (apache#38426) ### Rationale for this change Slight modifications to the `ToString` and `FromString` methods of decimal 256 to ensure proper accurate and proper conversion given precision and scale. ### Are these changes tested? Yes * Closes: apache#38395 Authored-by: Matt Topol <[email protected]> Signed-off-by: Matt Topol <[email protected]>
Rationale for this change
Slight modifications to the
ToStringandFromStringmethods of decimal 256 to ensure proper accurate and proper conversion given precision and scale.Are these changes tested?
Yes