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

New deserialization API - preparations #1065

Merged

Conversation

wprzytula
Copy link
Collaborator

@wprzytula wprzytula commented Aug 22, 2024

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 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.

@wprzytula wprzytula self-assigned this Aug 22, 2024
@wprzytula wprzytula added this to the 0.15.0 milestone Aug 22, 2024
Copy link

github-actions bot commented Aug 22, 2024

cargo semver-checks detected some API incompatibilities in this PR.
Checked commit: d47aa81

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev 5752af488655287fc332ec3dd68645ef522e0d7c
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 5752af488655287fc332ec3dd68645ef522e0d7c
     Cloning 5752af488655287fc332ec3dd68645ef522e0d7c
     Parsing scylla v0.13.0 (current)
      Parsed [  22.765s] (current)
     Parsing scylla v0.13.0 (baseline)
      Parsed [  20.042s] (baseline)
    Checking scylla v0.13.0 -> v0.13.0 (no change)
     Checked [   0.079s] 79 checks: 78 pass, 1 fail, 0 warn, 0 skip

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field col_specs of struct QueryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-5752af488655287fc332ec3dd68645ef522e0d7c/e5083df900adadf2ab85ec0d0ea2d0b0e87030b8/scylla/src/transport/query_result.rs:22
  field col_specs of struct QueryResult, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-5752af488655287fc332ec3dd68645ef522e0d7c/e5083df900adadf2ab85ec0d0ea2d0b0e87030b8/scylla/src/transport/query_result.rs:22

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [  42.933s] scylla
     Parsing scylla-cql v0.2.0 (current)
      Parsed [  10.073s] (current)
     Parsing scylla-cql v0.2.0 (baseline)
      Parsed [  10.049s] (baseline)
    Checking scylla-cql v0.2.0 -> v0.2.0 (no change)
     Checked [   0.084s] 79 checks: 76 pass, 3 fail, 0 warn, 0 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field Rows.paging_state_response in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/frame/response/result.rs:490

--- failure enum_variant_added: enum variant added on exhaustive enum ---

Description:
A publicly-visible enum without #[non_exhaustive] has a new variant.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#enum-variant-new
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/enum_variant_added.ron

Failed in:
  variant ParseError:DeserializationTypeCheckError in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla-cql/src/frame/frame_errors.rs:49

--- failure struct_pub_field_missing: pub struct's pub field removed or renamed ---

Description:
A publicly-visible struct has at least one public field that is no longer available under its prior name. It may have been renamed or removed entirely.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/struct_pub_field_missing.ron

Failed in:
  field paging_state of struct ResultMetadata, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-5752af488655287fc332ec3dd68645ef522e0d7c/e5083df900adadf2ab85ec0d0ea2d0b0e87030b8/scylla-cql/src/frame/response/result.rs:434

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  20.256s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Aug 22, 2024
@wprzytula wprzytula force-pushed the new-deserialization-api-preparations branch from 48417d0 to ace6cf0 Compare August 23, 2024 06:00
scylla/src/transport/query_result.rs Outdated Show resolved Hide resolved
scylla/src/transport/iterator.rs Outdated Show resolved Hide resolved
@@ -689,7 +689,7 @@ where
.load_balancing_policy
.on_query_success(&self.statement_info, elapsed, node);

self.paging_state = rows.paging_state.take();
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@wprzytula wprzytula force-pushed the new-deserialization-api-preparations branch from 545e829 to 5c2e8ae Compare August 26, 2024 14:43
@wprzytula
Copy link
Collaborator Author

v1.1: reverted paging_state.clone() to paging_state.take() in iterator.

@wprzytula wprzytula force-pushed the new-deserialization-api-preparations branch from 5c2e8ae to 2849ca9 Compare August 27, 2024 09:26
@wprzytula
Copy link
Collaborator Author

v1.1.1: fixed tests in QueryResult.

wprzytula and others added 8 commits August 27, 2024 12:13
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.
@wprzytula wprzytula force-pushed the new-deserialization-api-preparations branch from 2849ca9 to 20c1264 Compare August 27, 2024 11:00
@wprzytula
Copy link
Collaborator Author

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.
@wprzytula wprzytula force-pushed the new-deserialization-api-preparations branch from 20c1264 to d47aa81 Compare August 27, 2024 12:08
@wprzytula
Copy link
Collaborator Author

wprzytula commented Aug 27, 2024

v1.2.1: fixed compile error on MSRV due to:
error[E0277]: the trait bound `[u8]: Default` is not satisfied

@wprzytula wprzytula merged commit 31f512c into scylladb:main Aug 29, 2024
11 checks passed
@wprzytula wprzytula deleted the new-deserialization-api-preparations branch August 29, 2024 09:55
@wprzytula wprzytula mentioned this pull request Nov 6, 2024
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deserialization semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants