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

BIGINT PRIMARY KEY AUTOINCREMENT in SQLite #689

Closed
tyt2y3 opened this issue Aug 30, 2023 · 6 comments
Closed

BIGINT PRIMARY KEY AUTOINCREMENT in SQLite #689

tyt2y3 opened this issue Aug 30, 2023 · 6 comments

Comments

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 30, 2023

https://sqlite.org/forum/info/2dfa968a702e1506e885cb06d92157d492108b22bf39459506ab9f7125bca7fd

SQLite would give error: "AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY"

So I guess we somehow have to make BIGINT PRIMARY KEY AUTOINCREMENT a special case and convert it implicitly to INTEGER PRIMARY KEY AUTOINCREMENT

Ref: SeaQL/sea-orm#1832

@Perksey
Copy link

Perksey commented Sep 21, 2023

Yes, just hit this.

thread 'backend::tests::sqlite_playground' panicked at 'called `Result::unwrap()` on an `Err` value: SQLite(SqliteFailure(Error { code: Unknown, extended_code: 1 }, Some("AUTOINCREMENT is only allowed on an INTEGER PRIMARY KEY")))', src/backend/mod.rs:288:55

For generated query

CREATE TABLE IF NOT EXISTS "example_table" ( "id" bigint PRIMARY KEY AUTOINCREMENT, "my_field1" integer NOT NULL, "my_field2" bigint )

I would personally suggest that sea-query maps the backend's type affinity exactly i.e. always mapping the sized integer types to just INTEGER, mapping both float and double to REAL, etc. Really sea-query is an abstraction over multiple SQL languages, and while it's very useful to the user to hint sized types for backends that do support more rigid type affinity, its usefulness wears off when the abstraction tries to preserve this rigidity for a database/query language that fundamentally isn't modelled that way and does not support/do anything with the information it is being given.

I'm happy to do the work in a PR if this is agreeable.

@tyt2y3
Copy link
Member Author

tyt2y3 commented Sep 21, 2023

A PR on the error above would be appreciated.

However I am not sure I understand your preference to type mapping. Or I think we are having the opposite. But only because we wanted to preserve the intended type, such that we when doing schema discovery, we'd get back i32 vs i64. But that's more on the ORM side of things.

Or may be we can add option to the backend to control what type mapping you'd want: original / exact or a better name.

@Perksey
Copy link

Perksey commented Sep 21, 2023

Adding an option may be the best way, e.g.

pub struct SqliteQueryBuilder<TTypeMapper: SqliteTypeMapper = ExactMapper>;

This should ensure compatibility with existing code that does not produce a generic type. Could use a field instead, but this will require downstream modification. Alternative is having an entirely different type.


I guess my main thinking is the end user gains nothing by sea-query being "accurate" to their original type on SQLite, but does on other backends. As such, I thought it'd make sense to only be 100% accurate where doing so yields any benefit.

I didn't consider schema discovery, but yeah that does seem like an implementation detail of the ORM. I'm building a tiny ORM myself based on sea-query, and that just uses whatever type is provided in the user's struct to determine the schema type. If SeaORM doesn't do this, what does it do in the scenario (if supported) where the schema was created by SQLite directly (using unsized SQLite types)?


But yeah, I'm happy to go with either of these approaches. Let me know which you want me to PR 😄

@tyt2y3
Copy link
Member Author

tyt2y3 commented Sep 23, 2023

pub struct SqliteQueryBuilder<TTypeMapper: SqliteTypeMapper = ExactMapper>;

This is a great idea

@tyt2y3
Copy link
Member Author

tyt2y3 commented Nov 21, 2023

Fixed via 37a30f6

@tyt2y3 tyt2y3 closed this as completed Nov 21, 2023
Copy link

🎉 Released In 0.30.3 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

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

2 participants