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

Allow serialization of "columns" that are not valid rust identifiers #1137

Merged
merged 3 commits into from
Dec 4, 2024

Conversation

nrxus
Copy link
Contributor

@nrxus nrxus commented Dec 3, 2024

Fixes

When trying to serialize a row that isn't a valid rust identifier, the macro fails to expand due to the invalid rust identifier being used when creating variables for the derived implementation. This PR fixes that such that those variables are driven by the field identifier rather than the column name.

In particular, this occurs when trying to serialize a dynamic TTL that would be using in a query such as:

INSERT INTO my_table (/* snip cols */) VALUES((/* snip cols */) USING TTL ?

The struct desired for serialization is:

struct InsertRow {
    #[scylla(rename = "[ttl]"]
    ttl: i32,
    /* snip cols */
}

However, in the current version of the driver this fails because it tries to create a variable called visited_flag_[ttl] which is not a valid rust identifier. As far as I could tell there is no reason for this variable to be based on the column name rather than the field name so I switched it to be based off the field name since that already has to be a valid identifier.

I added a test to verify that the serialization works but I didn't add any integration tests to make sure that the TTL insert actually works. I have tested it locally however.

For context: I am using this dynamic TTL to allow moving of rows to different "buckets" (which drive partitions in my schema). I want the re-bucketing to not affect the existing TTL so I am reading the existing TTL in one query and then applying it to the new row in the new bucket in an insert.

I also fixed a doc typo that was missing an ending square bracket.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

Copy link

github-actions bot commented Dec 3, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 8b579e6

Lorak-mmk
Lorak-mmk previously approved these changes Dec 3, 2024
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@piodul I don't see anything wrong with this change, but you wrote those macros so it would be good if you took a look - maybe I missed something.
@wprzytula Do we need the same change for deserialization?

@Lorak-mmk Lorak-mmk requested review from piodul and wprzytula December 3, 2024 10:22
Copy link
Collaborator

@piodul piodul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I believe we have exactly the same issue in SerializeValue as well. @nrxus could you fix it in this PR, too? The fix should be very similar to what you did in the first commit.

@Lorak-mmk
Copy link
Collaborator

Looks good, but I believe we have exactly the same issue in SerializeValue as well. @nrxus could you fix it in this PR, too? The fix should be very similar to what you did in the first commit.

Is it possible to have UDT field names that are not correct Rust identifiers?

@piodul
Copy link
Collaborator

piodul commented Dec 3, 2024

Looks good, but I believe we have exactly the same issue in SerializeValue as well. @nrxus could you fix it in this PR, too? The fix should be very similar to what you did in the first commit.

Is it possible to have UDT field names that are not correct Rust identifiers?

Apparently, yes:

cqlsh> create type ks.x ("a$a" int);
cqlsh> describe ks.x

CREATE TYPE ks.x (
    "a$a" int
);
cqlsh> 

nrxus added 3 commits December 3, 2024 09:11
fixes cases where the "column name" is not a valid rust identifier;
such as trying to pass in a dynamic TTL
this adds support for UDTs that have fields that are not valid rust
idents but are valid scylla field names
@wprzytula
Copy link
Collaborator

@wprzytula Do we need the same change for deserialization?

This is how Deserialize{Row,Value} builds a visited flag variable's ident:

    fn visited_flag_variable(field: &Field) -> syn::Ident {
        quote::format_ident!("visited_{}", field.ident.as_ref().unwrap().unraw())
    }

So it should be free of the mentioned limitation.

Copy link
Collaborator

@wprzytula wprzytula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 Thank you for this contribution, @nrxus!

@Lorak-mmk
Copy link
Collaborator

Thanks for the contribution! We plan to release 0.15.1 this or next week, it will contain your patch.

@Lorak-mmk Lorak-mmk merged commit 4b6ad84 into scylladb:main Dec 4, 2024
11 checks passed
wprzytula pushed a commit to wprzytula/scylla-rust-driver that referenced this pull request Dec 11, 2024
Allow serialization of "columns" that are not valid rust identifiers

(cherry picked from commit 4b6ad84)
@wprzytula wprzytula mentioned this pull request Dec 11, 2024
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

Successfully merging this pull request may close these issues.

4 participants