Add BigDecimal Crate by akubera · Pull Request #224 · rust-num/num

akubera · 2016-09-16T18:26:39Z

Rebase of pull request #193. Addresses issue #8.

Implementation of BigDecimal, which comprises a BigInt and a u64 scale factor, allowing for storage of 2^64 digits.

Further implementation is required to set the default maximum precision (so numbers like 1/3 are not represented by 2^64 digits). The current default is 100.

Add, Sub, Mul implemented by @iterion, I reimplemented Div, Eq, FromStr.

Div is currently broken, as it only returns the integer portion instead of a float as one might expect. Other traits are stubbed out to satisfy the Num trait.

…dded support for exponential form in from_str.

…xed implementation of eq() so BigDecimals with large scale differences are compared correctly

…c if exponent is not a valid integer.

… Err Results. Empty string now returns error (instead of zero). Added tests to test bad strings

cuviper · 2016-09-20T20:30:37Z

I still need to sit down and do a full review of this, but since you're apparently tweaking the parsing right now -- it looks over-complicated to me. I would actually expect this to be a rather "dumb" wrapper -- just finding the decimal point if it exists and letting BigInt do the real work (actually BigUint working under that), including your enhancement for dealing with underscores.

cuviper · 2016-09-20T20:48:49Z

I know you are leaning on BigInt and BigUint to some extent, but I think most of the char-parsing is out of place. I think it should be possible to only worry about '.' and the 'e' exponent here, and leave everything else to the lower layer.

akubera · 2016-09-20T21:44:23Z

Ok, I believe a well placed partition method can simplify things. I'll see what I can do.

akubera · 2016-09-21T00:27:33Z

Errr, partition isn't what I thought it was (python's str.partition). It's now a few split_at calls and special case handling.

Question: Should I support underscores?

… searches and splits instead of copying everything into a vector. Added new support for raising i32 parsing errors.

…e BigInt parser implementation)

…the decimal_offset algorithm.

akubera · 2016-09-21T01:32:02Z

Ok, I re-read and see now that underscore support should be removed. That's been done, and the from_str_radix function I think is ready for review. There are lots of comments, I hope that's ok.

akubera · 2016-09-21T01:48:41Z

In terms of the decimal point, I find it and (if found) push the characters before and after it into a String, and pass that to the BigInt. I think that requires a copy. I don't know if there's a better way around that (aside from a BigInt constructor from Iterator and chain the two slices).

cuviper

OK, here are some more points for you to work on.

I'm especially concerned about the computation required in eq though. We should research/explore if there are other representations that would be more convenient.

cuviper · 2016-09-21T04:51:29Z

+bigdecimal = ["num-bigdecimal"]
 complex = ["num-complex"]
 rational = ["num-rational"]
 default = ["bigint", "complex", "rational", "rustc-serialize"]


You should add it to the default features here.

cuviper · 2016-09-21T04:52:27Z

@@ -0,0 +1,39 @@
+[package]
+authors = ["The Rust Project Developers"]
+description = "Big integer implementation for Rust"


Need to update this.

cuviper · 2016-09-21T04:53:32Z

+
+[dependencies.serde]
+optional = true
+version = "0.7.0"


Take a look at bigint for how we're now allowing either serde 0.7 or 0.8.

cuviper · 2016-09-21T04:54:49Z

+
+[dependencies.rand]
+optional = true
+version = "0.3.14"


Are you actually using rand?

cuviper · 2016-09-21T04:55:51Z

@@ -0,0 +1,637 @@
+// Copyright 2013-2014 The Rust Project Developers. See the COPYRIGHT


It's 2016. Not that we've been very diligent about keeping these updated, but at least start on the right foot. :)

cuviper · 2016-09-21T05:43:32Z

+    fn add(self, other: &BigDecimal) -> BigDecimal {
+        let scale = max(self.scale, other.scale);
+        let left = self.clone().set_scale(scale);
+        let right = other.clone().set_scale(scale);


See, set_scale is weird so it seemed a clone was needed here, though it's currently cloning on its own.

The math equations would benefit from a single clone (or rather "rescale") and then an inplace operator on the new object. It looks like std::ops::AddAssign was marked stable in 1.8; which means I cannot use that, right? Is there a way to write two implementations based on rustc version?

cuviper · 2016-09-21T05:53:32Z

+
+impl fmt::Display for BigDecimal {
+    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
+        f.pad_integral(true, "", &self.int_val.to_str_radix(10))


This will need a decimal point, or trailing zeros, or perhaps just an e+123 sort of suffix.
(Hmm, the scale is the opposite of a normal exponent -- that's kind of awkward now that I notice it.)

cuviper · 2016-09-21T06:00:24Z

+            Some(loc) => {
+                let (base, exp) = s.split_at(loc);
+                // slice after 1 to skip the 'e' char
+                (base, try!(i64::from_str(&exp[1..])))


We currently still support Rust 1.0, but split_at wasn't added/stabilized until 1.4.

But just s[..loc] and s[loc+1..] should be fine. (It's mutable slicing where you get into trouble.)

(&s[..loc], &s[loc..]), the beauty of half open ranges.

libs people don't think bumping the rust version is breaking rust-lang/rfcs#1619 (comment) Every such thing is a judgment call though, of course.

@bluss I think they're wrong about that, and the comment you linked acknowledges that this idea was the most contentious part of that RFC. But this is not the place to argue that.

fair enough

cuviper · 2016-09-21T06:03:11Z

+
+    /// Creates and initializes a BigDecimal.
+    #[inline]
+    fn from_str_radix(s: &str, radix: u32) -> Result<BigDecimal, ParseBigDecimalError> {


It's a little strange to support a radix on something called "decimal", but hey, that's what Num wants... :)

Do you think I should just Err if radix != 10?

Sure, I think that's reasonable.

cuviper · 2016-09-21T06:06:22Z

+            }
+        };
+
+        let scale = decimal_offset - exponent_value;


Hmm, actually on the radix point, this is wrong for radixes other than 10, and others can't be perfectly converted.
(e.g. "0.1" radix 3 is 1/3)

cuviper · 2016-09-21T06:24:33Z

On the underscore support, I'm OK with having that -- it just belongs in BigUint's parser. That can be a PR independent of this one, if you'd like a smaller task. :)

akubera · 2016-09-21T13:19:45Z

Thanks for the review. Most things not from_str_radix and eq were written by @iterion, so I don't know if there was, for example, an intention to use rand or not.

Is adding "bigdecimal" to the Makefile all that's needed for travis testing to work?

I'll revisit the project this weekend.

hauleth · 2016-09-21T13:25:34Z

@akubera yep, that should be everything what you need.

iterion · 2016-09-21T15:08:37Z

@akubera no idea why I added rand, my guess was it was just a copy paste from somewhere else that got left in.

Edit: believe the same is true for Serde, I was just copying some things from other files in the project that was boilerplate.

…ption and updated serde version.

…igInt errors. Removed `split_at` usage in favor of slices.

cuviper · 2016-09-26T06:57:34Z

I'm not sure what tangible benefit you'd get from AddAssign. When you have a local object, you can just move it by value into Add and accept what comes out. That is, "x += y" and "x = x + y" are not much different.

…e efficient)

t-botz · 2016-12-07T03:26:22Z

What is the status on that? Rust is definitely lacking an implementation of BigDecimal.
I would be keen to work on it, I am just wondering what would be required.

akubera · 2016-12-07T15:31:18Z

It works insofar the tests for construction and arithmetic pass, but the implementation is not as efficient as it could/should be. As it stands, it uses a BigInt to store the numerical data, so we lose access to the BigDigits which could be used for optimization for, say, testing equality.

The other thing to worry about is how to manage precision. Right now I limit decimal places in the division operator with an arbitrary number. The python decimal module (I almost typed 'crate', haha) uses thread-scoped context objects to manage such information; that's probably not the way to go for Rust. I haven't worked with/looked at any other such library, someone should throw ideas around.

I don't know the industry standards for such a library: is a goal keeping track of significant figures? should (1.00 == 1.00000)? should there be a function that returns false in that case? Should it coupled to the num-rational crate? Should every object have their own "context" properties? What rules govern objects with different such properties?

I'd need input from experts before moving forward, and I've been busy and haven't had time to work on this for couple months. Any input would be appreciated.

rpedela · 2016-12-29T19:02:25Z

I don't know the industry standards for such a library: is a goal keeping track of significant figures? should (1.00 == 1.00000)? should there be a function that returns false in that case? Should it coupled to the num-rational crate? Should every object have their own "context" properties? What rules govern objects with different such properties?

I think it may be worth looking at the source for Postgres's "numeric" type. The PG community cares a lot about correctness and I am willing to bet they have tackled many of your non-Rust-specific questions.

Github mirror: https://github.com/postgres/postgres
hacker mailing list: https://www.postgresql.org/list/pgsql-hackers/

homu · 2017-02-10T00:20:11Z

☔ The latest upstream changes (presumably #263) made this pull request unmergeable. Please resolve the merge conflicts.

rubdos · 2017-04-04T09:23:15Z

☔️ The latest upstream changes (presumably #263) made this pull request unmergeable. Please resolve the merge conflicts.

I rebase the thing on master, should I make a new PR?

rubdos · 2017-04-04T09:29:01Z

I think it may be worth looking at the source for Postgres's "numeric" type. The PG community cares a lot about correctness and I am willing to bet they have tackled many of your non-Rust-specific questions.

IRC:

2017-04-04 11:25:22 rubdos Someone knows out of the top of their head whether postgresql considers NUMERIC 1.00000 == 1.0 ?
2017-04-04 11:25:37 johto yes

iterion · 2017-04-04T15:49:41Z

@rubdos yeah, a new PR is how we originally handled it when I ran out of time to work on this. So, 👍 from me. Would still love to see this merged as well as BigDecimal support for diesel, which is why I originally started this!

akubera · 2017-04-04T16:11:43Z

Yes, that is fine with me.

cuviper · 2017-04-04T16:49:06Z

I think what might be best for this is to develop BigDecimal as a standalone project.

There's clearly a lot of interest in having this type, and I feel bad about dragging my feet. But at the same time, there are still design questions that I'm not terribly comfortable with, and don't have great ideas how to make it better. Yet we rust-num maintainers are the ones stuck with the maintenance bill if we merge this. Plus, we treat num stability very conservatively, which doesn't mesh well with a crate that may still need nascent development iteration.

So, how would you folks feel about starting a separate project for BigDecimal? I think the only thing you'd lose is getting a re-export from the base num crate, but IMO that's not such a big deal. Is there any reason you think this needs to be part of rust-num/num?

akubera · 2017-04-04T17:46:30Z

Other than relieving people of the paradox of choice, I don't think it would need to be in num.

I think it'd be fine to have a few competing implementations, and perhaps one day there would be one that is unarguably better than the others, and be blessed by the num developers and merged into the project. I certainly like having a "one-stop-shop" crate handling multiple similar needs, and I would trust that the maintainers of such crates have worked on ensuring quality and interoperability.

I can think of two options:

Let anyone interested (@rubdos) create their own repository and work on their own. Maybe rust-num/num keeps an issue open keeping track of the status of participating projects.
A rust-num owner could create a rust-num/bigdecimal or rust-num/bigdecimal-nursery repository, with different implementations managed on different branches. Tradeoffs and designs can be discussed & implemented in this repo, and collectively people learn what works and what doesn't.

2 is obviously more difficult, but would provide people who know what they're doing an easy-to-find place to comment, and would reduce reinvention of the wheel for anyone who wants to start something new.
Also, providing a standard testing+benchmark suite would be nice.

iterion · 2017-04-04T18:13:02Z

Overall, I'm cool with the idea of separating this out into its own package. Ultimately, if the implementation stabilizes then rust-num/num would be a great place for it. But, we're not there yet and it also assumes that a single standard can be agreed upon. That said, it seems other languages make that choice for you: ruby, python, etc.

I like some form of @akubera's option 2, as it doesn't necessarily preclude competing standards while still making it easy for newcomers to find some sort of community agreed upon decimal implementation. I'm happy to volunteer to help maintain any project that is created as well.

rubdos · 2017-04-04T19:29:36Z

It looks like Add is bugged; I'm getting invoices of a few trillion euro's :D

rubdos · 2017-04-04T19:33:16Z

Is there any reason you think this needs to be part of rust-num/num?

Yes. Obviously because rust-num is the place to be for everything number related, and I think avoiding separation would enhance potential cohesion of the Rust ecosystem.

I'm totally okay with splitting off; the only remaining question will be whether Diesel would still merge something that pulls from rubdos/big-decimal-rs instead of rust-num. Btw, the Diesel implementation is linked in somewhere here, and I must say that it's pretty neat.

rubdos · 2017-04-04T19:36:35Z

You know what; I'll make the merge request for a split-off crate. You guys leave it open, review from now and then, and in a few months, when things stabilize, we merge again. In the meantime, I'll maintain a separate crate for this alone. Sounds like an idea?

cuviper · 2017-04-04T19:42:45Z

I think you're putting too much emphasis on rust-num's "blessing" -- we're just a couple of people who volunteered to maintain num when it was booted from the standard library. Beyond that, there's no connection to rust-lang proper as far as having some sort of official sanctioning power. But maybe I underestimate the network effects, who knows.

The separate project doesn't need to be treated as a competition, unless you folks disagree about something between yourselves. Just have someone open a personal repo and invite the others to collaborate. Later down the road, we can evaluate whether it makes sense to merge into rust-num/num, move it into rust-num/big-decimal, or just continue living on its own.

@rubdos if you really want to keep an open merge request here, I guess that's ok too.

sgrif · 2017-04-04T20:04:16Z

Someone please release a crate somewhere so I have a type to target for the PG decimal type. I'm sick of telling people that I'm not going to implement FromSql<Decimal> for f64

akubera · 2017-04-04T22:40:36Z

I've made a bigdecimal repository maintaining the history of the subdirectory here: akubera/bigdecimal-rs, I assume @iterion is ok with that. I've ignored the 1.0.0 compatible commits, so I only know it's compatible with stable (1.16.0) and beyond. @rubdos, you're free to clone (i.e. not github fork but hard clone and push to your own repo) and use however you and the Desiel project wishes (I started cherry-picking before I realized you said you'd fork it).

rubdos · 2017-04-05T10:58:39Z

I've made a bigdecimal repository maintaining the history of the subdirectory here: akubera/bigdecimal-rs, I assume @iterion is ok with that. I've ignored the 1.0.0 compatible commits, so I only know it's compatible with stable (1.16.0) and beyond. @rubdos, you're free to clone (i.e. not github fork but hard clone and push to your own repo) and use however you and the Desiel project wishes (I started cherry-picking before I realized you said you'd fork it).

I'd rather work together then. Want to meet up on an IRC channel, or shall I open some issues on your repo?

cuviper · 2017-04-05T19:31:24Z

I found a few existing crates too:

decimal: fixed 128-bit representation
rust_decimal: also fixed 128-bit
decimate: generic significand, could be BigInt

I'd take a close look at decimate then...

rubdos · 2017-04-06T12:12:10Z

It's probably worth noticing that code based on @iterion's work has been pushed to the bigdecimal crate, and is further developed at @akubera 's repo

cuviper · 2017-04-06T16:11:27Z

Cool - happy hacking everyone!

Fixes rust-num#224

cuviper · 2017-10-07T21:26:31Z

I'm trying to clean up old PRs. Since you all have moved to a separate crate, I think that it's not helpful to leave this open. Hope it's going well!

iterion and others added 9 commits September 16, 2016 13:55

Add BigDecimal Add, Sub, Mul, and Div

e7e3e76

Div is currently broken, as it only returns the integer portion instead of a float as one might expect. Other traits are stubbed out to satisfy the Num trait.

Simplify Add and Sub

6dd4b9d

Updated documentation and fixed some unit-tests

d986652

Reimplemented from_str_radix to handle decimal points correctly

4bd0f33

BigDecimal: Implemented a correct version of divide.

7f2f8bc

BigDecimal: Rewrote equality test to correct for scale differences. A…

e78b39a

…dded support for exponential form in from_str.

BigDecimal: Added more test conditions for math operators

6706258

BigDecimal: Added equality tests to ensure that == works properly. Fi…

421f8bc

…xed implementation of eq() so BigDecimals with large scale differences are compared correctly

Updated bigint & num-traits dependency versions to 0.1.35

80be642

akubera mentioned this pull request Sep 16, 2016

Add BigDecimal #193

Closed

akubera added 2 commits September 20, 2016 14:38

BigDecimal: Added some more tests, and from_str_radix will now pani…

1bf9fb2

…c if exponent is not a valid integer.

BigDecimal: Changed parsing error-handling from panicing to returning…

48f297f

… Err Results. Empty string now returns error (instead of zero). Added tests to test bad strings

akubera added 3 commits September 20, 2016 20:51

BigDecimal: Rewrite of from_str_radix method to do less parsing - now…

5ea8911

… searches and splits instead of copying everything into a vector. Added new support for raising i32 parsing errors.

BigDecimal: Removed support for underscores in string (deferred to th…

e07c9db

…e BigInt parser implementation)

BigDecimal: Fully removed underscore support and slightly simplified …

651b4f7

…the decimal_offset algorithm.

Added bigdecimal to Makefile for testing

2c04aae

cuviper requested changes Sep 21, 2016

View reviewed changes

akubera added 2 commits September 26, 2016 01:33

BigDecimal: Cargo.toml - Removed unnecessary rand crate, fixed descri…

9872dc0

…ption and updated serde version.

BigDecimal: Implemented ParseBigDecimalError variants for integer & B…

d7d54cd

…igInt errors. Removed `split_at` usage in favor of slices.

akubera added 2 commits October 5, 2016 13:40

BigDecimal: Changed code to be compatible with rust 1.0.0

e1ecf31

BigDecimal: Re-implemented ten_to_the to be recursive (and MUCH mor…

9793d41

…e efficient)

cuviper mentioned this pull request Feb 24, 2017

Any plans for Fixed Point numbers? #265

Closed

rubdos mentioned this pull request Apr 4, 2017

numeric/BigDecimal diesel-rs/diesel#837

Merged

10 tasks

remexre pushed a commit to remexre/num that referenced this pull request Jun 1, 2017

Impl Default for Value

56df4f9

Fixes rust-num#224

cuviper closed this Oct 7, 2017

cuviper mentioned this pull request Jun 24, 2019

Decimal numbers operations #377

Open

rubdos mentioned this pull request Jun 28, 2019

Consider to move the crait in rust_num akubera/bigdecimal-rs#60

Closed

		@@ -0,0 +1,637 @@
		// Copyright 2013-2014 The Rust Project Developers. See the COPYRIGHT

Conversation

akubera commented Sep 16, 2016

Uh oh!

cuviper commented Sep 20, 2016

Uh oh!

cuviper commented Sep 20, 2016

Uh oh!

akubera commented Sep 20, 2016

Uh oh!

akubera commented Sep 21, 2016

Uh oh!

akubera commented Sep 21, 2016

Uh oh!

akubera commented Sep 21, 2016

Uh oh!

cuviper left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuviper Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuviper Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cuviper commented Sep 21, 2016

Uh oh!

akubera commented Sep 21, 2016

Uh oh!

hauleth commented Sep 21, 2016

Uh oh!

iterion commented Sep 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cuviper commented Sep 26, 2016 via email

Uh oh!

t-botz commented Dec 7, 2016

Uh oh!

akubera commented Dec 7, 2016

Uh oh!

rpedela commented Dec 29, 2016

Uh oh!

homu commented Feb 10, 2017

Uh oh!

rubdos commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rubdos commented Apr 4, 2017

Uh oh!

cuviper Sep 21, 2016 •

edited

Loading

cuviper Sep 21, 2016 •

edited

Loading

iterion commented Sep 21, 2016 •

edited

Loading

rubdos commented Apr 4, 2017 •

edited

Loading

akubera commented Apr 4, 2017 •

edited

Loading