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

Support unsigned versions of integers. #409

Closed
dtzxporter opened this issue Mar 3, 2022 · 6 comments
Closed

Support unsigned versions of integers. #409

dtzxporter opened this issue Mar 3, 2022 · 6 comments
Assignees
Milestone

Comments

@dtzxporter
Copy link
Contributor

Unsigned versions can be automatically mapped to the signed versions that database uses.

@havaker
Copy link
Contributor

havaker commented Mar 3, 2022

I don't think that the driver should do any implicit lossy casts (e.g. u64::MAX cannot be automatically mapped to the i64).

@psarna
Copy link
Contributor

psarna commented Mar 4, 2022

Lossy - definitely not. A runtime bound check should be performed

@gsson
Copy link

gsson commented Jun 24, 2022

Since it just bit me, for varint columns I think serialising a u64 should Just Work.

Also, while I am a big fan of strongly typed APIs, this snippet makes me think that strong typing isn't really possible in the current design

use scylla::SessionBuilder;

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let session = SessionBuilder::new()
        .known_nodes(&[...])
        .user("... "...")
        .build()
        .await?;
    let _ = session.query("DROP KEYSPACE IF EXISTS test", ())
        .await?;
    let _ = session.query("CREATE KEYSPACE IF NOT EXISTS test WITH REPLICATION = {'class': 'SimpleStrategy', 'replication_factor': 1}", ())
        .await?;
    session.use_keyspace("test", true)
        .await?;
    let _ = session.query("
CREATE TABLE IF NOT EXISTS test_types (
  text         TEXT,
  smallint     SMALLINT,
  varint       VARINT,
  PRIMARY KEY ((text, smallint, varint))
);", ())
        .await?;

    let prepared = session.prepare("INSERT INTO test_types (text, smallint, varint) VALUES (?, ?, ?)")
        .await?;

    let text = String::from("AB");
    let smallint = 16963i16;
    let bigint = num_bigint::BigInt::from(17220u64);

    let _ = session.execute(&prepared, (text.clone(), text.clone(), text.clone()))
        .await?;
    let _ = session.execute(&prepared, (smallint, smallint, smallint))
        .await?;
    let _ = session.execute(&prepared, (bigint.clone(), bigint.clone(), bigint.clone()))
        .await?;

    let query = session.query("SELECT * FROM test_types", ()).await?;
    for row in query.rows.unwrap() {
        eprintln!("{:?}", row.columns);
    }

    Ok(())
}

On my machine it outputs

[Some(Text("BC")), Some(SmallInt(16963)), Some(Varint(BigInt { sign: Plus, data: BigUint { data: [16963] } }))]
[Some(Text("AB")), Some(SmallInt(16706)), Some(Varint(BigInt { sign: Plus, data: BigUint { data: [16706] } }))]
[Some(Text("CD")), Some(SmallInt(17220)), Some(Varint(BigInt { sign: Plus, data: BigUint { data: [17220] } }))]

which, since the Value trait has no information on what the destination type is, is quite reasonable.

To me it makes the unsigned limitation seem rather arbitrary though :)

@wprzytula
Copy link
Collaborator

wprzytula commented Jun 20, 2024

Why would we want to support unsigned integers directly in the driver?

  • first thought: We don't want to. ScyllaDB uses signed integers only. Then perhaps we should always represent data using signed integers, even if they don't suit the use case best.
  • second thought: But forcing users to operate on signed integers (e.g. in structs representing UDTs or rows), requiring bound checks on their side before passing unsigned data to the driver - is inconvenient at best.

So, I think we should implement {De,S}erializeValue for unsigned integers.

Specifics

u64 should be usable interchangeably with i64, u32 with i32 and so on. Runtime bound check should be performed and {De,S}erializationError returned in case of exceeding the boundaries.

@wprzytula wprzytula added this to the 1.0.0 milestone Jun 20, 2024
@Lorak-mmk
Copy link
Collaborator

I think that this idea may backfire on us later. Lack of unsigned types in Scylla / C* is of course an issue for some application developers because it increases complexity (as the developer now needs to validate data and deal with errors).
This complexity is however inherent to developing with Scylla / C* (as long as they don't have unsigned types) and trying to hide this complexity may cause a perfectly working application to suddenly start throwing errors after long time.

New deserialization framework will probably allow to downcast error to specific implementation and check what exactly went wrong - but doing this will be much more difficult that creating 2 structures (first with signed types, to interact with DB, second with unsigned types, to use in rest of the application) and handling conversion errors.

for those reasons I think it's better to make users aware of this complexity and deal with it explicitly - it will result in more robust, less error-prone applications.

@Lorak-mmk Lorak-mmk closed this as not planned Won't fix, can't repro, duplicate, stale Jun 20, 2024
@JonahPlusPlus
Copy link

Instead of implementing SerializeCql and FromCqlVal for u* types, this crate could provide wrapper types for handling different serialization patterns.

There are 4 different approaches:

  • as: Just convert the unsigned to signed and back. (Could be a wrapper like SignedU*)
  • MappedU*: e.g. MappedU32(u32) would map the range of an u32 to the range of an i32. (So, u32::MIN..u32::MAX -> i32::MIN..i32::MAX)
  • BlobU*: e.g. BlobU32(u32) would convert the u32 to [u8; 4] and back. (Also supports u128)
  • BigInt: Store the signed number as a BigInt. (Could be a wrapper like VarU*)

I'm using MappedU* and BlobU* in my project.

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

8 participants