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

FromRow depends on the struct having the same layout as table. #370

Closed
ChillFish8 opened this issue Dec 30, 2021 · 7 comments
Closed

FromRow depends on the struct having the same layout as table. #370

ChillFish8 opened this issue Dec 30, 2021 · 7 comments
Assignees
Labels
area/deserialization area/proc-macros Related to procedural macros enhancement New feature or request
Milestone

Comments

@ChillFish8
Copy link

FromRow when derived depends on the struct fields matching the same layout as the table itself, which can be a little interesting to deal with when you would expect that it bases it off of the column name as the majority of other databases drivers in Rust.

Is there any particular reason why it depends on having the struct mirror the layout of the table vs getting each field by column name?

@psarna psarna added the enhancement New feature or request label Jan 3, 2022
@psarna
Copy link
Contributor

psarna commented Jan 3, 2022

Hello and Happy New Year! (pardon us for the delay, everybody was celebrating)

The only reasons are performance and ease of development - if we assume the layout to have the same order as the table definition, we save time and effort on matching column names to struct names and validating if everything matches.

I agree though that it would be nice to optionally allow (perhaps with a separate macro) a matching based on names instead, since that's what users might find more friendly to use.

@ChillFish8
Copy link
Author

Ah that makes sense.

Although i wonder how much of an effect the additional overhead would have on performance. Especially on smaller structs.

@Jasperav
Copy link
Contributor

@ChillFish8 Note that you can generate every table definition into rust structs by using the orm: https://github.com/Jasperav/scylla_orm let me know if you have any problems or missing features!

@piodul
Copy link
Collaborator

piodul commented Mar 24, 2023

We'll be moving away from FromRow and the new DeserializeRow will allow matching column names to rust field names. This is what the impl generated by #[derive(DeserializeRow)] will do by default. It will also be able to generate an impl that assumes that the order of columns is the same as rust fields but will verify that the corresponding column names and field names are equal - so a little bit more efficient but still safe.

This should be covered after this PR is merged and finished: #665

@piodul piodul added this to the 0.9.0 milestone Mar 24, 2023
@Jasperav
Copy link
Contributor

@piodul will the verification of the fields be optional? What I mean with that, is that I want to make sure Catalytic remains up to date by using this new derive macro, but since the code is generated, it does not need these checks. So it would be nice if there is a method called something like ‘deserialize_unsafe’ or whatever which skips these columns checks, which I can call with Catalytic.

@piodul
Copy link
Collaborator

piodul commented Mar 24, 2023

@piodul will the verification of the fields be optional? What I mean with that, is that I want to make sure Catalytic remains up to date by using this new derive macro, but since the code is generated, it does not need these checks. So it would be nice if there is a method called something like ‘deserialize_unsafe’ or whatever which skips these columns checks, which I can call with Catalytic.

In the current state of the PR - no, but I will add an attribute for the derive macro so that the name check can be disabled.

@piodul piodul added the area/proc-macros Related to procedural macros label Oct 26, 2023
@Lorak-mmk Lorak-mmk self-assigned this Nov 15, 2023
@avelanarius avelanarius modified the milestones: 1.0.0, 0.14.0 Apr 30, 2024
@wprzytula wprzytula assigned wprzytula and unassigned Lorak-mmk May 14, 2024
@wprzytula
Copy link
Collaborator

The PR with new deserialization macros has been merged (#1024), but the new deserialization API they generate implementation for is not yet available in the driver. For tracking it, see #462.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deserialization area/proc-macros Related to procedural macros enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants