WiseLibs / better-sqlite3

The fastest and simplest library for SQLite3 in Node.js.
MIT License
5.37k stars 394 forks source link

BigInt rounding error in edge case #1046

Closed fabiospampinato closed 1 year ago

fabiospampinato commented 1 year ago

Repro:

import sqlite3 from 'better-sqlite3';

const db = sqlite3(':memory:');
const result = db.prepare('SELECT ? AS value').get(1152735103331642317n);

console.log ( result );

db.close ();

Output:

{ value: 1152735103331642400 }

As you can see we got a number, when we should have gotten a bigint since 1152735103331642317n is outside of the safe integer range.

Prinzhorn commented 1 year ago

BigInt needs to be explicitly enabled, documented here https://github.com/WiseLibs/better-sqlite3/blob/master/docs/integer.md#getting-bigints-from-the-database

import sqlite3 from 'better-sqlite3';

const db = sqlite3(':memory:');
db.defaultSafeIntegers();
const result = db.prepare('SELECT ? AS value').get(1152735103331642317n);

console.log ( result );

db.close ();
{ value: 1152735103331642317n }
fabiospampinato commented 1 year ago

Interesting 🤔

I thought one would get normal numbers back if within the safe range, and bigints otherwise.

But this gives me a bigint:

db.defaultSafeIntegers ();
const result = db.prepare ( 'SELECT ? AS value' ).get ( 123n );

And this gives me a number:

db.defaultSafeIntegers ();
const result = db.prepare ( 'SELECT ? AS value' ).get ( 123 );

Which all things considered seems to be better than how I thought things should work. Thanks.

fabiospampinato commented 1 year ago

Wait, it doesn't exactly work like that though.

Now pragmas give me small bigints, which is kinda weird:

db.prepare ( `PRAGMA page_size` ).get ();

And if I insert a small number somewhere I get a bigint back.

Reopening as I find this behavior quite confusing, and basically undocumented. IMO maybe there should be an option for "everything is a number just like by default, but if you enable this option and you explicitly give me a bigint value then I'll give you that value, and only that value, as a bigint back".

Prinzhorn commented 1 year ago

but if you enable this option and you explicitly give me a bigint value then I'll give you that value, and only that value, as a bigint back

That doesn't work because SQLite supports 64 integers and JavaScript Number doesn't. The information if you originally inserted a Number or BigInt is lost and irrelevant to SQLite (it stores it in whatever form it finds best suitable, since SQLite does not actually have any sort of strict types at all until very recently https://www.sqlite.org/datatype3.html and https://sqlite.org/stricttables.html).

In the docs I've linked you can see that you can also enable that behavior per statement, so only when you need it (stmt.safeIntegers()). So your PRAGMA should not be affected. There is also https://github.com/WiseLibs/better-sqlite3/issues/1031 if you need more control.

fabiospampinato commented 1 year ago

I see, that makes a lot of sense 🤔

Nothing more to do here I guess.

I suppose a flag that returns integers if they are within the safe range, and bigints otherwise, could be implemented. I'm not particularly interested in that though, so closing this issue.