-
Notifications
You must be signed in to change notification settings - Fork 110
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
New deserialization API - preparations #1065
New deserialization API - preparations #1065
Conversation
See the following report for details: cargo semver-checks output
|
48417d0
to
ace6cf0
Compare
scylla/src/transport/iterator.rs
Outdated
@@ -689,7 +689,7 @@ where | |||
.load_balancing_policy | |||
.on_query_success(&self.statement_info, elapsed, node); | |||
|
|||
self.paging_state = rows.paging_state.take(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here as well (regarding @Lorak-mmk 's comment). I think this should belong to other commit. I am not even sure why we were modifying received rows
(by moving the paging state out of them), even though we send them via some channel later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that because the iterator manages paging state itself, and it does not expose any getters of it.
545e829
to
5c2e8ae
Compare
v1.1: reverted |
5c2e8ae
to
2849ca9
Compare
v1.1.1: fixed tests in QueryResult. |
ValueList is a deprecated API.
As the old deserialization framework only validates types when some row is actually converted into the end type (and not if 0 rows were received), the error went unnoticed. The new framework, however, failed with the bad type provided.
The impl was missing, and lack of it makes it impossible to e.g. unwrap a `Result<TypedRowIterator, SomeError>`.
This was overlooked in a previous PR, where TypeCheckError was introduced.
This is a temporary measure, until error refactor gets rid of the awful QueryError::InvalidMessage(String) variant and replaces it with a decent matchable error.
The new deserialization API will need to receive ownership of the serialized frame for two reasons: - To be able to deserialize to types that borrow from the frame (e.g. `text` as &str), - To be able to deserialize to types that share ownership of the bytes (e.g. `blob` -> bytes::Bytes).
ScyllaDB does not distinguish empty collections from nulls. That is, INSERTing an empty collection is equivalent to nullifying the corresponding column. As pointed out in [scylladb#1001](scylladb#1001), it's a nice QOL feature to be able to deserialize empty CQL collections to empty Rust collections instead of `None::<RustCollection>`. Deserialization logic is modified to enable that.
This aids readability.
2849ca9
to
20c1264
Compare
v1.2: rebased on main. |
Although paging state comes from the DB in the <result_metadata> CQL protocol entity, it should be considered separate. The main reason for that is that PreparedStatement also receives (and optionally caches) ResultMetadata upon preparation, which is then used for an optimisation (col_specs are not sent in RESULT::Rows response by the DB). However, paging state should not be cached this way, as it's going to change in between execution of the same statement with different paging state given. In the next commit ResultMetadata is going to be shared between PreparedStatement and its QueryResults by an Arc, and for that paging state must be kept separate.
This is preparation for next commits, where QueryResult is going to hold Arc<ResultMetadata> instead of Vec<ColSpec>.
For now, the metadata is owned. In the next commit, metadata's ownership is made shared by Arc.
ResultMetadata is now passed behind an Arc, enabling shared ownership between PreparedStatement and QueryResult and RowIterator. Thanks to that, if `use_cached_metadata` flag is set on PreparedStatement, no new allocations occur when deserializing ResultMetadata on a request result.
20c1264
to
d47aa81
Compare
v1.2.1: fixed compile error on MSRV due to: |
This PR consists the first bunch of minor preparatory steps for the main leap of the deserialization refactor, extracted from #1057 as @Lorak-mmk requested.
For explanation and broader context see #1057.
Ref: #462
Pre-review checklist
[ ] I added relevant tests for new features and bug fixes.[ ] I have adjusted the documentation in./docs/source/
.[ ] I added appropriateFixes:
annotations to PR description.