serialization: implement new, type-safe variants of BatchValues #881
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
We will introduce types with the same name but different interface, and we want to move off the current ones gradually. Rename the existing ones as the first step.
Lorak-mmk
reviewed
Dec 15, 2023
Add a method to RowWriter which allows copying all contents of some SerializedValues object to it. Without it, the only method would be to parse the values in the SerializedValues via iter() and write them to the RowWriter, which is unnecessarily ineffective.
We will need to pass an empty row serialization context when we serialize values for unprepared queries in a batch.
It's not needed to use a block to make sure that `writer` is dropped before `data` is used, non-lexical lifetimes already take care of that.
It will be used to serialize data produced by upcoming batch values iterators.
A small refactor in order to improve the clarity of the later commits.
A small refactor to improve the clarity of later commits.
This refactor will improve clarity of the next change, but will also prevent an obscure issue from happening that causes futures not to be `Send` when an instance of a GAT exists across an await point.
If we changed the code to use the new BatchValues API first, the compiler would complain about some lifetime issues of temporary objects passed to the `match` block.
The PreparedStatement::calculate_token method takes an object that implements SerializeRow, serializes it to SerializedValues and returns the token. For the sake of an optimization in the code that handles batches we would like to provide a SerializedValues object directly. Extract the part that calculates token from SerializedValues to a new, separate method, but keep it pub(crate) - we'd like to avoid exposing type-unsafe interfaces to the users.
`SerializableRequest::serialize` accepts `impl BufMut`, but in reality we only pass `Vec<u8>` as an argument. Change the type of the argument to Vec<u8>, as in further commits we will need the argument to be precisely Vec<u8>.
piodul
force-pushed
the
new-serialize-api-batches
branch
from
59534dd to
afde4a1
Compare
AFAIK there is not standard library definition of a lending iterator. Not sure whether there are "industry standard" crates that would define such a trait. Besides, this issue is irrelevant in the newest version of the PR. |
piodul
force-pushed
the
new-serialize-api-batches
branch
from
afde4a1 to
c861937
Compare
|
Marking as "ready", I'll update the docs in a separate PR. |
8 tasks
Lorak-mmk
requested changes
Dec 15, 2023
Comment on lines
23
to
30
| /// An iterator-like for `ValueList` | ||
| /// | ||
| /// An instance of this can be easily obtained from `IT: Iterator<Item: ValueList>`: that would be | ||
| /// `BatchValuesIteratorFromIterator<IT>` | ||
| /// | ||
| /// It's just essentially making methods from `ValueList` accessible instead of being an actual iterator because of | ||
| /// compiler limitations that would otherwise be very complex to overcome. | ||
| /// (specifically, types being different would require yielding enums for tuple impls) |
There was a problem hiding this comment.
In this comment you probably meant SerializedValues, not ValueList, right?
There was a problem hiding this comment.
Right, this comment was copied over from the legacy BatchValuesIterator but not updated. I'll update in a second.
piodul
force-pushed
the
new-serialize-api-batches
branch
2 times, most recently
from
cfd9e97 to
fcdeb08
Compare
Lorak-mmk
reviewed
Dec 15, 2023
| fn batch_values_iter(&self) -> Self::RawBatchValuesIter<'_>; | ||
| } | ||
|
|
||
| /// An iterator-like for `ValueList` |
There was a problem hiding this comment.
One more value list hides here @piodul
Introduces the successors of the previous BatchValues trait and its friends. The structure of the new traits is similar to the old traits, but they come in two flavors: - BatchValue, BatchValueIterator - those are user-facing traits. They allow iterating over the sets of values for batch's statements, but need to have the information about the names and types of the columns/bind markers supplied from the outside. - RawBatchValues, RawBatchValueIterator, RawSerializeRow - those serve as a glue between the logic in `scylla` and `scylla-cql`. They are analogous to `BatchValues`, `BatchValueIterator` and `SerializeRow`, but do not need the type information to be able to serialize themselves into the request. Co-authored-by: Karol Baryła <[email protected]>
Implement an adapter layer which takes a `BatchValues` object (which needs type information to serialize), pairs it with an iterator over `RowSerializationContext` objects and returns something which implements `RawBatchValues` (which don't require type information to serialize). It will be used by the `scylla` crate to pass batch data to `scylla-cql` in a type-erased form.
The purpose of the new struct is to enable token-aware routing of batches - which already exists in the old API - in the new API. Batches are routed according to the token calculated based on the first statement in the batch (if the first statement is a prepared statement). Calculation of the token must happen before the load balancing policy computes a plan and chooses the first connection, but serialization only happens after a connection is chosen. In order not to repeat serialization work, BatchValuesFirstSerialized wrapper can be used to transform a BatchValues into another BatchValues which caches the result of the first serialization. The types are put into a module in the `scylla` crate and hidden inside it. The wrapping functionality is exposed via a function which constructs the BatchValuesFirstSerialized object but returns it as `impl BatchValues`.
The driver is updated to use the new BatchValues API on all layers at once. Fortunately, there aren't many changes and they are mostly simple.
Simplify the logic of `Session::batch` by moving the parts responsible for token calculation and wrapping the `BatchValues` argument into a separate function in the `batch_values` module.
In case somebody has a custom implementation of BatchValues, they can use the adapter.
This way can reuse the existing tests for impls of the new traits and see that they behave in the same way.
Migration of batches to the new API is complete, so this method is no longer needed and can finally be removed.
piodul
force-pushed
the
new-serialize-api-batches
branch
from
fcdeb08 to
fbeafd7
Compare
Lorak-mmk
approved these changes
Dec 15, 2023
This was referenced Dec 21, 2023
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.
This PR replaces the existing BatchValues API with one that leverages the recently introduced
SerializeCql/SerializeRowinfrastructure and is type-safe.Refs: #801
Pre-review checklist
./docs/source/.Fixes:annotations to PR description.