Skip to content
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

Reexamine BIGINT returning strings #2398

Open
MadaraUchiha opened this issue Nov 2, 2020 · 8 comments
Open

Reexamine BIGINT returning strings #2398

MadaraUchiha opened this issue Nov 2, 2020 · 8 comments

Comments

@MadaraUchiha
Copy link

Related: #353

Back in 2013 this was a good idea, as BIGINTs would lose precision when converted to JavaScript numbers (which are doubles).

Today we have native BigInt support in JavaScript, can we reconsider returning/accepting BigInts when doing database operations with the BIGINT type?

Pros and cons as far as I can tell:

Pros:

  • More accurate data representation
  • Easier client-side processing (can use real math operations on BigInts)

Cons:

  • Breaking change
  • BigInt is not serializable to JSON, extra work for database clients is required for that.

Thoughts?

@vitaly-t
Copy link
Contributor

vitaly-t commented Nov 2, 2020

Related, from 5 days ago - #2395

This however idea is the opposite. We do not need native BigInt for data coming from the server, as in most cases the precision isn't needed. But when it is needed, you can easily make the driver do the conversion for you, so we do not need this kind of change.

It is the use of BigInt when preparing parameters for the server, that would be useful, imo.

@charmander
Copy link
Collaborator

charmander commented Nov 2, 2020

See brianc/node-pg-types#78 for existing discussion about this.

We do not need native BigInt for data coming from the server, as in most cases the precision isn't needed.

That’s not very reassuring, but the difference between the existing string behaviour and BigInt isn’t precision.

Anyway, the rest of my comment is also in that issue: brianc/node-pg-types#78 (comment).

@MadaraUchiha
Copy link
Author

MadaraUchiha commented Nov 3, 2020

@vitaly-t Today you are returning strings, so you still don't lose precision, given that I don't understand the "precision is not needed" argument. (Also, if I picked BigInt as my datatype, it potentially means I do want the extra precision.)

@charmander From what I gather from the linked issue, there's a general consensus that it's a positive change (also I'm not sure why browsers are discussed there), is there anything else blocking it?

@vitaly-t
Copy link
Contributor

vitaly-t commented Nov 3, 2020

@vitaly-t Today you are returning strings, so you still don't lose precision, given that I don't understand the "precision is not needed" argument.

Most developers opt to parse 64-bit numbers as int, that's what it means:

type.setTypeParser(20, a => parseInt(a));

@MadaraUchiha
Copy link
Author

That's fair, if you want to do any sort of math on it whatsoever, it has to be a number of some sort.

Also, I suspect that not many developers today are even aware of the existence of BigInt in the language, it's kind of an obscure feature.

@charmander
Copy link
Collaborator

@charmander From what I gather from the linked issue, there's a general consensus that it's a positive change (also I'm not sure why browsers are discussed there), is there anything else blocking it?

@MadaraUchiha A plan to reduce the number of surprising bugs people run into while upgrading to pg 9.x, or agreement that no such plan is necessary. The default change can probably go into pg-types right now, since that package is already accumulating breaking changes for its next major version.

@vitaly-t
Copy link
Contributor

This should be closed, because even today, BigInt remains to be dramatically slower than number. In my tests under NodeJS v16, I'm seeing it being about 40 times slower than number. So, it would introduce a serious performance penalty, and therefore should never become the default.

@charmander
Copy link
Collaborator

@vitaly-t The current default is strings, not numbers, and correct/surprise-free behaviour is more important than small and avoidable performance differences anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants