scylladb / scylla-rust-driver

Async CQL driver for Rust, optimized for ScyllaDB!
Apache License 2.0
584 stars 107 forks source link

Implement FromCqlVal for frame::value::MaybeUnset #429

Open DayOfThePenguin opened 2 years ago

DayOfThePenguin commented 2 years ago

Migrating this from the comments on #421 because it's a separate discrete feature.

I tried implementing this myself, but the problem I'm running into is that since associative type binds aren't supported yet, I can't do it for any generic V that implements Value (i.e. impl FromCqlVal for crate::frame::value::MaybeUnset<V: Value>. The workaround for this seems to be to implement it for MaybeUnset<CqlValue> and accept that you have to match for each variant of CqlValue. Something like:

impl FromCqlVal<CqlValue> for crate::frame::value::MaybeUnset<CqlValue> {
    fn from_cql(cql_val: CqlValue) -> Result<Self, FromCqlValError> {
        match cql_val {
            CqlValue::Ascii(_) => todo!(),
            CqlValue::Boolean(_) => todo!(),
            CqlValue::Blob(_) => todo!(),
            CqlValue::Counter(_) => todo!(),
            CqlValue::Decimal(_) => todo!(),
            CqlValue::Date(_) => todo!(),
            CqlValue::Double(_) => todo!(),
            CqlValue::Duration(_) => todo!(),
            CqlValue::Empty => todo!(),
            CqlValue::Float(_) => todo!(),
            CqlValue::Int(_) => todo!(),
            CqlValue::BigInt(_) => todo!(),
            CqlValue::Text(_) => todo!(),
            CqlValue::Timestamp(_) => todo!(),
            CqlValue::Inet(_) => todo!(),
            CqlValue::List(_) => todo!(),
            CqlValue::Map(_) => todo!(),
            CqlValue::Set(_) => todo!(),
            CqlValue::UserDefinedType { keyspace, type_name, fields } => todo!(),
            CqlValue::SmallInt(_) => todo!(),
            CqlValue::TinyInt(_) => todo!(),
            CqlValue::Time(_) => todo!(),
            CqlValue::Timeuuid(_) => todo!(),
            CqlValue::Tuple(_) => todo!(),
            CqlValue::Uuid(_) => todo!(),
            CqlValue::Varint(_) => todo!(),
        }
    }
}

It's not clear to me how to implement the base case for MaybeUnset::Unset though. I have a feeling that changes to the generic implementation of from_cql<Option<CqlValue>> (lines 61-71 in cql_to_rust.rs) might be required too?

Originally posted by @DayOfThePenguin in https://github.com/scylladb/scylla-rust-driver/issues/421#issuecomment-1088490364

psarna commented 2 years ago

If we decide to implement FromCqlVal for MaybeUnset, we also need to decide what's the semantics of its serialization and deserialization. Unset is a specific thing, because you'll never really get it from Scylla - you can only get either a value or null. So in terms of putting MaybeUnset in a struct for serializing/deserializing, we should define what it means - e.g. if we get a null, we simply deserialize it to Unset, and when we serialize it back in order to send it, we'll send an Unset. With such semantics it's not really possible to send null via this struct (as in, delete a value with a tombstone), so it's not really that versatile, but perhaps useful enough if somebody's use case never deletes single column values, but really wants to use Unset to in prepared statements to avoid overriding previous values with tombstones.

So, for starters, perhaps it's enough to simply implement FromCqlVal<CqlValue> for MaybeUnset<CqlValue> and accept that Option<MaybeUnset<T>> will simply continue deserializing to null, making such a construct rather useless.

Alternatively, we can try to figure out another interface for sending unsets - e.g. not implement FromCqlVal for MaybeUnset (which sounds quite hacky), and instead allowing to declare (e.g. as a derive parameter) that Option<CqlValue> is to be serialized as Unset if it's null. @piodul?

piodul commented 2 years ago

@psarna I don't like the idea of making MaybeUnset deserializable. I don't think it was ever supposed to be deserialized, and I doubt that deserialization could be supported in an ergonomic a composable way.

Using attributes to control the behavior of a derive sounds better to me. I was thinking about something like this:

#[derive(ValueList)]
struct SensorReading {
    sensor_id: String,

    #[scylla(serialize_as_unset)]
    data: Option<i8>,
}
psarna commented 2 years ago

Yup, tagging specific values as serializable to Unset sounds much better, I don't really like the idea of making MaybeUnset pseudo-deserializable.

psarna commented 2 years ago

although we probably need to name it something like serialize_null_as_unset, to emphasize that regular values are still serialized as values.