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

Struct containing UDTs doesn't deserialize #742

Closed
iguberman opened this issue Jun 8, 2023 · 5 comments
Closed

Struct containing UDTs doesn't deserialize #742

iguberman opened this issue Jun 8, 2023 · 5 comments

Comments

@iguberman
Copy link

iguberman commented Jun 8, 2023

Values get properly inserted. Then selected from Scylla no problem:
SELECT RESULT:
QueryResult { rows: Some([Row { columns: [
Some(Uuid(86bfcb0c-3fea-424a-a44d-fb1ba73db6bb)),
Some(Uuid(28e499f7-04f4-484e-ae43-84f83d3e616f)),
Some(List([
UserDefinedType { keyspace: "my_keyspace", type_name: "building_block", fields:
[("id", Some(Uuid(d8eec8f6-674e-49d6-b061-67e8e7599c93))), ("owner_id", Some(Uuid(660abd60-8e39-4897-b502-cb79708d6227))), ("locked", Some(Boolean(false))), ("asset_id", Some(Int(12345)))] },
UserDefinedType { keyspace: "my_keyspace", type_name: "building_block", fields: [("id", Some(Uuid(4bc63c4c-3fc6-418c-9ee8-6fe1dc07fdeb))), ("owner_id", Some(Uuid(970602aa-dd19-42b7-aac7-4b0e6726fd0b))), ("locked", Some(Boolean(true))), ("asset_id", Some(Int(56789)))
] }])),
Some(UserDefinedType { keyspace: "my_keyspace", type_name: "coordinates", fields: [("x_axis", Some(Float(11.1))), ("y_axis", Some(Float(-22.2))), ("z_axis", Some(Float(33.3)))] })] }]),
warnings: [],
tracing_id: None,
paging_state: None,
col_specs: [ColumnSpec { table_spec: TableSpec { ks_name: "my_keyspace", table_name: "prefabs" }, name: "owner_id", typ: Uuid }, ColumnSpec { table_spec: TableSpec { ks_name: "my_keyspace", table_name: "prefabs" }, name: "id", typ: Uuid }, ColumnSpec { table_spec: TableSpec { ks_name: "my_keyspace", table_name: "prefabs" }, name: "blocks", typ: List(UserDefinedType { type_name: "building_block", keyspace: "my_keyspace", field_types: [("id", Uuid), ("owner_id", Uuid), ("locked", Boolean), ("asset_id", Int)] }) }, ColumnSpec { table_spec: TableSpec { ks_name: "my_keyspace", table_name: "prefabs" }, name: "location", type: UserDefinedType { type_name: "coordinates", keyspace: "my_keyspace", field_types: [("x_axis", Float), ("y_axis", Float), ("z_axis", Float)] } }], serialized_size: 194 }

THE PROBLEM !!!

Then this error deserializing with first_row_typed (or any other similar methods I tried). Makes no sense, column 2 which is fabrications clearly has 2 values in the query result.
first_row_typed RESULT:
Err(FromRowError(BadCqlVal { err: ValIsNull, column: 2 }))
thread 'components::prefabrication::tests::prefab_persistence_works' panicked at 'called Result::unwrap() on an Err value: FromRowError(BadCqlVal { err: ValIsNull, column: 2 })',

(This approach works from cqlsh no problem)
I use scylla driver version 0.8.1 and scylla from docker image scylladb/scylla:5.2.0

Table creation:

CREATE TYPE my_keyspace.coordinates (x_axis Float, y_axis Float, z_axis Float);
CREATE TYPE my_keyspace.building_block (id UUID, owner_id UUID, locked Boolean, asset_id Int);

CREATE TABLE IF NOT EXISTS my_keyspace.prefabs (
fab_id UUID,
owner_id UUID,
fabrications list<frozen<my_keyspace.building_block>>,
location my_keyspace.coordinates,
PRIMARY KEY(fab_id, owner_id)
);

My structs:

# THIS is the typed struct that corresponds to the table my_keyspace.prefabs

#[derive(Debug, Clone, PartialEq, FromRow, ValueList)]
pub struct Prefabrication {
    pub fab_id: Uuid,
    pub owner_id: Uuid,
    pub fabrications: Vec<BuildingBlock>,
    pub location: Coordinates,
}


# This is the first UDI in the table

#[derive(Debug, Clone, PartialEq, IntoUserType, FromUserType)]
pub struct BuildingBlock {
    pub block_id: Uuid,
    pub owner_id: Uuid,
    pub locked: bool,
    pub asset_id: i32,
}

# This is the second UDI in the table

#[derive(Debug, Clone, PartialEq, IntoUserType, FromUserType)]
pub struct Coordinates {
    x_axis: f32,
    y_axis: f32,
    z_axis: f32,
}

Please note that I also tried without List/Vec of BuildingBlock and replaced with just a single one to rule out this causing a problem but had exact same error.

@piodul
Copy link
Collaborator

piodul commented Jun 9, 2023

Then this error deserializing with first_row_typed (or any other similar methods I tried). Makes no sense, column 2 which is fabrications clearly has 2 values in the query result.
first_row_typed RESULT:
Err(FromRowError(BadCqlVal { err: ValIsNull, column: 2 }))

Could you show us the query string? When deserializing results to a type that implements FromRow, the order of columns defined in the SELECT query must be the same as the order of struct fields. The column: 2 refers to the second (counting from zero) column as defined in SELECT. Maybe you have a mismatch here and a different column than fabrications is actually null.

In general, if you have a column that can be null, you should wrap the corresponding rust struct field in an Option.

@iguberman iguberman changed the title Struct containing UDIs doesn't deserialize Struct containing UDTs doesn't deserialize Jun 9, 2023
@iguberman
Copy link
Author

Then this error deserializing with first_row_typed (or any other similar methods I tried). Makes no sense, column 2 which is fabrications clearly has 2 values in the query result.
first_row_typed RESULT:
Err(FromRowError(BadCqlVal { err: ValIsNull, column: 2 }))

Could you show us the query string? When deserializing results to a type that implements FromRow, the order of columns defined in the SELECT query must be the same as the order of struct fields. The column: 2 refers to the second (counting from zero) column as defined in SELECT. Maybe you have a mismatch here and a different column than fabrications is actually null.
QUERY STRING: SELECT * FROM my_keyspace.prefabs where fab_id = ? and owner_id = ?;

Please note that I included the entire QUERY RESULT first thing in the description of the issue. It looks perfect with column # 2 being a List of 2 UDTs (so everything got selected as expected on the Scylla end). No values in there are null and they are in the same order as the struct indeed.

In general, if you have a column that can be null, you should wrap the corresponding rust struct field in an Option.
Thanks! Good point. I will keep that in mind. No nullable columns for my current use case though.

Just in case, here is troubleshooting code that splits query results and deserialization:
``let qres = session.query(scylla_db::SELECT_PREFAB_QUERY, (id, owner_id))
.await?;
println!("SELECT RESULT: {:?}", qres);
let res = qres
.first_row_typed::();

println!("first_row_typed RESULT: {:?}", res);
PLEASE NOTE:  There are no examples complex as mine.   But from documentation annotating UDTs and Structs it should just work or at least give a more reasonable error.  (Based on what you told me seems like it actually IS a bug) 

@piodul
Copy link
Collaborator

piodul commented Jun 9, 2023

I think I know where the problem lies. I see that in case of the BuildingBlock Rust struct its field names do not correspond to the UDT field names. For Rust you have block_id, owner_id, locked, asset_id and in case of the UDT you have id, owner_id, locked, asset_id. Both names and the order must match for a UDT to be deserialized properly, and in your case the first two don't match.

The reason why you got a ValIsNull error is because the UDT deserialization logic tries to match the Rust and UDT names one to one and, if there is a mismatch, the Rust field is skipped and treated as if it were null:

quote_spanned! {field.span() =>
    #field_name: <#field_type as FromCqlVal<::std::option::Option<CqlValue>>>::from_cql(
        {
            let received_field_name: Option<&::std::string::String> = fields_iter
                .peek()
                .map(|(ref name, _)| name);

            // Order of received fields is the same as the order of processed struct's
            // fields. There cannot be an extra received field present, so it is safe to
            // assign subsequent received fields, to processed struct's fields (inserting
            // None if there is no received field corresponding to processed struct's
            // field)
            if let Some(received_field_name) = received_field_name {
                if received_field_name == stringify!(#field_name) {
                    let (_, value) = fields_iter.next().unwrap();
                    value
                } else {
                    None
                }
            } else {
                None
            }
        }
    ) ?,
}

It looks like the code assumes that this is an extra field and decides to skip it, treating it as if it were null.

In #665 we are going to introduce new macros with more features and better error reporting, and FromRow + FromUserType will be deprecated. We've always had an implicit assumption that the field order must match, including field names - we should document it more clearly for the old macros. I'm not sure if it is worth changing the behavior of the old ones at this point.

@iguberman
Copy link
Author

Whew! I literally made sure table matches my main struct and thought I had UDT fields matching as well, except that id vs. block_id in the building block UDT. Definitely would be nice to have a friendlier error! But for now this fixed my issue!! Woo-hoo! Thanks so much for getting back to me so quickly and resolving my issue Piotr! @piodul

@iguberman
Copy link
Author

The problem was really misleading error messages when there was a column name that didn't match the UDT struct field name. The functionality works as expected!

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

2 participants