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

[bun:sqlite] configurable javascript runtime type for sqlite integers #1536

Closed
dhbaird opened this issue Nov 21, 2022 · 7 comments · Fixed by #11887
Closed

[bun:sqlite] configurable javascript runtime type for sqlite integers #1536

dhbaird opened this issue Nov 21, 2022 · 7 comments · Fixed by #11887
Labels
bun:sqlite Something to do with bun:sqlite enhancement New feature or request

Comments

@dhbaird
Copy link

dhbaird commented Nov 21, 2022

What is the problem this feature would solve?

I'd like to propose an option for the sqlite API to go beyond the current 32-bit limit that is currently imposed on SELECT queries.

Background: Current API truncates integers to 32-bits when selecting (see below). Can be worked around by CASTing to REAL or TEXT (see below). Proposal objective: eliminate the CAST().

import { Database, constants } from "bun:sqlite";
const db = Database.open(":memory:");
db.exec("CREATE TABLE Test ( x INTEGER )")
const insert = db.prepare("INSERT INTO Test (x) VALUES (?)");
insert.run([2147483647]);
insert.run([2147483648]);
insert.run([2147483649]);

console.log(db.prepare("SELECT x FROM Test").all());
// >>>
// Actual:   [ { x: 2147483647 }, { x: -2147483648 }, { x: -2147483647 } ]
// Desired:  [ { x: 2147483647 }, { x: 2147483648 }, { x: 2147483649 } ]

console.log(db.prepare("SELECT CAST(x AS REAL) x FROM Test").all());
// >>>
// Got and expected (all good): [ { x: 2147483647 }, { x: 2147483648 }, { x: 2147483649 } ]

A common use case (for me): storage of epoch time values in milliseconds, which requires ~41-bits. The intended benefit of this proposal is that it would eliminate the need make query or schema alterations, helping to decouple runtime details of this API from sqlite.

What is the feature you are proposing to solve the problem?

An additional option is proposed to control integer conversion, perhaps as follows,

Choice Behavior Range
AS_INT32 Select out as truncated to 32-bits (current behavior) 32-bits (signed)
AS_REAL Select out as a double + or - 53 bits (i.e. Number.MAX_SAFE_INTEGER / Number.MIN_SAFE_INTEGER)
AS_BIGINT Select out as a 64-bit int, and assign to a JavaScript BigInt 64-bits (signed)

Examples of the concept:

const db = Database.open(":memory:", {
    integerMode: constants.AS_INT32 | constants.AS_REAL | constants.AS_BIGINT });
// or, at query level:
const stmt = db.prepare("SELECT x FROM Test", {
    integerMode: constants.AS_INT32 | constants.AS_REAL | constants.AS_BIGINT });

Relevant lines ultimately impacted by this option are found in constructResultObject() and constructResultRow():

What alternatives have you considered?

Workarounds are possible:

  • Use REAL typed columns in the schema. Or,
  • CAST() to REAL. Or,
  • CAST() to TEXT. And convert result to a BigInt in application code. (If up to the 64-bits is actually needed.)
@dhbaird dhbaird added the enhancement New feature or request label Nov 21, 2022
@Jarred-Sumner
Copy link
Collaborator

The bug is here

result->initializeIndex(scope, i, jsNumber(sqlite3_column_int(stmt, i)));

Bun should be getting as int64 instead

@Jarred-Sumner
Copy link
Collaborator

It will now truncate up to 52 bits of precision (max int for a double)

An improvement but not a complete fix ce6fc86

@dhbaird
Copy link
Author

dhbaird commented Nov 22, 2022

That's a great improvement @Jarred-Sumner, thanks!!

@dhbaird
Copy link
Author

dhbaird commented Nov 22, 2022

As for whether to stop at double, or continue to the next level (BigInt, right?), I propose making it be an option like the example in the OP.

Rationale: An option enables the choice based on user's requirements to tradeoff:

  • double: moire efficient but smaller range. Some use cases are fine with the smaller range. Versus,
  • BigInt: more resource intensive but larger range.

Thoughts?

@Jarred-Sumner
Copy link
Collaborator

Yeah I think it should be an option that defaults to double

We could copy better-sqlite3's api for this so that if people have existing apps using it, it's easier to switch

@dhbaird
Copy link
Author

dhbaird commented Nov 22, 2022

Had to lookup better-sqlite3 :) That's great they had already thought of this and looks mostly good. I have one minor criticism that it conflates the use of BigInt with additional range checking and throwing on insert for "safety"; And that does not align with my approach to application development: I would tend to write the code that is already using domains/ranges as appropriate for the application and the application does its own checks when and if needed; And thus the extra checks in better-sqlite3 would be a slight bit of wasted overhead. But this is so minor that I wouldn't block it if other thinking about it still considers it essential for some reason, and that aside it looks good.

@Hebilicious
Copy link

Hebilicious commented Aug 27, 2023

As I was working on tursodatabase/libsql-client-ts#71, I wasn't able to return a bigint from the bun:sqlite driver. It would be great to have something similar to bettersqlite-3 https://github.com/WiseLibs/better-sqlite3/blob/master/docs/integer.md

Here's an API suggestion :

const intType: "auto" | "bigint" | "number" | "string"  = "auto" 
db.setIntegerResultType(intType) //global
statement.setIntegerResultType(intType) //local

Auto would automatically choose between number and bigint return types, whereas the other types would try to enforce bigint, number or string.

For the query type we could use something similar similar db.setIntegerQueryType with a default to auto. Both could also be combined with something more flexible like statement.setIntegerType({ query: "REAL", return: "bigint"})

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bun:sqlite Something to do with bun:sqlite enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants