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

SQlite INTEGER should map to i64 not i32 #1067

Closed
data-retriever opened this issue Sep 24, 2022 · 10 comments · Fixed by SeaQL/sea-query#556
Closed

SQlite INTEGER should map to i64 not i32 #1067

data-retriever opened this issue Sep 24, 2022 · 10 comments · Fixed by SeaQL/sea-query#556

Comments

@data-retriever
Copy link

From SQlite's documentation:

INTEGER. The value is a signed integer, stored in 0, 1, 2, 3, 4, 6, or 8 bytes depending on the magnitude of the value.

But sea-orm-cli generate entity generates a i32

@data-retriever
Copy link
Author

data-retriever commented Sep 24, 2022

Ok, after some random testing I see that if the type is BIGINT but not (BIGINTEGER) on the sql statement then the generated type is i64.

So, SQL -> Rust:

field INTEGER -> field: i32
field BIGINT -> field: i64
field BIGINTEGER -> field: Vec<u8>

This is really surprising. Specially coming from sqlx that would handle INTEGER as i64.

Also, why is BIGINT different from BIGINTEGER?

@billy1624
Copy link
Member

Hey @data-retriever, welcome! Nice icon lolll

Note that in SQLite it only has INTEGER, TEXT, BLOB, REAL and NUMERIC datatype. So, for example BIGINT will be affinized as INTEGER and BIGINTEGER will be affinized as NUMERIC.

For CLI codegen, the mapping from SQLite datatype to Rust type is providing by SeaSchema at here

@anshulxyz
Copy link
Contributor

@billy1624
Copy link
Member

That's correct, SQLite did support up to i64. But I think for most use cases i32 is enough and considering i64 is much more costly than i32. So, I feel i32 is a good default. Demanding user can change the integer field to i64 easily.

Input? @tyt2y3

@data-retriever
Copy link
Author

That's correct, SQLite did support up to i64. But I think for most use cases i32 is enough and considering i64 is much more costly than i32. So, I feel i32 is a good default. Demanding user can change the integer field to i64 easily.

Input? @tyt2y3

Perhaps what's missing is some comment in the documentation. It's not clear at all that using BIGINT is the way to go.

@billy1624
Copy link
Member

Ok, after some random testing I see that if the type is BIGINT but not (BIGINTEGER) on the sql statement then the generated type is i64.

So, SQL -> Rust:

field INTEGER -> field: i32
field BIGINT -> field: i64
field BIGINTEGER -> field: Vec<u8>

This is really surprising. Specially coming from sqlx that would handle INTEGER as i64.

Also, why is BIGINT different from BIGINTEGER?

Short answer, yes, INT / BIGINT is the way to go.

Circle back to the original question. According to https://www.sqlite.org/datatype3.html, INT, BIGINT...etc will be treated as INT but BIGINTEGER isn't mention so that's why SeaSchema parse it as BLOB column type.

@AngelOnFira
Copy link
Contributor

AngelOnFira commented Dec 14, 2022

Another inconsistency that I've run into is when

creating migration -> running migration -> creating entities

I'm using Sqlite for development of a project, and I've written a migration as such:

manager
    .create_table(
        Table::create()
            .table(Channel::Table)
            ...
            .col(ColumnDef::new(Channel::DiscordId).big_integer().not_null())
            ...
            .to_owned(),
    )
    .await?;

In this case, running the migration against a Sqlite DB will change that big_integer call to an INTEGER in Sqlite, as noted above. So when entities are generated with sea-orm-cli, the model becomes:

pub struct Model {
    ...
    pub discord_id: i32,
    ...
}

As far as I know, there's no way for me to annotate somewhere that I want it to come out as an i64 (which makes sense, I don't know of anywhere that would make sense to go). Since I'm generating entities, it's not as easy as being the "demanding user" that will go through and switch things out all the time.

For a sensible default, I feel it makes more sense to be an i64 than an i32. It's ambiguous since data is lost when extracting from Sqlite, so I understand why i32 seems fine, since saving space is good. However, generating entities from an existing Sqlite DB that makes full use of 8 bytes of data in an INTEGER field would mean data is lost as the default behaviour.

I'm going to try and PoC this, and see what behaviour comes up to make sure that assumption is correct.

@AngelOnFira
Copy link
Contributor

AngelOnFira commented Dec 15, 2022

I threw together a repo that can be found here: https://github.com/AngelOnFira/sea-orm-sqlite-integer-issue

Essentially, I generated entities from a Sqlite database with an INTEGER column that held a number larger than an i32, and when I read the number in using SeaORM, it didn't retrieve the correct number.

tl;dr 999_999_999_999_999 came out as -1530494977. I think this potential behaviour should be considered incorrect, and hopefully, this can motivate changing the default 🙂

@data-retriever
Copy link
Author

data-retriever commented Dec 15, 2022

For a sensible default, I feel it makes more sense to be an i64 than an i32. It's ambiguous since data is lost when extracting from Sqlite, so I understand why i32 seems fine, since saving space is good. However, generating entities from an existing Sqlite DB that makes full use of 8 bytes of data in an INTEGER field would mean data is lost as the default behaviour.

I agree 100%. The default behavior is very surprising for someone that was already working with sqlite databases before using seaorm.

But also it seems there is another issue. It seems that on the DSL there is only .big_integer(), no .big_int() or similar. If using SQL the workaround is to use field bigint instead of field integer or field biginteger. So how would someone generate i64 fields with the DSL?

@tyt2y3
Copy link
Member

tyt2y3 commented Dec 18, 2022

Thank you everyone for bringing this up.

I think we should map sea-query's BigInteger to bigint
https://github.com/SeaQL/sea-query/blob/485b0872669720d33f0f347ba7211799321f4c1b/src/backend/sqlite/table.rs#L55

This way, it will be picked up correctly by sea-schema thus completing the loop.

But mapping int to i32 and bigint to i64 is by design, to align the behaviour with MySQL / Postgres, in which int is 32 bit.

tyt2y3 added a commit to SeaQL/sea-query that referenced this issue Dec 18, 2022
@billy1624 billy1624 moved this from Triage to Done in SeaQL Dev Tracker Jan 13, 2023
CyanoJ added a commit to CyanoJ/sea-query-patch that referenced this issue Feb 26, 2023
CyanoJ added a commit to CyanoJ/sea-query-patch that referenced this issue Feb 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants