paupino / rust-decimal Goto Github PK

let x: Result<Decimal, Error> = "".parse() panics. It looks like the implementation assumes the string is non-empty.

At the moment, the performance of this function has not been optimized. Essentially the format of scientific notation is similar to the internal representation of Decimal so we should be able to take advantage of this.

the following assertions fails for me using version 0.7.0

assert_eq!(cmp::min(Decimal::from_str("-0.5").unwrap(), Decimal::from_str("-0.01").unwrap()), Decimal::from_str("-0.5").unwrap());

Currently, multiplication can underflow in certain situations from the following lines:

For consistency, we should attempt to round on underflow situations whilst applying checked functions for overflow.

At the moment, there is a lot of variance between runs which can be mitigated with some libraries such as criterion.rs. I want to make sure that an appropriate range of inputs is benchmarked as well as validate that the results are able to be accurately compared for improvements.

This should be relatively easily since we store the negative bit within the flag however would still be a useful function to have.

Hi,

I understand that rust-decimal panics in case of unrecoverable errors. However, Today I encountered two panics that seem odd.

    let rate = Decimal::new(19, 2);
    let part = rate / (rate + Decimal::new(1, 0));
    let result = Decimal::new(169, 0) * part; // panics

This will panic with Multiplication underflowed. However if I switch the operands (to part * Decimal::new(169, 0)), it does not panic anymore.

I tried to also fix this issue by rounding the part variable first, but this panicked as well:

    let rate = Decimal::new(19, 2);
    let part = rate / (rate + Decimal::new(1, 0));
    let part = part.round_dp(2); // panics

It panics with Cannot have an exponent greater than 28 (29 > 28).

Thanks

Ultimately, we should follow a similar standard to floats and provide a consistent API. This will involve deprecating both is_positive and is_negative methods so that we replace with is_sign_positive and is_sign_negative

the following code fails to compile:

let x = dec!(7314);
let y = -x;

with error:

40 |     let y = -x;
   |             ^^ cannot apply unary operator `-`
   |
   = note: an implementation of `std::ops::Neg` might be missing for `rust_decimal::Decimal`

do you want std::ops::Neg implemented? then i'd do so and open a PR : )

Currently, strings with _ separators aren't parsing correctly, occasionally causing substraction overflows.

Currently writing to Postgres requires a string conversion. There may be an opportunity to avoid this conversion and consequently improve write performance.

CSV deserialize error: record 1 (line: 2, byte: 523): invalid type: floating point `17.19287`, expected a Decimal type representing a fixed-point number

It doesn't seem to be related to parsing, since Decimal::from_str("17.19287") works fine.

Was just wondering if there were a reason why postgres is a default feature?

I'm interested in ability of saving/loading rust Decimal in Postgres number and saw some implementation in this crate. But I have a list of questions.

What is current status of integration with Postgres? (ready to use or need some changes)
Is there any documentation about this feature?
Is there any restrictions of usage for current implementation?

Currently user documentation is relatively sparse. To help ease new adoption documentation needs to be written against Decimal.

Would it be worth adding #[derive(Hash)] to Decimal?

Right now FromPrimitive is implemented with the default f32/f64 behavior of rounding to an int.... I would propose that there be a lossy implementation or that it always return None -- I much prefer the former.

In C# it is handled as follows:

When you convert float or double to decimal, the source value is converted to decimal representation and rounded to the nearest number after the 28th decimal place if required. Depending on the value of the source value, one of the following results may occur:

If the source value is too small to be represented as a decimal, the result becomes zero.

If the source value is NaN (not a number), infinity, or too large to be represented as a decimal, an OverflowException is thrown.

This results in lo being -1 instead of correctly setting the sign bit.

currently Decimal derives Debug

when printing out for a example a vec of some structs containing decimals with {:?} or {:#?}
in place of the decimals one gets a lot of Decimal { flags: 131072, hi: 0, lo: 3260, mid: 0 }
and has to do additional annoying work to print the actual number represented.

implementing Debug like Display and printing the number represented by the decimal would help me a lot.

is this something you want?

would be happy to implement this then!

I'm doing some math in my rust code and it looks like the scale is messed up:

((844.13000000 -  843.65000000) * 0.1818181818181818181818181818 ) + 843.65000000
              ^^^              ^^^                                ^^^
      0.48000000      0.0457142857142857142857142856     84369571428571428571.42857143

It looks like there is an overflow. Shouldn't this be a panic?

Commonly, amounts in JSON are sent unquoted, i.e.

{ "amount": 100.20 }

At the moment, this causes the deserialization to raise an error whereas it'd be convenient if it could also parse this format successfully.

pub fn trunc(self) -> Decimal
Returns the integer part of a number.

pub fn fract(self) -> Decimal
Returns the fractional part of a number.

There are a number of functions in the library that can be optimized, however before optimization it'd be nice to be able to measure performance (so that performance gains are measured).

The following code:

let a = Decimal::new(100, 2);
let b = Decimal::new(300, 2);
let c = a / b;
println!("{}", c);
println!("{:.2}", c);

produces the output:

0.3333333333333333333333333333
0.

This is because the implementation converts to a string first and applies the pad method. Decimal should implement further format instructions to support producing 2dp without requiring rounding first up.

I was having a quick look through your code, and I see you have a new trait called ToDecimal which simply converts integer types into a decimal. I was curious as to why you didn't opt for the Into and From traits from the standard library which do exactly that?

Would you like a pull request to remove the ToDecimal trait and replace it with the standard library equivalent?

Current benchmarks on 2.3GHz i7, 16GB DDR3, macOS High Sierra:

test bench_decimal_mul ... bench:   1,560,175 ns/iter (+/- 379,456)

To help cover unknown edge case testing, look into implement QuickCheck.

I did my own implementation, but I don't think it's pretty and/or perfomant, but I'd like to have support for numbers like "9.7e-7", "1.2e10" etc.

Currently round_dp limits to 20dp to assist in pow10 lookups however is an unnecessary arbritrary limit. This should be updated to include an overflow word instead of relying on default implementations.

Currently the deserialize implementation calls the serde deserialize_any function, which breaks Decimal serialization with non-self describing formats like bincode [0].

When implementing Deserialize, you should avoid relying on Deserializer::deserialize_any unless you need to be told by the Deserializer what type is in the input. Know that relying on Deserializer::deserialize_any means your data type will be able to deserialize from self-describing formats only, ruling out Bincode and many others.

There are two possible easy fixes:

• Replace deserialize_any with deserialize_str
• Simply have Decimal derive Serialize, Deserialize

[0] https://serde.rs/impl-deserialize.html

My small experiments shows that implementation Decimal on top of types i128/u128 should be simpler and more performant. In my rough reimplementation of add method, timings of bench add_negative_pi drops from 133 ns/iter to 41 ns/iter. As I know, i128/u128 types supported on all rust targets.
I suppose it enough reasons to consider reimplementation or alternative implementation of Decimal type on top of i128/u128.

These operations should be the easiest to optimize.

Current benchmarks on 2.3GHz i7, 16GB DDR3, macOS High Sierra:

test bench_decimal_add ... bench:   1,408,588 ns/iter (+/- 219,523)
test bench_decimal_sub ... bench:   1,357,040 ns/iter (+/- 256,864)

The current implementation of div focused on correctness over performance therefore can use some optimization.

Current benchmarks on 2.3GHz i7, 16GB DDR3, macOS High Sierra:

test bench_decimal_div ... bench:  47,344,736 ns/iter (+/- 6,536,510)

There are some areas where panic is warranted however there are others (e.g. rescale, from_bytes_le) that we could utilize Rust's error handling further. In addition, with libraries such as error-chain we can perhaps make the error handling experience richer with little overhead.

Creating a new decimal object is rather verbose. It'd be nice to have a shorthand option to define a decimal such as: 12.4M. This could possibly be implemented via a build.rs file.

At the moment, there are a lot of tests at the end of each file. It should not be difficult to move these tests over into the tests/ directory, and that'll help keep the main source files nice and tidy. If for some reason a test can't be moved outside, it should be moved into a separate submodule (as opposed to being their own stand-alone functions).

The introduction of add_assign, mul_assign et al allows us to easily do an arithmetic operation and an assign in a single operation. e.g. x += 2.

Extend the test suites so that the tests are more dynamic. e.g. loop through an array of examples + results asserting along the way.

As part of this I want to also implicitly test transitive operations as appropriate. e.g. a + b = b + a, a * b = b * a.

The following test cases will fail with Decimal Overflow due to rescaling

    #[test]
    fn test_max_compares() {
        let x = "225.33543601344182".parse::<Decimal>().unwrap();
        let y = Decimal::max_value();
        assert!(x < y);
        assert!(y > x);
        assert!(y != x);

    }
    #[test]
    fn test_min_compares() {
        let x = "225.33543601344182".parse::<Decimal>().unwrap();
        let y = Decimal::min_value();
        assert!(x > y);
        assert!(y < x);
        assert!(y != x);
    }

thanks for your work on this crate : ) so far it's been very useful to me.

i've replaced some code that previously used f64s by Decimals.
the code did some plotting in the end.
when comparing the plots i noticed a lot of differences.
i was able to reduce it to a minimal example:

    let mut x = dec!(7314.6229858868828353570724702);
    let y = dec!(1000);
    x += y;
    println!("{}", x);

prints:

391.8067344604490760026774366

this seems pretty bad to me : /

i guess it's an overflow. i think it should absolutely panic in this case instead of silently breaking math!

using rust decimal 0.8.1 and rustc 1.27.0-nightly (f0fdaba04 2018-05-15)

It'd be handy to see where by PR's are causing any performance hits/improvements. This can be integrated using Travis as per http://sunjay.ca/2017/04/27/rust-benchmark-comparison-travis

We currently support a round function - it's natural to also extend this to also support floor (Returns the largest integer less than or equal to a number) and ceil (Returns the smallest integer greater than or equal to a number) functions.

To help alleviate some of the panic scenario's currently present in the library we should introduce checked methods. Checked returns an Option<Decimal> and returns None on overflow.

Currently, rescaling can cause a silent overflow during some common operations which leads to incorrect results. A good test case as raised by @rkusa is:

---- it_adds_decimals stdout ----
	thread 'it_adds_decimals' panicked at 'assertion failed: `(left == right)`
  left: `"12.45378151260504201681"`,
 right: `"4.5309652611786082574523276562"`: 11.815126050420168067226890757 + 0.6386554621848739495798319328', tests/decimal_tests.rs:168:8

In this particular case, 11.815126050420168067226890757 (scale 27) is being rescaled to a representation of 28. This causes an expected overflow logically however because the overflow is consequently ignored it leads to incorrect results. Behind the scene's it currently loses the significance and converts 11.815126050420168067226890757 to 3.8923097989937343078724957234. Consequently, 0.6386554621848739495798319328 + 3.8923097989937343078724957234 = 4.5309652611786082574523276562.

In order to resolve this; the rescale logic should be refactored and instead return an extra 32 bit's for overflow, which is ultimately used in any resulting calculations (as required).

Version: 0.7.2 (c22883b)

Decimal panics when formatting precision on a fractional number between 1 > n > -1 which contains leading zeros.

let foo: Decimal = "0.0023554701772169".parse().unwrap();
println!("{:.4}", foo);

thread 'main' panicked at 'attempt to subtract with overflow', /home/terry/.cargo/git/checkouts/rust-decimal-5e2779474ac0f95f/c22883b/src/decimal.rs:1658:30

The expected output is:

0.0023

Have you any plans for stabilization or moving forward in short term for this crate?

Continuous integration should be added to test the crate on Rust stable, beta and nightly.

Along with this, it will possible to have the master branch be protected so that only changes which pass the CI can be merged, and then the main development will happen on the develop branch (where things might break temporarily).

Currently we precalculate constants based on the number of groups that a postgres type has. We do this at runtime upon read. Each runtime group is 10000x the previous group.

We could comfortably pre-calculate this set of constants since they are the same between reads which should improve read performance.