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

Support summation into 128-bit integer #1692

Closed
coder543 opened this Issue May 12, 2018 · 4 comments

Comments

Projects
None yet
2 participants
@coder543

coder543 commented May 12, 2018

If I have a table that has a 64-bit value column in it, it would be nice to be able to sum() it into a 128-bit integer, rather than having to pull in some BigDecimal crate, now that 128-bit integers are in stable Rust. For 32-bit integers, they currently sum() into a 64-bit integer, so I think this is a nice symmetry to add. Personally, I would like to have the option to sum() into a same-sized type like the Rust stdlib does, but it is also nice to ensure there will not be an overflow by summing into a larger type.

Right now, the following code:

use db::schema::some_table::dsl::*;
use diesel::dsl::sum;
some_table
    .filter(user.eq(self.id))
    .select(sum(value))
    .first::<i128>(conn)
    .unwrap()

yields this error:

error[E0277]: the trait bound `i128: diesel::Queryable<diesel::sql_types::Nullable<diesel::sql_types::Numeric>, _>` is not satisfied
   --> src/db.rs
    |
    |             .first::<i128>(conn)
    |              ^^^^^ the trait `diesel::Queryable<diesel::sql_types::Nullable<diesel::sql_types::Numeric>, _>` is not implemented for `i128`
    |
    = note: required because of the requirements on the impl of `diesel::query_dsl::LoadQuery<_, i128>` for <snip>
@sgrif

This comment has been minimized.

Member

sgrif commented May 13, 2018

Thanks, but there's no SQL type which maps to a 128 bit integer. Numeric is neither an integer type, nor is it limited to 128 bits.

@sgrif sgrif closed this May 13, 2018

@coder543

This comment has been minimized.

coder543 commented May 13, 2018

So there's no way we can make it type-safe to sum a column that we know is made of 64-bit integers into a 128-bit integer value? At compile time we have all the information needed, don't we?

@coder543

This comment has been minimized.

coder543 commented May 13, 2018

The Numeric that we get back from the database really must map precisely to an integer if the only values summed were integers. The only values being summed are integers. Assuming that it will fit into 128-bits is the same trade-off as assuming a summation of 32-bit integers will fit into a 64-bit integer, which is already being done.

I would rather have the option to sum a 64-bit column into a 64-bit integer with possible overflow than to deal with pulling in a BigDecimal crate, but I think summing into a 128-bit integer fits better with the model that's already in use than summing into a 64-bit integer, and will practically ensure no overflow happens. It's much more likely that someone will have at least 2^32 rows of 32-bit integers and overflow a 64-bit integer than it is for them to have at least 2^64 rows of 64-bit integers and overflow a 128-bit integer. But ok, if there's no way that we can make this work... then I guess there's nothing to be done, although I don't fully agree.

@sgrif

This comment has been minimized.

Member

sgrif commented May 13, 2018

Possibly, but that would require:

  • Moving away from the actual semantics of the database
  • Introducing a new SQL type, which does not actually exist
  • Changing the behavior of sum (breaking change)
  • Dropping support for older versions of Rust

It's definitely possible for you to do this in a third party crate though, by introducing a new SQL type, and defining a new sum function with sql_function!.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment