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

[FEATURE]: bigint mode in SQLite integer column #1980

Open
hyunbinseo opened this issue Mar 8, 2024 · 9 comments · May be fixed by #2579
Open

[FEATURE]: bigint mode in SQLite integer column #1980

hyunbinseo opened this issue Mar 8, 2024 · 9 comments · May be fixed by #2579
Labels
db/sqlite enhancement New feature or request

Comments

@hyunbinseo
Copy link

Describe what you want

  • SQLite's maximum integer value is 9223372036854775807
  • JavaScript's maximum safe integer is 9007199254740991
9223372036854775807 // SQLite's largest possible integer
9007199254740991 // JavaScript Number.MAX_SAFE_INTEGER

Allow SQLite integer columns to be inferred as JavaScript BigInt.

integer('id', { mode: 'number' })
integer('id', { mode: 'bigint' }) // support this

PostgreSQL and MySQL support this feature in the bigint column.

// will be inferred as `number`
bigint: bigint('bigint', { mode: 'number' })

// will be inferred as `bigint`
bigint: bigint('bigint', { mode: 'bigint' })

This will be useful for Autoincrement columns that can become large.

Related issues:

@hyunbinseo hyunbinseo added the enhancement New feature or request label Mar 8, 2024
@ksmithut
Copy link

ksmithut commented Jun 9, 2024

One complication with this will be supporting all the different sqlite drivers. For example, better-sqlite3 has some documentation on how to set up BigInt, but it seems to be sort of an all-or-nothing thing, not column specific. So turning on BigInt for everything would mean that all the other int types would need to convert bigints to normal javascript numbers in order for the types to work.

@MrRahulRamkumar
Copy link
Contributor

Will try and create a PR for this this weekend, but not sure what to do about the complication @ksmithut mentioned.

@MrRahulRamkumar
Copy link
Contributor

Did some digging into this and it looks likes most drivers have a configuration option on how SQLite integers are converted to JavaScript values for the entire database. libsql-client-ts allows you to return integers as numbers, big ints or as strings. (See tursodatabase/libsql-client-ts#32 and tursodatabase/libsql-client-ts#51) Whereas bun:sqlite only supports conversion of all SQLite integers to big ints. (See https://bun.sh/docs/api/sqlite#safeintegers-true).

Since we do not have the ability to configure this per column and not all drivers support conversion to big ints, I suggest we add a new bigint column type which would be used instead of the number column. This column would have the same functionality and modes as number with the only difference being that it is set up to handle the underlying driver returning JavaScript big ints instead of numbers.

So for example, with the libsql driver you would do this:

import { drizzle } from 'drizzle-orm/libsql';
import { createClient } from '@libsql/client';
const client = createClient({
  url: "DATABASE_URL",
  authToken: "DATABASE_AUTH_TOKEN",
  intMode: "bigint"
});
const db = drizzle(client);

// all integer columns would use the bigint column
const table = sqliteTable('table', {
  id: bigint('id')
});

// you can customize integer mode to be number, boolean, timestamp, timestamp_ms
bigint('id', { mode: 'number' })
bigint('id', { mode: 'boolean' })
bigint('id', { mode: 'timestamp_ms' })
bigint('id', { mode: 'timestamp' }) // Date

@dankochetov I can create a PR if you are okay with this aproach

@hyunbinseo
Copy link
Author

@MrRahulRamkumar If the int mode is set to BigInt, are bigint columns forced?

What would happen to the (existing) integer columns? Would they throw errors?

const client = createClient({ intMode: 'bigint' });

const db = drizzle(client);
const db = drizzle(client, { schema }); // Check for the schema here?

If BigInt → Number conversion has minimal performance impacts, this could be possible:

// Always returns a Number
// If the driver returns Number, keep
// In the driver returns BigInt, covnert
integer('id', { mode: 'number' });

// Always returns a BigInt
// If the driver returns Number, convert
// In the driver returns BigInt, keep
integer('id', { mode: 'bigint' });

Values larger than the Number.MAX_SAFE_INTEGER should be handled by the developer.

@MrRahulRamkumar
Copy link
Contributor

MrRahulRamkumar commented Jul 2, 2024

@hyunbinseo if the int mode is set to bigint, all integer columns would be returned as Javascript big ints by the driver. Existing integer columns would possibly break as drizzle expects the driver to return Javascript number and not bigint.

Your suggestion would work but we would need to make sure that all integer modes (number, boolean, timestamp, timestamp_ms) are also updated to handle big ints being returned by the driver.

There might also be some cases where the developer would have to manually handle big ints being returned by drizzle.

I will create a PR that implements what you suggested. Let's see what the maintainers think.

@xray-robot
Copy link

+1

@GustavoOS
Copy link

Any news on the PR?

@kotsoft
Copy link

kotsoft commented Oct 22, 2024

Honestly, I would almost consider this a BUG, not just an enhancement (as this issue has been labeled). Could lead to a security vulnerability or critical system failure later down the line.

@GustavoOS
Copy link

GustavoOS commented Oct 22, 2024

Honestly, I would almost consider this a BUG, not just an enhancement (as this issue has been labeled). Could lead to a security vulnerability or critical system failure later down the line.

This is the main reason I can't use something like Snowflake Id in SQLite with turso. If a table holds the primary key as blob, it would be unqueriable (see #2902 ), but Drizzle won't let us set it as a integer, despite SQLite supporting up to 8 bytes integers

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

Successfully merging a pull request may close this issue.

7 participants