Skip to content

Add flatten attribute to derive SerializeRow #1144

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

Merged
merged 13 commits into from
Apr 9, 2025

Conversation

nrxus
Copy link
Contributor

@nrxus nrxus commented Dec 7, 2024

This is similar to the flatten attribute in serde.

This PR adds support for both the match_by_name and the enforce_ordering flavors but it does not allow these structs to be mix-and-matched. This means that structs of different flavors of serialization cannot be flattened into one another. This is a feasibility limitation as these two methods of serialization are completely at odds with each other and hence cannot be combined. The error produced if these two flavors are matched will be at compile time but it may not be the clearest error since it would be about the struct not implementing some doc hidden trait they wouldn't be able to see in the docs.

I have only added this attribute to SerializeRow because it was easier than DeserializeRow but I also want to add it to that macro in a future PR next chance I get to dig into this code.

All the new traits/structs/enums needed for this change are inside the _macro_internal subdmodule such that no new public API is exposed. Maybe in the future those could be made public but it felt too early to know if all the signatures were exactly how we wanted to expose them or not.

For context, I am currently dealing with an issue that if I have different insert queries where one sets N columns and another one sets the same N and one extra, then I have two make two structs with N repeated fields. With this PR I'd be able to to instead flatten the struct with N fields inside the other struct to make my code more maintainable.

By name serialization

ser::row::ByName

A new internal-only struct ser::row::ByName is added that wraps a struct that implements a new trait: SerializeRowByName. This new type has a single function ser::row::ByName::serialize and attempts to serialize an entire RowSerializationContext, returning an error if any of the columns in the context were not serialized or do not belong to the struct. This is basically the implementation of SerializeRow::serialize for any struct that implements SerializeRowByName but split into its own internal-type so that the macro doesn't have to create this shared code. This couldn't be added as a default implementation in one of our traits because we need to call for some functions using Self as a generic parameter which caused some compilation errors.

SerializeRowByName

When deriving SerializeRow using the match_by_name flavor the struct will also implement a new internal-only trait: SerializeRowByName. This trait has a single type associated type Partial, and a function partial() that creates it. The partial struct has 3 main parts:

  1. For every field that is a column (will not be flattened): A reference to the field.
  2. For every field that is a nested struct (will be flattened) The partial view of that nested struct. This means that the nested struct must also implement: SerializeRowByName such that partial() can be called on it.
  3. A hashset to check that every field (not column) has been serialized. For fields that are columns we do this via their column-name. For nested structs, we do this via the field name of the nested struct.

The partial struct is required to implement a new trait PartialSerializeRowByName

PartialSerializeRowByName

PartialSerializeRowByName has two required functions:

  • serialize_field: takes the spec of a single column and attempts to serialize the corresponding field to it. If this column does not belong to this partial struct then the caller is told that the column is unused so that the caller can instead try to use a different field for this same column (i.e., when testing to see if any nested structs can serialize to that column). If the column is used, then a check is done to see if that column has completed the serialization of this field so that it can remove it out of its missing set. The caller is informed if that column has finished the serialization of this partial struct or not.

  • check_missing: consumes the partial struct while checking if all the fields in this struct were serialized, returning an error if not. This is used inside ser::row::ByName::serialize to verify that the a struct has been fully serialized. If a field has not finished serializing and the field is a nested struct (i.e., not just a column) then we should get the error from the nested struct instead for better error messaging.

To do this signaling, a new internal-only enum ser::row::FieldStatus was added that returns whether a column was used for the field, was used and completed the field, or was used by the field is still missing more columns.

By order serialization

ser::row::InOrder

A new internal-only struct ser::row::InOrder is added that wraps a struct that implements a new trait: SerializeRowInOrder. This new type has a single function ser::row::InOrder::serialize that attempts to serialize an entire RowSerializationContext, returning an error if any of the columns in the context were not serialized or do not belong to the struct. It does this by:

  1. Wrapping an iterator over the columns in the context around a new struct ser::row::ByColumn.
  2. Calling for the generated SerializeRowInOrder implementation for the struct we are deriving SerializeRow for using the ser::row::ByColumn instance.
  3. Verifying that the the ser::row::ByColumn instance was fully consumed.

This is basically the implementation of SerializeRow::serialize for any struct that implements SerializeRowInOrder but split into its own internal-type so that the macro doesn't have to create this shared code. This couldn't be added as a default implementation in one of our traits because we need to call for some functions using Self as a generic parameter which caused some compilation errors.

ser::row::ByColumn

ser::row::ByColumn wraps an iterator over column specs and provides the following methods:

  • next: Given a value to serialize it type and name checks it against the next column spec in the iterator, serializing it if successful or returning an error therwise

  • next_skip_name: Given a value to serialize it type checks (but skips name check) it against the next column spec in the iterator, serializing it if successful or returning an error therwise

  • finish: verifies that the iterator is fully consumed.

SerializeRowInOrder

When deriving SerializeRow using the enforced_ordering flavor the struct will also implement a new internal-only trait: SerializeRowInOrder. This trait has a single method serialize_in_order() whose generated implementation will:

  1. For every field that is a column (will not be flattened): It will call for for next or next_skip_name on the given ser::row::ByColum instance.
  2. For every field that is a nested struct (will be flattened), then it will call for serialize_in_order on it (implying the nested struct must also implement SerializeRowInOrder and pass along its ser::row::ByColum instance.

Note that this method does not call for finish() on ser::row::ByColumn because it does not need to verify that the iterator was fully consumed as it could have been called during flattening and we only want to verify that the iterator is consumed on the root struct being serialized.

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.

Copy link

github-actions bot commented Dec 7, 2024

cargo semver-checks found no API-breaking changes in this PR.
Checked commit: c5470a0

@nrxus nrxus force-pushed the flatten-serialize-name branch 8 times, most recently from 0de5bb0 to dc2a19e Compare December 9, 2024 04:25
@nrxus
Copy link
Contributor Author

nrxus commented Dec 9, 2024

I apologize about the multiple force pushes yesterday, I was chewing on it yesterday a bit more since it wasn't reviewed yet and decided to move some stuff around to make it more clear that all the new structs are internal to the macro implementation only (by moving it to that sub-module). All that should be done by now.

I have also started work on adding the flatten attribute for the enforce_order flavor as well but in a separate branch. Let me know if you have a preference on whether to put that as its own PR or push it here so that the entire support for #[flatten] for SerializeRow is all on in one PR.

@nrxus
Copy link
Contributor Author

nrxus commented Dec 10, 2024

I have finished the work to also support flattening when serializing with flavor = "enforced_order". It's on a separate branch though since I wasn't sure if this PR was already considered too big or not. Let me know if you'd like me to merge it into here so you can review it all as one piece or if I should just wait until this is reviewed and merged as the changes are already large.

@nrxus
Copy link
Contributor Author

nrxus commented Dec 12, 2024

@wprzytula would you mind taking a look at this PR and tell me if you would like me to keep it as-is or add the flatten support to the enforced_order flavor as well in this PR?

@nrxus nrxus force-pushed the flatten-serialize-name branch from c03bf5b to 4184a7b Compare December 13, 2024 17:18
@nrxus
Copy link
Contributor Author

nrxus commented Dec 17, 2024

@Lorak-mmk , could you take a look and let me know if there are any concerns holding this PR?

@wprzytula
Copy link
Collaborator

@nrxus We're sorry for poor responsivity on our side. We're busy with next year planning; we'll be able to look at your PR later.

@Lorak-mmk
Copy link
Collaborator

This is a significant but breaking change, so we most likely won't be able to attend to it before releasing 1.0. We are quite busy with other work :(

@wprzytula
Copy link
Collaborator

This is a significant but breaking change, so we most likely won't be able to attend to it before releasing 1.0. We are quite busy with other work :(

Are you sure? semver-checks disagrees with you:

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

@nrxus
Copy link
Contributor Author

nrxus commented Dec 18, 2024

I made especially sure not to change any existing API, and to hide all of the new types/traits in the existing internal module to not increase the public API surface area other the new attribute.

@wprzytula
Copy link
Collaborator

I made especially sure not to change any existing API, and to hide all of the new types/traits in the existing internal module to not increase the public API surface area other the new attribute.

In such case, we will technically be able to release it in, say, 1.1, when we find time to review and accept this after we release 1.0. Does it sound OK to you, @nrxus ?

@nrxus
Copy link
Contributor Author

nrxus commented Dec 18, 2024

I made especially sure not to change any existing API, and to hide all of the new types/traits in the existing internal module to not increase the public API surface area other the new attribute.

In such case, we will technically be able to release it in, say, 1.1, when we find time to review and accept this after we release 1.0. Does it sound OK to you, @nrxus ?

Yep sounds good! I'll just keep pointing to my branch for now. I also have a branch to do this same support but when serializing with order enforced. Should I just merge it here so you all only have to review it as one complete feature? It'd make the overall size of the PR bigger which is why I had kept it separate

@wprzytula
Copy link
Collaborator

I made especially sure not to change any existing API, and to hide all of the new types/traits in the existing internal module to not increase the public API surface area other the new attribute.

In such case, we will technically be able to release it in, say, 1.1, when we find time to review and accept this after we release 1.0. Does it sound OK to you, @nrxus ?

Yep sounds good! I'll just keep pointing to my branch for now. I also have a branch to do this same support but when serializing with order enforced. Should I just merge it here so you all only have to review it as one complete feature? It'd make the overall size of the PR bigger which is why I had kept it separate

IMO let's have it in a single PR, separate commits.

@Lorak-mmk
Copy link
Collaborator

This is a significant but breaking change, so we most likely won't be able to attend to it before releasing 1.0. We are quite busy with other work :(

Are you sure? semver-checks disagrees with you:

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

I made a typo. I definitely meant that this is NOT a breaking change, and thus we will not prioritise the review before releasing 1.0.

@nrxus nrxus force-pushed the flatten-serialize-name branch 5 times, most recently from 236ab1a to 031f226 Compare December 24, 2024 19:53
@nrxus
Copy link
Contributor Author

nrxus commented Apr 8, 2025

I took a stab at writing those messages, but I see you were faster. My commits are available at: https://github.com/Lorak-mmk/scylla-rust-driver/tree/flatten-serialize-name I'm not sure which messages are better, mine or yours. I'll let you and others (cc @wprzytula @muzarski ) decide.

I am perfectly happy with your version so I just did that but I removed the do_not_recommend diagnostic attributes because those were only introduced in 1.85 and I think you all have a 6-month MSRV cycle so that'd be too recent.

Also, you were right, once I switched to fully qualifying the path then there is only one error for the first case as expected:

error[E0277]: `serialize::row::tests::foobar::Inner` cannot be flattened here
    --> scylla-cql/src/serialize/row_tests.rs:1220:14
     |
1220 |     #[derive(SerializeRow)]
     |              ^^^^^^^^^^^^ `serialize::row::tests::foobar::Inner` is not a struct that derives `SerializeRow` with `match_by_name` flavor
     |
     = help: the trait `SerializeRowByName` is not implemented for `serialize::row::tests::foobar::Inner`
     = note: There are two common reasons for that:
             - `serialize::row::tests::foobar::Inner` does not use `#[derive(SerializeRow)]`
             - `serialize::row::tests::foobar::Inner` uses `#[scylla(flavor = "enforce_order")]`
     = help: the following other types implement trait `SerializeRowByName`:
               RowWithTTL
               TestRowWithColumnRename
               TestRowWithColumnSorting
               TestRowWithGenerics<'a, T>
               TestRowWithNoColumns
               TestRowWithSkippedFields
               serialize::row::tests::foobar::Outer
               serialize::row::tests::test_row_serialization_nested_structs::InnerColumnsOne
             and 2 others
     = note: this error originates in the derive macro `SerializeRow` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `scylla-cql` (lib test) due to 4 previous errors

@nrxus nrxus force-pushed the flatten-serialize-name branch from 1af115e to ea09107 Compare April 8, 2025 21:27
wprzytula
wprzytula previously approved these changes Apr 9, 2025
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.

Last 2 minor changes.

Comment on lines 290 to 302
// if not, then check if any flattened field has a column for it
_ => 'flatten_try: {
#({
match self.#flattened_fields.serialize_field(spec, writer)? {
// there is a column and the field is done
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ugh, I reall thought I would give approve this time, but there is one more unqualified call :(

Comment on lines 351 to 363
// if what is missing is a flattened field then report that error
#(if !self.#flattened_visited_flag_names {
return self.#flattened_fields.check_missing()
})*

Copy link
Collaborator

Choose a reason for hiding this comment

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

And another one here

muzarski and others added 5 commits April 9, 2025 11:43
This test checks that `DeserializeValue` macro behaves correctly in case
of https://github.com/apache/cassandra/blob/4a80daf32eb4226d9870b914779a1fc007479da6/doc/native_protocol_v4.spec#L1003.

1. Serialize the UDT with the missing field value
2. Deserialize the serialized UDT, but include the missing field in metadata
3. Expect that the missing field value is treated as null

I don't know of any way to simulate this using the Scylla/C* cluster.
CQL protocol mentions that such scenario is possible, but does not specify
when it can appear.
the function itself is technically public but in the doc hidden
internal macro module so that it can be used for macros to simplify
code expansion but doesn't affect the surface area of the public API
otherwise it can't compile if the impl block already had a 'b named lifetime
`NoColumnWithName` and `ValueMissingForColumn` were
flipped.

`NoColumnWithName` is returned when there is a serializable field in
the Rust struct (i.e., is it not ignored) but there is no matching
column for it in the spec

`ValueMissingForColumn` should be returned when the spec expects a
column but there is no matching field in the Rust struct.
@Lorak-mmk
Copy link
Collaborator

Lorak-mmk commented Apr 9, 2025

I see at least one other unqualified call. Give me a moment, I'll fix them and push to my branch.

nrxus and others added 8 commits April 9, 2025 12:13
This is a no-op refactor to the code generation by SerializeRow. New
types and traits are introduced in the doc hidden module
`_macro_internal` such that the API surface area is unchanged. The
goal of this refactor is to make the addition of the flattened
serialization feature easier
any helper types/methods have been created inside the internal macro
module so as not to increase the public API surface area
This commit focuses on the `match_by_name` flavor.
If a struct's field is a reference, allow that reference to use the
`flatten` attribute so that the field can be serialized using
SerializeRow and the columns produced flattened into the parent
struct.

Prior to this commit this was not possible so this commit does two
changes:

1. Adds a blanket implementation on  the needed
traits (SerializeRowByName and SerializeRowInOrder) for references of
structs that already implement them.

2. Adds a bound to the lifetimes of the fields when making the partial
structs during serialization such that the reference to the fields
have to outlive the partial struct being created during
serialization.
It was released over 6 months ago so it is ok to use it.
It allows us to use LazyCell.
This uses `diagnostic::on_unimplemented` to hint the users to derive
`SerializeRow` with the correct flavor
@Lorak-mmk
Copy link
Collaborator

I've pushed some changes to my branch ( https://github.com/Lorak-mmk/scylla-rust-driver/tree/flatten-serialize-name ), feel free to use it.

  • Got rid of unqualified calls
  • Moved one formatting change to a different commit. Without that make static is not passing on one commit.

I have no more issues with this version, so if you decide to use it I can approve without another review.

@nrxus
Copy link
Contributor Author

nrxus commented Apr 9, 2025

I have rebased my branch against yours such that this PR now has your changes. Let me know if that's enough.

@nrxus nrxus requested review from Lorak-mmk and wprzytula April 9, 2025 16:36
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.

Really great work! I hope you don't hate us too much for that many review rounds 😅
I don't think we need to wait for @wprzytula - he approved it many times already.

@Lorak-mmk Lorak-mmk added this to the 1.2.0 milestone Apr 9, 2025
@Lorak-mmk Lorak-mmk merged commit dea72ee into scylladb:main Apr 9, 2025
12 checks passed
@nrxus
Copy link
Contributor Author

nrxus commented Apr 9, 2025

Thanks for the approval and merge! No worries about the many rounds, it was a big change so I appreciate thoroughness!

@wprzytula wprzytula mentioned this pull request Apr 12, 2025
4 tasks
@wprzytula wprzytula mentioned this pull request May 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/proc-macros Related to procedural macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants