New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle bigint as string to prevent precision loss #353

Merged
merged 2 commits into from Jun 19, 2013

Conversation

6 participants
@sevastos
Contributor

sevastos commented May 20, 2013

TIL dealing with bigints in javascript is a bit of a pain.

I had a weird issue of inserting correctly bigints on the db but getting off by a bit numbers on the response.

Apparently all the numbers in javascript are floating-point numbers1 and the accuracy is only assured for integers ±253 between -9007199254740992 and 9007199254740992.
However bigint ranges ±263: -9223372036854775808 to 9223372036854775807 2.

The most common solution for handling bigint data on javascript is carry them with strings and use special libs for math operations. So strings it is.

However, I wonder if it may cause any troubles to anyone that is treating bigint as number on javascript-level and do some checks around that. Though they shouldn't since that only works till the ±253 range and is not the right way.

Anyway let me know what you think.
Thanks

@rpedela

This comment has been minimized.

Show comment
Hide comment
@rpedela

rpedela May 20, 2013

Contributor

Related to #339.

Contributor

rpedela commented May 20, 2013

Related to #339.

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc May 20, 2013

Owner

thanks for bringing this up. There's been discussion around it but nothings' really completely addressed it yet. I'll keep this open & really soon get to dealing with this more properly. Sorry for the headache you ran into so far!

Owner

brianc commented May 20, 2013

thanks for bringing this up. There's been discussion around it but nothings' really completely addressed it yet. I'll keep this open & really soon get to dealing with this more properly. Sorry for the headache you ran into so far!

},{
name: 'binary-bigint/int8-full',
format: 'binary',
dataTypeID: 20,
actual: [1, 0, 0, 0, 0, 0, 0, 102],
expected: 72057594037928030
expected: '72057594037928038'

This comment has been minimized.

@sevastos

sevastos May 20, 2013

Contributor

I believe this wasn't correct.

1* 2567 + 102 = 72057594037928038

@sevastos

sevastos May 20, 2013

Contributor

I believe this wasn't correct.

1* 2567 + 102 = 72057594037928038

@sevastos

This comment has been minimized.

Show comment
Hide comment
@sevastos

sevastos May 20, 2013

Contributor

Thanks for letting me know @rpedela, for some reason, it didn't came up to my search before.

Hey @brianc, no worries and thanks for your work on this, great stuff. I am completely with you on having a consistent strategy on data type handling, I mostly did it for my project because it's quite urgent.

Contributor

sevastos commented May 20, 2013

Thanks for letting me know @rpedela, for some reason, it didn't came up to my search before.

Hey @brianc, no worries and thanks for your work on this, great stuff. I am completely with you on having a consistent strategy on data type handling, I mostly did it for my project because it's quite urgent.

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc May 23, 2013

Owner

Yeah so the only thing I can see is this requires a major version bump from 1.x to 2.x because it's a breaking change. There are a few other numeric problems execelently described by @rpedela in #339 which probably should be addressed before doing this major version bump since they're also backwards incompatible. I'll try to get all of them put in at the same time and merged. I'm not sure I'll have time for the next 10 days or so because I'm on vacation, but I'll try. If I don't get to it by then I'll try to get to it ASAP. If you were interested in appending the other number parsing changes to this pull request along with test modifications I could most likely merge it as soon as it was ready, but I understand if your time is limited & you can't get to the changes or dont want to. 😄 I'll eventually get to fixing this, it just might be 2 weeks or so.

Now, I'm off to the beach! 🏄

Owner

brianc commented May 23, 2013

Yeah so the only thing I can see is this requires a major version bump from 1.x to 2.x because it's a breaking change. There are a few other numeric problems execelently described by @rpedela in #339 which probably should be addressed before doing this major version bump since they're also backwards incompatible. I'll try to get all of them put in at the same time and merged. I'm not sure I'll have time for the next 10 days or so because I'm on vacation, but I'll try. If I don't get to it by then I'll try to get to it ASAP. If you were interested in appending the other number parsing changes to this pull request along with test modifications I could most likely merge it as soon as it was ready, but I understand if your time is limited & you can't get to the changes or dont want to. 😄 I'll eventually get to fixing this, it just might be 2 weeks or so.

Now, I'm off to the beach! 🏄

@sevastos

This comment has been minimized.

Show comment
Hide comment
@sevastos

sevastos May 23, 2013

Contributor

Sure, I'll give it a try, unless @rpedela wants to take it over. Enjoy your vacations 😃 !

Hey @rpedela, I haven't dealt with decimal and numeric but from what I see decimal is just an alias to numeric, you know if that's the case? Also the serial equivalents of the integer-variants are effectively the integers with a simple constraint for the autoincrement, so no need for new handlers.

Contributor

sevastos commented May 23, 2013

Sure, I'll give it a try, unless @rpedela wants to take it over. Enjoy your vacations 😃 !

Hey @rpedela, I haven't dealt with decimal and numeric but from what I see decimal is just an alias to numeric, you know if that's the case? Also the serial equivalents of the integer-variants are effectively the integers with a simple constraint for the autoincrement, so no need for new handlers.

@rpedela

This comment has been minimized.

Show comment
Hide comment
@rpedela

rpedela May 23, 2013

Contributor

Yes, decimal is an alias for numeric. And yes serial is just a column constraint. I added them for completeness relative to the Postgres number type docs.

I have been working non-stop the last couple months on a product launch set for the beginning of June. I will have more time starting mid-June. This is why I haven't done it already. Sorry about that. But if you want to do it then great!

Contributor

rpedela commented May 23, 2013

Yes, decimal is an alias for numeric. And yes serial is just a column constraint. I added them for completeness relative to the Postgres number type docs.

I have been working non-stop the last couple months on a product launch set for the beginning of June. I will have more time starting mid-June. This is why I haven't done it already. Sorry about that. But if you want to do it then great!

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc May 26, 2013

Owner

@rpedela no worries at all. If I get time on this vacation I'll knock it out - otherwise it'll be when I get back. Since you can override the type parsing with relative ease it isn't a critical thing but I do appreciate and share your concern over the correctness of the out-of-box behavior.

Also, good luck with your product launch!

Owner

brianc commented May 26, 2013

@rpedela no worries at all. If I get time on this vacation I'll knock it out - otherwise it'll be when I get back. Since you can override the type parsing with relative ease it isn't a critical thing but I do appreciate and share your concern over the correctness of the out-of-box behavior.

Also, good luck with your product launch!

@sevastos

This comment has been minimized.

Show comment
Hide comment
@sevastos

sevastos May 27, 2013

Contributor

Oops, I got interrupted.

So, the text parsers are pretty much done, but the binary parsers have some issues.

  • parseFloat32 suffers from loss of precision (e.x 101.1 => 101.0999984741211) but that's seems to be expected (according to IEEE754)
  • Parsing variabled-width numerics from binary responses looks quite the task to me

Unfortunately, I don't have time nor much knowledge on these to go more in depth, sorry about that.

Contributor

sevastos commented May 27, 2013

Oops, I got interrupted.

So, the text parsers are pretty much done, but the binary parsers have some issues.

  • parseFloat32 suffers from loss of precision (e.x 101.1 => 101.0999984741211) but that's seems to be expected (according to IEEE754)
  • Parsing variabled-width numerics from binary responses looks quite the task to me

Unfortunately, I don't have time nor much knowledge on these to go more in depth, sorry about that.

@@ -82,7 +99,7 @@ var types = [{
// ignore some tests in binary mode
if (helper.config.binary) {
types = types.filter(function(type) {
return !(type.name in {'real':1, 'timetz':1, 'time':1, 'numeric': 1, 'double precision': 1});
return !(type.name in {'real': 1, 'timetz':1, 'time':1, 'numeric': 1});

This comment has been minimized.

@sevastos

sevastos May 27, 2013

Contributor

Not sure about these.

parseFloat32 (real) somtimes has loss of precision. I tried Buffer's native function readFloat1 but the same precision loss happened.

However a code snippet from dzone :P returns correct amounts but it's probably slower and I haven't tested it thoroughly.

After reading some bits about floating points and IEEE754, it seems that sometimes loss of precision is expected.

@sevastos

sevastos May 27, 2013

Contributor

Not sure about these.

parseFloat32 (real) somtimes has loss of precision. I tried Buffer's native function readFloat1 but the same precision loss happened.

However a code snippet from dzone :P returns correct amounts but it's probably slower and I haven't tested it thoroughly.

After reading some bits about floating points and IEEE754, it seems that sometimes loss of precision is expected.

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Jun 7, 2013

Owner

Hey @sevastos sorry for taking a long time to respond - I've been out.

First thing, don't worry about the binary parser. That was 100% community supplied by a couple of people & if they want to change their implementation they're more than welcome, but I don't support it myself. So, don't consider that a hold up by any means.

So, is anything outstanding other than the binary stuff? You can always skip the tests that are failing in binary mode. I've done it before. Whenever you're ready for me to review this without the binary stuff just let me know.

Owner

brianc commented Jun 7, 2013

Hey @sevastos sorry for taking a long time to respond - I've been out.

First thing, don't worry about the binary parser. That was 100% community supplied by a couple of people & if they want to change their implementation they're more than welcome, but I don't support it myself. So, don't consider that a hold up by any means.

So, is anything outstanding other than the binary stuff? You can always skip the tests that are failing in binary mode. I've done it before. Whenever you're ready for me to review this without the binary stuff just let me know.

@sevastos

This comment has been minimized.

Show comment
Hide comment
@sevastos

sevastos Jun 7, 2013

Contributor

No problemo @brianc. Yeap, you can go ahead and review.

Cheers

Contributor

sevastos commented Jun 7, 2013

No problemo @brianc. Yeap, you can go ahead and review.

Cheers

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Jun 7, 2013

Owner

Okay - I reviewed this.

Just so I'm not missing anything, this is what I see as the changed datatypes

summary of changes

1. float4 -> parsed to javascript number (reverts change in v1.x, breaking change)
2. float8 -> parsed to javascript number (reverts change in v1.x, breaking change)
3. int2/4/oid -> parsed to javascript number (no change)
4. int8 -> parsed to javascript string (breaking change)
5. numeric -> parsed to javascript string (breaking change)
6. decimal -> parsed to javascript string (breaking change)

concerns

My only concern is including the ref dependency for the binary parser. I do appreciate the binary parser is correct, but like I said before I didn't write any of it and don't support it myself. Not a huge deal, but if ref not compiling starts giving windows users errors or something, I'll likely rip the binary stuff out completely into another module. For the mean time we can leave it in.

thoughts

I really appreciate everyone's help on this issue and the careful thought put into correct type handling. I apologize for not doing a better job of it myself: my own use of numeric types in applications I have done has never hit any of these pain points. Thankfully the open source & node community is full of friendly helpful folks to get things like this figured out for the greater good! 🎸

@rpedela since you were the originator of the well thought out issue #339 would you mind chiming in on this as well?
@booo any thoughts?

Once I hear from @rpedela I'll go ahead & do a major version bump, documentation update, and do what I can to broadcast this change out on twitter & IRC.

Owner

brianc commented Jun 7, 2013

Okay - I reviewed this.

Just so I'm not missing anything, this is what I see as the changed datatypes

summary of changes

1. float4 -> parsed to javascript number (reverts change in v1.x, breaking change)
2. float8 -> parsed to javascript number (reverts change in v1.x, breaking change)
3. int2/4/oid -> parsed to javascript number (no change)
4. int8 -> parsed to javascript string (breaking change)
5. numeric -> parsed to javascript string (breaking change)
6. decimal -> parsed to javascript string (breaking change)

concerns

My only concern is including the ref dependency for the binary parser. I do appreciate the binary parser is correct, but like I said before I didn't write any of it and don't support it myself. Not a huge deal, but if ref not compiling starts giving windows users errors or something, I'll likely rip the binary stuff out completely into another module. For the mean time we can leave it in.

thoughts

I really appreciate everyone's help on this issue and the careful thought put into correct type handling. I apologize for not doing a better job of it myself: my own use of numeric types in applications I have done has never hit any of these pain points. Thankfully the open source & node community is full of friendly helpful folks to get things like this figured out for the greater good! 🎸

@rpedela since you were the originator of the well thought out issue #339 would you mind chiming in on this as well?
@booo any thoughts?

Once I hear from @rpedela I'll go ahead & do a major version bump, documentation update, and do what I can to broadcast this change out on twitter & IRC.

@rpedela

This comment has been minimized.

Show comment
Hide comment
@rpedela

rpedela Jun 7, 2013

Contributor

Yeah I think it looks great! I don't have any comment on the binary parser dependency.

Sorry I could not get to this sooner. On the bright side, my product was just released and it uses node-postgres.
https://www.datalanche.com/

Contributor

rpedela commented Jun 7, 2013

Yeah I think it looks great! I don't have any comment on the binary parser dependency.

Sorry I could not get to this sooner. On the bright side, my product was just released and it uses node-postgres.
https://www.datalanche.com/

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Jun 7, 2013

Owner

awesome! Congrats! No worries on taking a while. I'll get this merged this weekend & do the whole release song & dance.

Owner

brianc commented Jun 7, 2013

awesome! Congrats! No worries on taking a while. I'll get this merged this weekend & do the whole release song & dance.

@defunctzombie

This comment has been minimized.

Show comment
Hide comment
@defunctzombie

defunctzombie Jun 10, 2013

Contributor

Here is a small gist I created for wrapping numbers to avoid users accidentally "adding" them.

https://gist.github.com/shtylman/5483285

Not sure if this is a great solution but it is something to think about when working with numbers in js. I think we just need to be more vocal about the number problems for folks with more "accounting" sensitive applications.

Contributor

defunctzombie commented Jun 10, 2013

Here is a small gist I created for wrapping numbers to avoid users accidentally "adding" them.

https://gist.github.com/shtylman/5483285

Not sure if this is a great solution but it is something to think about when working with numbers in js. I think we just need to be more vocal about the number problems for folks with more "accounting" sensitive applications.

@brianc

This comment has been minimized.

Show comment
Hide comment
@brianc

brianc Jun 10, 2013

Owner

That's great @shtylman - I'll put big red letters somewhere about node-postgres returning a string if it's a big number and tell them to watch out and remind them to optionally drop in your exception throwing goodness.

Owner

brianc commented Jun 10, 2013

That's great @shtylman - I'll put big red letters somewhere about node-postgres returning a string if it's a big number and tell them to watch out and remind them to optionally drop in your exception throwing goodness.

brianc added a commit that referenced this pull request Jun 19, 2013

Merge pull request #353 from sevastos/bigint-bulletproofing
Handle bigint as string to prevent precision loss

@brianc brianc merged commit 42bae0c into brianc:master Jun 19, 2013

1 check passed

default The Travis CI build passed
Details
@mariusa

This comment has been minimized.

Show comment
Hide comment
@mariusa

mariusa Mar 2, 2017

Would it be possible to please add a preference/setting, so that all bigint are parsed with parseInt(), knowing there will be data loss on really big numbers?

We constantly find bugs because in some place we forget data is transformed to string.

Thanks!

mariusa commented Mar 2, 2017

Would it be possible to please add a preference/setting, so that all bigint are parsed with parseInt(), knowing there will be data loss on really big numbers?

We constantly find bugs because in some place we forget data is transformed to string.

Thanks!

@eric-brechemier

This comment has been minimized.

Show comment
Hide comment
@eric-brechemier

eric-brechemier Mar 2, 2017

Apparently all the numbers in javascript are floating-point numbers1 and the accuracy is only assured for integers ±253 between -9007199254740992 and 9007199254740992.
However bigint ranges ±263: -9223372036854775808 to 9223372036854775807 2.

The most common solution for handling bigint data on javascript is carry them with strings and use special libs for math operations. So strings it is.

Why not keep BIGINT values as numbers up to ±253, and only use strings for values between ±253 and ±263?

This would be less confusing in most cases, and only confusing when truly required.

eric-brechemier commented Mar 2, 2017

Apparently all the numbers in javascript are floating-point numbers1 and the accuracy is only assured for integers ±253 between -9007199254740992 and 9007199254740992.
However bigint ranges ±263: -9223372036854775808 to 9223372036854775807 2.

The most common solution for handling bigint data on javascript is carry them with strings and use special libs for math operations. So strings it is.

Why not keep BIGINT values as numbers up to ±253, and only use strings for values between ±253 and ±263?

This would be less confusing in most cases, and only confusing when truly required.

@rpedela

This comment has been minimized.

Show comment
Hide comment
@rpedela

rpedela Mar 2, 2017

Contributor

@mariusa Personally, I am not opposed to a config option. In the meantime, you can override the default type parser.

var pg = require('pg');

// bigint
pg.types.setTypeParser(20, function (value) {
    return parseInt(value);
});

// numeric
pg.types.setTypeParser(1700, function (value) {
    return parseFloat(value);
});
Contributor

rpedela commented Mar 2, 2017

@mariusa Personally, I am not opposed to a config option. In the meantime, you can override the default type parser.

var pg = require('pg');

// bigint
pg.types.setTypeParser(20, function (value) {
    return parseInt(value);
});

// numeric
pg.types.setTypeParser(1700, function (value) {
    return parseFloat(value);
});
@mariusa

This comment has been minimized.

Show comment
Hide comment
@mariusa

mariusa Mar 2, 2017

Thanks, that works!

We'll trust the magic "20" number :) (saw the internal mapping, hope that won't change)

mariusa commented Mar 2, 2017

Thanks, that works!

We'll trust the magic "20" number :) (saw the internal mapping, hope that won't change)

@rpedela

This comment has been minimized.

Show comment
Hide comment
@rpedela

rpedela Mar 2, 2017

Contributor

That magic number comes from SELECT oid FROM pg_type WHERE typname = 'int8'; and is unlikely to change.

Contributor

rpedela commented Mar 2, 2017

That magic number comes from SELECT oid FROM pg_type WHERE typname = 'int8'; and is unlikely to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment