types: serialize: constrain the new serialization traits to make them easier and safer to use #855
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
piodul
force-pushed
the
adjust-interface
branch
2 times, most recently
from
281cabd to
25677ba
Compare
12 tasks
|
@Lorak-mmk review ping |
piodul
force-pushed
the
adjust-interface
branch
from
25677ba to
cc2476c
Compare
|
v2:
|
piodul
force-pushed
the
adjust-interface
branch
2 times, most recently
from
e672d2f to
7038cbc
Compare
|
v2.1:
|
Introduce the `read_value` function which is able to read a [value], as specified in the CQL protocol. It will be used in the next commit, in order to make the interface of the SerializedValue iterators more correct.
Currently, the SerializedValues' `iter()` method treats both null and unset values as None, and `iter_name_value_pairs()` just assumes that values are never null/unset and panics if they are. Make the interface more correct by adjusting both methods to return RawValue. The iterators will be used in the next commit to implement the fallback that allows to implement `SerializeRow`/`SerializeCql` via legacy `ValueList`/`Value` traits.
piodul
force-pushed
the
adjust-interface
branch
from
7038cbc to
448acd2
Compare
Lorak-mmk
approved these changes
Nov 23, 2023
|
Actually I have one question: is mod.rs the best place for those new traits and structs? I don't really know Rust conventions in this matter, but it is quite a lot of code, wouldn't it be better to make a new file for it? |
Makes sense, I can move the |
piodul
force-pushed
the
adjust-interface
branch
from
448acd2 to
a00b284
Compare
|
v3: moved the newly introduced types and traits to a separate module (they are reexported from the place they were defined previously) |
… interfaces Currently, `SerializeRow` and `SerializeCql` traits are just given a mutable reference to a Vec<u8> and asked to append their CQL representation to the end. While simple, there are some issues with the interface: - The serialize method has access to the serialized representation of the values that were appended before it. It's not necessary for a correct implementation to have access to it. - Implementors technically can append any byte sequence to the end, but actually are expected to produce a CQL [value] containing the serialized value. While the `SerializeRow` and `SerializeCql` traits are not generally meant to be manually implemented by the users, we can make the interface easier to use and harder to misuse by making it append-only, restricting what the users are allowed to append and requiring the users to append anything by using a dash of type-level magic. Introduce `RowWriter` and `CellWriter` traits which satisfy the above wishes and constraints, and pass them instead of Vec<u8> in `SerializeRow` and `SerializeCql`. The new traits have two implementations - a Vec<u8> backed one that actually appends the bytes given to it, and a usize-backed one which just measures the length of the output without writing anything. Passing the latter before doing the actual serialization will allow to preallocate the right amount of bytes and then serialize without reallocations. It should be measured whether the reallocation cost always outweighs the calculation cost before implementing this optimization.
piodul
force-pushed
the
adjust-interface
branch
from
a00b284 to
29a37b4
Compare
|
v3.1: fixed links in the docs |
Lorak-mmk
approved these changes
Nov 24, 2023
Yes, I'll go ahead and merge it. |
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently,
SerializeRowandSerializeCqltraits are just given a mutable reference to a Vec and asked to append their CQL representation to the end. While simple, there are some issues with the interface:While the
SerializeRowandSerializeCqltraits are not generally meant to be manually implemented by the users, we can make the interface easier to use and harder to misuse by making it append-only, restricting what the users are allowed to append and requiring the users to append anything by using a dash of type-level magic.Introduce
RowWriterandCellWritertraits which satisfy the above wishes and constraints, and pass them instead of Vec inSerializeRowandSerializeCql.The new traits have two implementations - a Vec backed one that actually appends the bytes given to it, and a usize-backed one which just measures the length of the output without writing anything. Passing the latter before doing the actual serialization will allow to preallocate the right amount of bytes and then serialize without reallocations. It should be measured whether the reallocation cost always outweighs the calculation cost before implementing this optimization.
Refs: #801
I have adjusted the documentation in./docs/source/.I added appropriateFixes:annotations to PR description.