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

u64 unsupported by sqlx-sqlite #1400

Closed
dob9601 opened this issue Jan 16, 2023 · 9 comments
Closed

u64 unsupported by sqlx-sqlite #1400

dob9601 opened this issue Jan 16, 2023 · 9 comments

Comments

@dob9601
Copy link

dob9601 commented Jan 16, 2023

Description

Using a u64 in the field of a database model yields the error u64 unsupported by sqlx-sqlite when used with the sqlite backend, even though the docs seems to suggest that u64's are supported.

Steps to Reproduce

Use a database model containing a u64 with a sqlite-based backend.

Expected Behavior

Either no error to occur - or more likely, the docs to reflect the fact that u64s aren't supported in sqlite (see: launchbadge/sqlx#2284 (comment))

Actual Behavior

panicked at 'u64 unsupported by sqlx-sqlite'

Reproduces How Often

Reproducible 100% of the time

Versions

├── sea-orm v0.10.6
│   ├── sea-orm-macros v0.10.6 (proc-macro)
│   ├── sea-query v0.27.2
│   │   ├── sea-query-derive v0.2.0 (proc-macro)
│   ├── sea-query-binder v0.2.2
│   │   ├── sea-query v0.27.2 (*)
│   ├── sea-strum v0.23.0
│   │   └── sea-strum_macros v0.23.0 (proc-macro)
│   ├── sea-orm v0.10.6 (*)
├── sea-orm v0.10.6 (*)

Additional Information

@dob9601
Copy link
Author

dob9601 commented Jan 16, 2023

Possible workaround might be to store u64s as a BLOB in sqlite, though as of now this still seems to raise the same "u64 unsupported" error message

@tyt2y3
Copy link
Member

tyt2y3 commented Jan 23, 2023

We might want to backport SeaQL/sea-query#556 to sea-query 0.28

@billy1624
Copy link
Member

billy1624 commented Feb 2, 2023

SQLx's SQLite driver doesn't support u64. We have two options:

  1. Update the docs and specify u64 isn't supported in SQLite

  2. Encode & decode u64 as u32. Encoding side (similar to this but TryInto<u32>): https://github.com/SeaQL/sea-query/blob/7c4e4094252ead1b7db39cec64db7ed82d1e7fab/sea-query-binder/src/sqlx_sqlite.rs#L34-L36.

@billy1624
Copy link
Member

As a temporary "fix" before a consensus has been reached, I'm going to update the docs.

@matdexir
Copy link

matdexir commented Feb 2, 2023

Hi @billy1624, I would like to give the second option a shot. But I am afraid that I may need some hand holding to properly fix it
I have a few questions:

  1. Should changing the section you mentioned be enough?
  2. Do I need to add any testing? If yes, should it be inside sea-query or sea-orm?

@billy1624
Copy link
Member

Hey @matdexir, sorry for the misinformation.

Encode & decode u64 as u32

This isn't ideal. A better option would be "encode & decode u64 as i64". Since the encoding part already handled by SeaQuery. No changes is necessary.

  1. Should changing the section you mentioned be enough?

We only need to change the decoding logics in SeaORM. Which is located at:

QueryResultRow::SqlxSqlite(_) => Err(type_err(format!(
"{} unsupported by sqlx-sqlite",
stringify!($type)
))
.into()),

So, we want to get an Option<i64> from the query result then cast it into u64.

Similar to this code snippet:

QueryResultRow::SqlxSqlite(row) => {
let val: Option<f64> = row
.try_get(idx.as_sqlx_sqlite_index())
.map_err(sqlx_error_to_query_err)?;
match val {
Some(v) => Decimal::try_from(v).map_err(|e| {
DbErr::TryIntoErr {
from: "f64",
into: "Decimal",
source: Box::new(e),
}
.into()
}),
None => Err(err_null_idx_col(idx)),
}
}

  1. Do I need to add any testing? If yes, should it be inside sea-query or sea-orm?

A test case on SeaORM side would be perfect. Where we try saving and select a model with an u64 field.

@matdexir
Copy link

matdexir commented Feb 2, 2023

This isn't ideal. A better option would be "encode & decode u64 as i64". Since the encoding part already handled by SeaQuery. No changes is necessary.

Basically what I was thinking as well, since the former solution would result in memory/value loss. I know that we also lose out almost on about half of the values present in u64 by using i64 but the use cases for such big numbers are quite rare.

Regarding the implementation, I was wondering, should I introduce a new block of with a structure similar for u32/f64
i,e:

#[cfg(feature = "with-bigdecimal")]
impl TryGetable for BigDecimal {
#[allow(unused_variables)]
fn try_get_by<I: ColIdx>(res: &QueryResult, idx: I) -> Result<Self, TryGetError> {
match &res.row {
#[cfg(feature = "sqlx-mysql")]
QueryResultRow::SqlxMySql(row) => row
.try_get::<Option<BigDecimal>, _>(idx.as_sqlx_mysql_index())
.map_err(|e| sqlx_error_to_query_err(e).into())
.and_then(|opt| opt.ok_or_else(|| err_null_idx_col(idx))),
#[cfg(feature = "sqlx-postgres")]
QueryResultRow::SqlxPostgres(row) => row
.try_get::<Option<BigDecimal>, _>(idx.as_sqlx_postgres_index())
.map_err(|e| sqlx_error_to_query_err(e).into())
.and_then(|opt| opt.ok_or_else(|| err_null_idx_col(idx))),
#[cfg(feature = "sqlx-sqlite")]
QueryResultRow::SqlxSqlite(row) => {
let val: Option<f64> = row
.try_get(idx.as_sqlx_sqlite_index())
.map_err(sqlx_error_to_query_err)?;
match val {
Some(v) => BigDecimal::try_from(v).map_err(|e| {
DbErr::TryIntoErr {
from: "f64",
into: "BigDecimal",
source: Box::new(e),
}
.into()
}),
None => Err(err_null_idx_col(idx)),
}
}
#[cfg(feature = "mock")]
#[allow(unused_variables)]
QueryResultRow::Mock(row) => row.try_get(idx).map_err(|e| {
debug_print!("{:#?}", e.to_string());
err_null_idx_col(idx)
}),
#[allow(unreachable_patterns)]
_ => unreachable!(),
}
}
}

But instead more like this:

impl TryGetable for u64

However I think that this may be too overkill.
Or maybe something closer to this?

impl<T: TryGetable> TryGetable for Option<T> {
fn try_get_by<I: ColIdx>(res: &QueryResult, index: I) -> Result<Self, TryGetError> {
match T::try_get_by(res, index) {
Ok(v) => Ok(Some(v)),
Err(TryGetError::Null(_)) => Ok(None),
Err(e) => Err(e),
}
}
}

Which would look like:

impl TryGetable for Option<i64>

What do you think?

Very Thankful for your assistance~~

@Diwakar-Gupta
Copy link
Contributor

Diwakar-Gupta commented Feb 9, 2023

I think the above idea of encoding u64 as i64 will be great, had an idea to do this.

range of u64 is [0 to 2^64 - 1]
range of i64 is [-2^63 to +2^63-1] say [i64.min to i64.max]

when we save value to database
take u64 then convert it to range of i64 => i64 = u64 + i64.min
then save i64

When we load value from database
convert i64 back to u64 => u64 = i64 - i64.min

NOTE:
for doing this conversion we will need an intermediate variable of type i128 to control overflow or underflow.

The down point with this approach is values in database are not exactly what uses wants, it needs to be decoded to be used.

@tyt2y3
Copy link
Member

tyt2y3 commented Feb 21, 2023

u64 unsupported by sqlx-sqlite is a pretty inherit limitation that we are not able to work around.

@tyt2y3 tyt2y3 closed this as not planned Won't fix, can't repro, duplicate, stale Feb 21, 2023
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

5 participants