nooberfsh / prusto

A presto/trino client library written in rust.
MIT License
37 stars 23 forks source link

Selecting an optional type results in None? #37

Closed choubacha closed 7 months ago

choubacha commented 7 months ago

I have a field that can be Null and I've wrapped the type in an Option, something like:

#[derive(prusto::Presto)]
struct MyRow {
  val: Option<String>,
}

When I select it, I get back no rows! If I remove this field and try again I get the rows.

choubacha commented 7 months ago

If the value is set, however, the rows are also returned. So it's the Null values which appear to invalidate the whole row!

choubacha commented 7 months ago

I see that this test should mean it's handled: https://github.com/nooberfsh/prusto/blob/master/tests/data_set.rs#L151

Which makes me think that it must be happening after parsing?

choubacha commented 7 months ago

Continuing to debug... I inserted some debug code in the send method where we deserialize from the response into a QueryResult and the code before the deserialization shows the row in the data set. However, after it deserializes the data field is empty.

choubacha commented 7 months ago

Was able to reproduce it in the prusto tests. I made one of the values in the row null in the model data that's loaded in and the entire dataset was returned as empty.

image
choubacha commented 7 months ago

Traced it back to the data_set visitor:

after next value seed
[src/types/data_set.rs:155] &e = Error("invalid type: null, expected varchar", line: 0, column: 0)

I don't see this error inside prusto, so it must come from serde_json. ~However, the test for the option type doesn't hit the visit_map code path and seems to not break.~

edit: It does hit visit_map but doesn't break 🤔

choubacha commented 7 months ago

Debugging continues and it looks like the deserialization from flatten ends up treating the null as a unit type (()) while serde_json treats it only as either some or none. Not sure why it's going another direction, but adding a visit_unit to the option deserializer fixes the issue.

Will put up a PR with this.