volatiletech / sqlboiler

Generate a Go ORM tailored to your database schema.
BSD 3-Clause "New" or "Revised" License
6.76k stars 545 forks source link

Use a decimal library for v3 #162

Closed aarondl closed 6 years ago

aarondl commented 7 years ago

Currently we use a string to represent decimal to avoid people doing mathematical operations on decimal types as float64, except for a bug where Postgres uses float64 currently :( :( :( Brought up in this issue: https://github.com/vattle/sqlboiler/issues/100

Also in that issue one user makes the recommendation that we just use a decimal library. I think this kind of makes sense, we were trying to stay away from dependencies but can we truly justify it since we rely so heavily on the null package? It looks stable enough that we don't have to fork it unlike some other packages we depend on.

Let your thoughts be heard here. This is still on the fence.

pennersr commented 7 years ago

Great! My thoughts on this point:

orian commented 6 years ago

I've used a https://github.com/shopspring/decimal in some projects and it worked just fine. As Go is not planning to add decimal using external lib seems plausible.

aarondl commented 6 years ago

@orian Are you sure that they haven't already? I was under the impression that this was one.

https://golang.org/pkg/math/big/#Float

Though it's API seems a bit cumbersome (you have to manage precision pretty carefully).

aarondl commented 6 years ago

Also currently I'm eyeing https://github.com/ericlagergren/decimal as the most likely candidate.

orian commented 6 years ago

@aarondl I've used shopsprint lib only to fetch data from DB and pass further, no real arithmetics.

As far as I understand big.Float is a custom precision float but still a floating point (example: https://play.golang.org/p/Xb7f4_OYi7Q)

I've previewed ericlagergren/decima, seems decent, more complex but more mature and no a real bugs opened.

What's you criteria?

aarondl commented 6 years ago

@orian You're correct about big.Float. I think the ericlagergren library is just a well made library, has some nice math features as well as much better performance. It has few bugs and the maintainer seems generally responsive about dealing with issues.

Just a great decimal library I think is what we need since the standard lib isn't coming through for us. Something that people can rely on and is performant and useful.

orian commented 6 years ago

It has some Postgre support: https://github.com/ericlagergren/decimal/tree/master/sql/postgres but does not have a distinct NullDecimal and Decimal. Not sure how this should be handled. Would *Decimal be good, or to have Decimal and to add NullDecimal?

I've checked the code, modifying the postgres.go is good place to start? https://github.com/volatiletech/sqlboiler/blob/master/bdb/drivers/postgres.go

aarondl commented 6 years ago

Yep. That's the right place. We'd also need a null decimal for full support. Though I'd ask any changes to be made on the v3 branch.

Dexus commented 6 years ago

What speaks against https://github.com/shopspring/decimal?

aarondl commented 6 years ago

@Dexus When I looked at the issues that were opened in shopspring at the time, I didn't like the problems they were having in the library as they seemed like scary problems to have for a math lib. NaN/+-Inf are not supported, as well as worse performance where the ericlagergren library has fewer bugs (admittedly maybe because less use but who knows), better performance, and more features. So it seems like shopspring has nothing going for it other than being slightly more popular.

orian commented 6 years ago

I've sent an implementation of NullDecimal: https://github.com/ericlagergren/decimal/pull/98

And I've started to work on decimal support in postgre: https://github.com/volatiletech/sqlboiler/compare/v3...orian:patch-1 I need to extend types package and add tests. I'll try to get it done by tomorrow (UTC+2).

aarondl commented 6 years ago

Oh wow, that's awesome @orian. There is a bit of background as to why this isn't done yet however! https://github.com/ericlagergren/decimal/issues/95

One of the problems with his postgres implementation is it has validation rules applied on the client side that are particular to postgres' decimal type. So we were going to make a generic one and were just dancing around the "how do we make the syntax work nicely" issue, though he hadn't responded to me in a bit. I needed to remind him, but I was just waiting on it while tackling other issues in v3. I'd be very happy if you were to help out though, but I'd hope it coincided with the talks in issue 95.

Thanks for looking into it.

aarondl commented 6 years ago

Done in v3. Ping @orian just so you know :)

orian commented 6 years ago

Thanks, I have bit of crunch time.

saulortega commented 6 years ago

Hello, @aarondl . Why double precision in postgres is Decimal? A mistake?

psql.go:

case "decimal", "numeric", "double precision":
    c.Type = "types.Decimal"
aarondl commented 6 years ago

Definitely a mistake @saulortega, fixed in rc9 freshly released.