TryGhost / node-sqlite3

SQLite3 bindings for Node.js
BSD 3-Clause "New" or "Revised" License
6.2k stars 813 forks source link

Precision lost with BIGINT #922

Open sushantdhiman opened 6 years ago

sushantdhiman commented 6 years ago

Inserting a record with BIGINT and selecting it will return data with lost precision

INSERT INTO `Users` (`id`,`username`,`age`,`level`,`bigLevel`,`isUser`,`isAdmin`) VALUES (NULL,'Adam',22,-1,'90071992547409911',0,1);

SELECT `id`, `username`, `age`, `level`, `bigLevel`, `isUser`, `isAdmin` FROM `Users` AS `User` WHERE `User`.`id` = 1;
[ { id: 1,
    username: 'Adam',
    age: 22,
    level: -1,
    bigLevel: 90071992547409900, // Precision lost, it should be 90071992547409911
    isUser: 0,
    isAdmin: 1 } ]
wmertens commented 6 years ago

JavaScript doesn't support 64 bit integers, only 52 bit precision IEEE-something floats. To store integers like that, you should store them as strings, and make the field type BLOB so sqlite doesn't auto-convert them.

Nothing to do with node-sqlite3 - I propose you close this issue.

sushantdhiman commented 6 years ago

JavaScript doesn't support 64 bit integers, only 52 bit precision IEEE-something floats

@wmertens I am aware of that fact, sqlite3 should either return BIGINT as string or provide an option to do so.

This is a standard among various other database connectors. Currently BIGINT stored in table will be incorrectly returned (as "JavaScript doesn't support 64 bit integers, only 52 bit precision IEEE-something floats.")

This should be tagged both as bug and feature request. I am going to need this in Sequelize, will try to go through code and try to send a PR :) (Any pointers will be helpful and save time)

wmertens commented 6 years ago

It is due to sqlite. You need to set the field type to BLOB. Otherwise sqlite will convert.

On Thu, Dec 28, 2017, 3:46 PM Sushant, notifications@github.com wrote:

JavaScript doesn't support 64 bit integers, only 52 bit precision IEEE-something floats

@wmertens https://github.com/wmertens I am aware of that fact, sqlite3 should either return BIGINT as string or provide an option to do so.

This is a standard among various other database connectors. Currently BIGINT stored in table will be incorrectly returned (as "JavaScript doesn't support 64 bit integers, only 52 bit precision IEEE-something floats.")

This should be tagged both as bug and feature request. I am going to need this in Sequelize, will try to go through code and try to send a PR :) (Any pointers will be helpful and save time)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mapbox/node-sqlite3/issues/922#issuecomment-354299303, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlmGuLVzr5VCW8oJ7HB4biEofE_Srks5tE6nJgaJpZM4RERuc .

wmertens commented 6 years ago

@sushantdhiman did that fix it for you? You can also set the field type to TEXT.

sushantdhiman commented 6 years ago

Still returned data is incorrect. SQLite returns big integers as exponential representation

sqlite> select 111111111111111111111111111111111111111 as abc;
1.11111111111111e+38

This module should return correct integer format converted from exponential notation (as string). Or atleast return exponential notation to us so we can convert, rather than truncating data

wmertens commented 6 years ago

This processing is done by sqlite, there's nothing this module can do about it

$ sqlite3 SQLite version 3.21.0 2017-10-24 18:55:49 Enter ".help" for usage hints. Connected to a transient in-memory database. Use ".open FILENAME" to reopen on a persistent database. sqlite> select 111111111111111111111111111111111111111111111111 as abc; 1.11111111111111e+47 sqlite>

sushantdhiman commented 6 years ago

This module can at least return 1.11111111111111e+38?

wmertens commented 6 years ago

JS only supports floats, not 64 bit ints. The module https://github.com/JoshuaWise/better-sqlite3 supposedly has some support for 64bit ints but not sure what that entails.

sushantdhiman commented 6 years ago

It not a matter of JS supporting big number, we have already discussed that :)

node-sqlite3 is receiving these numbers from C binding, but its not converting those numbers to proper string when it should. Its just treating incoming packets as integers, which results in truncation.

node-sqlite3 should provide a way to treat incoming big numbers as string.

wmertens commented 6 years ago

Hmmm - so as soon as the C API returns a number value that cannot be represented as a float without precision loss it should convert it into a string? That seems like it might backfire some day - better store those values as string in the first place

I tried some more and I think the problem might be happening on insert with parameter conversion, because the cli handles mixed strings/nums fine:

sqlite> select "11111111111111111111111111111111111111111"; 11111111111111111111111111111111111111111 sqlite> create table foo(t); sqlite> insert into foo values("1111111111111111111111111111111111111111"); sqlite> select from foo; 1111111111111111111111111111111111111111 sqlite> insert into foo values("111111111111111111111111111111111111111111111111111"); sqlite> select from foo; 1111111111111111111111111111111111111111 111111111111111111111111111111111111111111111111111 sqlite> insert into foo values(111111111111111111111111111111111111111111111111111); sqlite> select * from foo; 1111111111111111111111111111111111111111 111111111111111111111111111111111111111111111111111 1.11111111111111e+50

Perhaps this module or the api is too eager in guessing field types? Can you check with the cli that the db does have the data as int?

On Mon, Jan 22, 2018 at 10:46 AM Sushant notifications@github.com wrote:

It not a matter of JS supporting big number, we have already discussed that :)

node-sqlite3 is receiving these numbers from C binding, but its not converting those numbers to proper string when it should. Its just treating incoming packets as integers, which results in truncation.

node-sqlite3 should provide a way to treat incoming big numbers as string.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mapbox/node-sqlite3/issues/922#issuecomment-359372544, or mute the thread https://github.com/notifications/unsubscribe-auth/AADWlqgvyxUQF_OV1V4_WOxBoIhWhnV5ks5tNFkJgaJpZM4RERuc .

wmertens commented 6 years ago

Investigating some more with the module:

16176 $ node
> t=require('sqlite3')
{ Database: [Function: Database],
  Statement: [Function: Statement],
  OPEN_READONLY: 1,
  OPEN_READWRITE: 2,
  OPEN_CREATE: 4,
  VERSION: '3.15.0',
  SOURCE_ID: '2016-10-14 10:20:30 707875582fcba352b4906a595ad89198d84711d8',
  VERSION_NUMBER: 3015000,
  OK: 0,
  ERROR: 1,
  INTERNAL: 2,
  PERM: 3,
  ABORT: 4,
  BUSY: 5,
  LOCKED: 6,
  NOMEM: 7,
  READONLY: 8,
  INTERRUPT: 9,
  IOERR: 10,
  CORRUPT: 11,
  NOTFOUND: 12,
  FULL: 13,
  CANTOPEN: 14,
  PROTOCOL: 15,
  EMPTY: 16,
  SCHEMA: 17,
  TOOBIG: 18,
  CONSTRAINT: 19,
  MISMATCH: 20,
  MISUSE: 21,
  NOLFS: 22,
  AUTH: 23,
  FORMAT: 24,
  RANGE: 25,
  NOTADB: 26,
  cached: { Database: [Function: Database], objects: {} },
  verbose: [Function] }
> d=new t.Database(':memory:')
Database { open: false, filename: ':memory:', mode: 65542 }
> d.exec('create table t(a TEXT, b NUMBER, c); insert into t values (111111111111111111111111111111111111111, 111111111111111111111111111111111111111, 111111111111111111111111111111111111111)')
Database { open: true, filename: ':memory:', mode: 65542 }
> d.all('select * from t', console.log)
Database { open: true, filename: ':memory:', mode: 65542 }
> null [ { a: '1.11111111111111e+38',
    b: 1.111111111111111e+38,
    c: 1.111111111111111e+38 } ]
> d.run('insert into t values(?,?,?)', "111111111111111111111111111111111111111", 111111111111111111111111111111111111111, 111111111111111111111111111111111111111)
Database { open: true, filename: ':memory:', mode: 65542 }
> d.run('insert into t values(?,?,?)', 111111111111111111111111111111111111111, "111111111111111111111111111111111111111", "111111111111111111111111111111111111111")
Database { open: true, filename: ':memory:', mode: 65542 }
> d.all('select * from t', console.log)
Database { open: true, filename: ':memory:', mode: 65542 }
> null [ { a: '1.11111111111111e+38',
    b: 1.111111111111111e+38,
    c: 1.111111111111111e+38 },
  { a: '111111111111111111111111111111111111111',
    b: 1.111111111111111e+38,
    c: 1.111111111111111e+38 },
  { a: '1.11111111111111e+38',
    b: 1.111111111111111e+38,
    c: '111111111111111111111111111111111111111' } ]

Actually that seems like how it should behave :-/

wmertens commented 6 years ago

@sushantdhiman do you see a problem with the current behavior as above?

I think that having the module returning numbers as strings when they get too long is wrong.

vss-devel commented 5 years ago

Since Node v10 supports BigInts (http://thecodebarbarian.com/an-overview-of-bigint-in-node-js.html) is there any way to get 64-bit integers supported?

wmertens commented 5 years ago

I wonder how to do it - BigInt and Number are completely distinct, so we can't get number columns as BigInt without some sort of configuration. fs.stat has the same issue and it uses a options argument.

So it seems to me that the only way to allow this is an extra argument on all query functions, so {bigint: true} returns all numbers as BigInt, and maybe {bigint: ['id']} only uses BigInt for the id column?

vss-devel commented 5 years ago

BigInts are quite cumbersome to handle. In this regard perhaps fine tuning a request with {bigint: ['id','id2'...]} would be a useful option.

sushantdhiman commented 5 years ago

Why don't just go ahead with an option like bigNumberStrings to return string for big numbers, every other client library in Node.js does that

vss-devel commented 5 years ago

So it seems to me that the only way to allow this is an extra argument on all query functions, so {bigint: true} returns all numbers as BigInt

Another alternative to this is to set such an option globally akin to sqlite3.verbose(). Then all integers are true integers and it's up to the user how to deal with it.

alexandrebodin commented 5 years ago

Hi there, I'm really interested in this too :D

Any updates on extending the type parser or any way to be of help ? @wmertens

wmertens commented 5 years ago

@alexandrebodin well mapbox is very busy with their day job, so the only way this will happen is if someone makes a PR (and even then it might take a while before it gets merged).

BTW, if you really want 64 bit support and you can live with a faster lib that is blocking, check out https://github.com/JoshuaWise/better-sqlite3/blob/master/docs/integer.md

It has an extra option: instead of using BigInt, they use an Integer class to encapsulate 64 bit values. That's probably a better idea anyway, since BigInt can be bigger than 64 bit.

alexandrebodin commented 5 years ago

@alexandrebodin well mapbox is very busy with their day job, so the only way this will happen is if someone makes a PR (and even then it might take a while before it gets merged).

BTW, if you really want 64 bit support and you can live with a faster lib that is blocking, check out https://github.com/JoshuaWise/better-sqlite3/blob/master/docs/integer.md

It has an extra option: instead of using BigInt, they use an Integer class to encapsulate 64 bit values. That's probably a better idea anyway, since BigInt can be bigger than 64 bit.

Thank you for those informations I'll have a look :)

I would really be satisfied with just an automatic casting of sqlite bigint to strings like postgres and mysql drivers do instead of BigInts which aren't supported in many ways.

Have a great day !

sacOO7 commented 3 years ago

@wmertens, this library is supposed to return string for bigint values, otherwise, throw an error or shouldn't read them in the first place. This is the default behaviour in all other clients.

jondubois commented 3 years ago

@wmertens My main questions/concerns about the approach of using blobs are:

For our use case, it would be ideal to store it as BigInt inside SQLite (64 bits is enough) and retrieve as strings since that's what other database drivers do by default.

wmertens commented 3 years ago

@jondubois I don't know much about the inner workings of this module, but I do know it doesn't have any sort of string-to-64bit mapping. You can only do blobs.

For your use case you might want to consider better-sqlite, provided you can live with its synchronous API.

sacOO7 commented 3 years ago

I have forked the code and changed functionality a bit ( to allow processing of BigInt values, and it's better than getting wrong values from database). So, currently we are returning strings for all integer values and then manually converting them back to int as required. It's super convenient. I will probably create working PR on this repo. after doing few more tweaks. Meanwhile feel free to use the library https://github.com/sacOO7/node-sqlite3

sacOO7 commented 3 years ago

To install use npm i https://github.com/sacOO7/node-sqlite3 or yarn add https://github.com/sacOO7/node-sqlite3

endian86 commented 2 years ago

I have forked the code and changed functionality a bit ( to allow processing of BigInt values, and it's better than getting wrong values from database). So, currently we are returning strings for all integer values and then manually converting them back to int as required. It's super convenient. I will probably create working PR on this repo. after doing few more tweaks. Meanwhile feel free to use the library https://github.com/sacOO7/node-sqlite3

hi: I seed your code. I think whether it can be considered, if it is an integer out of bounds, then convert it to a string and return it.

like this:

case SQLITE_INTEGER: { if(((Values::Integer)field)->value & 0xffe0000000000000) { std::string s = std::to_string(((Values::Integer)field)->value); value = Napi::String::New(env, s.c_str(), s.size()); } else { value = Napi::Number::New(env, ((Values::Integer*)field)->value); } }

FreeTrade commented 2 years ago

While no solution is ideal here given that JS doesn't handle 64bit numbers easily, silently truncating the numbers is probably the least desirable way to handle this as it has the possibility of unexpected data loss. Instructively, this is not the solution used by other libraries facing similar issues.

I'd suggest the suggested solutions are desirable as the default in the following order.

Convert to String > Use BigInt > Throw Error > Silently Truncate Number

As to the suggestion to store the large numbers as strings, this has performance impacts on indexes, and additional storage is required.

The suggestion to use better-sqlite3 is a good one - this supports the large numbers, however it offers no way to interrupt a query which is a deal breaker for me. (Asynchronous use is possible with worker threads, albeit complicated) https://github.com/WiseLibs/better-sqlite3/issues/568

The suggestion to use @sacOO7 fork might work, but i'm getting a program crash with it when using threads, and I guess it is not being updated and maintained in the same way as this library, so definitely not ideal.

Edit: This fork which seems to be more up to date - https://github.com/renatoalencar/node-sqlite3/tree/int64-support

Finally, for anyone else facing a problem with this, a workaround is to explicitly cast the numbers to strings in the SQL query - eg

SELECT cast(bignumberfield as varchar) as bignumberfieldstring from mytable;

This works, but at the cost of rewriting all your SQL queries. This is particularly awkward if you have any 'SELECT * from mytable' as all of the fields now need to be individually specified or renamed.

FreeTrade commented 2 years ago

This seems to work better - https://github.com/juanrgm/node-sqlite3/tree/int64-support