akubera / bigdecimal-rs

Arbitrary precision decimal crate for Rust
Other
302 stars 73 forks source link

Incorrect BigDecimal Format! output #140

Closed DBoehm-Superstate closed 4 weeks ago

DBoehm-Superstate commented 1 month ago

BigDecimal does not format correctly for some values.

let result:BigDecimal = BigDecimal::from(999999991) / 1e6;
assert_eq!(format!("{:.2}", result), "1000.00");

I would expect this assert to succeed, but format!("{:.2}", result) yields "100.00". I could at least believe "999.99" but "100.00" seems like a bug.

Interestingly both of the below give the expected answer.

assert_eq!(format!("{:.2}", result.to_f64().unwrap()), "1000.00");
assert_eq!(format!("{:.2}", result.round(2)), "1000.00");
DBoehm-Superstate commented 1 month ago

apply_rounding_to_ascii_digits appears to never expect the rounding to affect the integer_digit_count, causing the decimal to continue to be inserted at after digit 3 instead of after digit 4. An additional 0 also needs to be appended to digits to maintain the requested precision.

DBoehm-Superstate commented 1 month ago

test for impl_fmt.rs

            mod dec_9d99 {
            use super::*;

            fn test_input() -> BigDecimal {
                "9.99".parse().unwrap()
            }

            impl_case!(fmt_default:      "{}" => "9.99");
            impl_case!(fmt_d0:        "{:.0}" => "10");
            impl_case!(fmt_d1:        "{:.1}" => "10.0");
            impl_case!(fmt_d2:        "{:.2}" => "9.99");
            impl_case!(fmt_d3:        "{:.3}" => "9.990");
        }
akubera commented 1 month ago

~It’s rounding to the nearest 2 decimal digits, so it will round up to 100.00, rather than truncate.~ ~If you want a truncated decimal to 2 places, you can use result.with_scale(2)~

Hastily miscounted zeros. You're quite right, this is a bug.

akubera commented 1 month ago

Maybe I misunderstood. It’s saying 100.00 but should be 1000.00? yeah that’s a bug, but might be fixed in a recent pull where round was reimplemented, can you check if trunk works?

akubera commented 4 weeks ago

I've cleaned and reorganized the impl_fmt.rs file in my local repo, fixing this issue. I'll have that up and released soon.

This boils down not to a off-by-one error like I expected, but using the same function for formatting scientific notation and "full" decimals. When rounding caused overflow it incremented the exponent 9e-1 -> 1e0 but the "full" formatting code ignored the exponent.

I've refactored the relevant rounding code to a new function which returns the exponent of the rounded digits, and both functions will handle exponent changes appropriately.

akubera commented 4 weeks ago

174 unit tests and 100% code coverage in impl_fmt.rs wasn't enough. 😮‍💨

I had 0.9, but not 0.99, and that took the wrong branch.

akubera commented 4 weeks ago

Fixed in v0.4.6.

Thanks for reporting this.

DBoehm-Superstate commented 4 weeks ago

Sorry I didn't check back in quickly enough to respond to your question. Thanks for fixing this!

akubera commented 4 weeks ago

Oh that's fine. I was on my way to dinner when I read (and subsequently re-read) the issue, and couldn't properly check it. I was just hoping it had already been fixed in trunk, but it wasn't.

I added some property tests to see if that would have caught the overflow bug, and it actually did not. So thanks again for letting me know.

\