Introduce new deserialization API #1057

wprzytula · 2024-08-14T09:43:44Z

Here it is, finally!

After #1004 with preparation, #970 with lower layer and #1024 with derive macros,
then #1065 and #1088 with yet more preparations, #1082, then #1101...
this PR does introduce the uppermost layer of the new deserialization framework - finally brings it available for the user!

The Big Picture

Session is made generic over the deserialization API kind, so that both old and new API can be used in the transition period by the users. The choice is done on SessionBuilder's level, by choosing either build() or build_legacy() method.
Internal interfaces (e.g. Connection's API, especially query_iter() widely used in topology.rs) are adjusted to use the new API.
Tests, examples and doctests are adjusted to use the new API.
Deserialization logic of collections in the new framework had already been changed in a way that now it is possible to deserialize empty collections into Collection instead of Option<Collection>, even though the DB represents empty collections as nulls.
A test is added based on WIP: session_test: regression test for empty collections deserialization #1007, showing that now it is indeed possible to deserialize empty collections into Collection instead of Option<Collection>.

Details

Achieving old behaviour for dynamic deserialization purposes

If one still wants to deserialize arbitrary rows (as the old framework would do) and only then try to convert them dynamically to some proper types, this is doable by using the old Row struct as the target type, e.g.

let qr = session.query("SELECT a FROM ks.table")?.into_rows_result()?.unwrap();
let rows = qr.rows::<Row>()?;
for row in rows {
    // Type check and deserialization succeeds for any valid CQL type.

    // Do arbitrary dynamic magic with `row`.
}

TODO

A follow-up is going to bring methods for conversions between Sessions of both APIs, so that both façades are cheaply convertible into each other (by sharing their inner data with an Arc). This will enable using both APIs in one application with minimal changes needed.
Documentation needs to be adjusted. This is to be done in a follow-up.
A migration guide is planned for a follow-up.
Deprecation notices for the legacy API are done, but delayed for a follow-up not to make this PR too huge.

Fixes: #964
Fixes: #1001
Nearly_resolves: #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 · 2024-08-14T09:50:49Z

v1.0.1: clippy fixes

github-actions · 2024-08-14T09:59:55Z

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

See the following report for details:

cargo semver-checks output

./scripts/semver-checks.sh --baseline-rev dce9e9fbdcee42d3bdec024776d4a4555207831b
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev dce9e9fbdcee42d3bdec024776d4a4555207831b
     Cloning dce9e9fbdcee42d3bdec024776d4a4555207831b
     Parsing scylla v0.14.0 (current)
      Parsed [  24.492s] (current)
     Parsing scylla v0.14.0 (baseline)
      Parsed [  23.192s] (baseline)
    Checking scylla v0.14.0 -> v0.14.0 (no change)
     Checked [   0.122s] 94 checks: 91 pass, 3 fail, 0 warn, 0 skip

--- failure auto_trait_impl_removed: auto trait no longer implemented ---

Description:
A public type has stopped implementing one or more auto traits. This can break downstream code that depends on the traits being implemented.
        ref: https://doc.rust-lang.org/reference/special-types-and-traits.html#auto-traits
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/auto_trait_impl_removed.ron

Failed in:
  type TablesMetadataError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:497
  type TablesMetadataError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:497
  type ProtocolError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:279
  type ProtocolError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:279
  type MetadataError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:408
  type MetadataError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:408
  type UdtMetadataError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:484
  type UdtMetadataError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:484
  type ViewsMetadataError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:519
  type ViewsMetadataError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:519
  type KeyspacesMetadataError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:446
  type KeyspacesMetadataError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:446
  type TracingProtocolError is no longer UnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:363
  type TracingProtocolError is no longer RefUnwindSafe, in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:363

--- failure enum_tuple_variant_changed_kind: An enum tuple variant changed kind ---

Description:
A public enum's exhaustive tuple variant has changed to a different kind of enum variant, breaking possible instantiations and patterns.
        ref: https://doc.rust-lang.org/reference/items/enumerations.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_tuple_variant_changed_kind.ron

Failed in:
  variant TracingProtocolError::TracesSessionNotRows in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:366
  variant TracingProtocolError::TracesEventsNotRows in /home/runner/work/scylla-rust-driver/scylla-rust-driver/scylla/src/transport/errors.rs:378

--- failure struct_missing: pub struct removed or renamed ---

Description:
A publicly-visible struct cannot be imported by its prior path. A `pub use` may have been removed, or the struct itself 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.36.0/src/lints/struct_missing.ron

Failed in:
  struct scylla::CachingSession, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-dce9e9fbdcee42d3bdec024776d4a4555207831b/9c3341b497457ded868142ba344de3632d10bca5/scylla/src/transport/caching_session.rs:34
  struct scylla::transport::session::Session, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-dce9e9fbdcee42d3bdec024776d4a4555207831b/9c3341b497457ded868142ba344de3632d10bca5/scylla/src/transport/session.rs:158
  struct scylla::Session, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-dce9e9fbdcee42d3bdec024776d4a4555207831b/9c3341b497457ded868142ba344de3632d10bca5/scylla/src/transport/session.rs:158

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  47.865s] scylla
     Parsing scylla-cql v0.3.0 (current)
      Parsed [  11.616s] (current)
     Parsing scylla-cql v0.3.0 (baseline)
      Parsed [  11.660s] (baseline)
    Checking scylla-cql v0.3.0 -> v0.3.0 (no change)
     Checked [   0.107s] 94 checks: 94 pass, 0 skip
     Summary no semver update required
    Finished [  23.436s] scylla-cql
make: *** [Makefile:61: semver-rev] Error 1

wprzytula · 2024-08-14T10:01:21Z

v1.0.2: docstrings fixes

wprzytula · 2024-08-19T11:39:17Z

v1.0.3: tests fixes.

wprzytula · 2024-08-19T11:44:43Z

v1.0.4: docstring fixes.

Lorak-mmk

Posting the comments that I already made so that they are not lost

scylla-cql/src/types/deserialize/result.rs

scylla-cql/src/frame/frame_errors.rs

scylla-cql/src/frame/response/result.rs

scylla-cql/src/types/deserialize/result.rs

scylla/src/transport/query_result.rs

scylla/src/transport/iterator.rs

wprzytula · 2024-09-18T12:59:53Z

v2.0:

rebased on main, incorporating paging-related changes,
resolved some Karol's comments.

wprzytula · 2024-09-19T09:34:02Z

v2.1:

rebased on main.

wprzytula · 2024-09-19T10:00:39Z

v2.1.1:

fixed doctests.

Implements methods related to sending queries for the new Session. Co-authored-by: Wojciech Przytuła <[email protected]>

Adjusts `Connection::fetch_schema_version` to use the new deserialization API. Connection is meant to be an internal API, so we don't introduce a LegacyConnection for this. `Connection::{query,execute}_iter` will be migrated in a further commit. Co-authored-by: Wojciech Przytuła <[email protected]>

In a similar fashion to Session, CachingSession was also made generic over the session kind. Co-authored-by: Wojciech Przytuła <[email protected]>

After Session was made generic wrt deserialization API, the references got broken.

Adjusts the CachingSession tests to use the new deserialization interface. Co-authored-by: Wojciech Przytuła <[email protected]>

The Connection::query_iter method is changed to use the new deserialization framework. All the internal uses of it in topology.rs are adjusted. Co-authored-by: Piotr Dulikowski <[email protected]>

`query_filter_keyspace_name` is monomorphised into 5 different functions, with considerably large body each. To reduce code bloat (and potentially have better caching by calling the same code, not distinct), the large common part is degenericised and extracted.

Adjusts the Session::try_getting_tracing_info method to use the new deserialization framework. Co-authored-by: Wojciech Przytuła <[email protected]>

wprzytula · 2024-11-12T14:10:30Z

v3.1.1: amended commit message about de-genericisation - now it mentions partial de-genericisation.

muzarski

Looks good

This is a large commit which goes over all existing tests that haven't been migrated in previous commits and adjusts them to use the new deserialization framework. There were lots of changes to be made, but they are mostly independent from each other and very simple.

This commit goes over all unadjusted examples and changes them to use the new deserialization framework. Again, it contains a lot of changes, but they are quite simple. Co-authored-by: Wojciech Przytuła <[email protected]>

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>`. A test is added that checks it.

As noted in a review comment, parsing `for<'r> DeserializeValue<'r, 'r>` to understand it's requiring an owned type is nontrivial and could be replaced with a subtrait with an informative name. Therefore, this commit introduces DeserializeOwnedRow and DeserializeOwnedValue (to be used by the `scylla` crate itself only).

Not to use legacy naming, (SingleConnection)RowIteratorWorker is renamed to (SingleConnection)PagerWorker.

wprzytula · 2024-11-12T15:17:13Z

v3.2: Added 2 new commits on top of the PR:

renamed RowIteratorWorker to PagerWorker,
made lifetime constraints in QueryPager::rows_stream() more rigid to prevent errors.

@Lorak-mmk

It appears that the previous requirements: ```rust fn rows_stream< 'frame, 'metadata, RowT: 'static + DeserializeRow<'frame, 'metadata> > ``` allowed creating the `TypedRowStream<&'static str>`. It was error-prone, because the compiler would accept `rows_stream::<&str>`, happily deducing that it's `&'static str`, and failing upon Stream `next()` not being a lending method. To prevent such situations, the constraints are changed the following way (credits to @Lorak-mmk): ```rust fn rows_stream< RowT: 'static + for<'frame, 'metadata> DeserializeRow<'frame, 'metadata> > ``` and now `&'static str` is not permitted (because it only implements `DeserializeValue<'static, '_>`. Co-authored-by: Karol Baryła <[email protected]>

wprzytula requested review from Lorak-mmk and muzarski August 14, 2024 09:43

wprzytula self-assigned this Aug 14, 2024

wprzytula added this to the 0.14.0 milestone Aug 14, 2024

wprzytula added enhancement New feature or request area/deserialization labels Aug 14, 2024

wprzytula force-pushed the new-deserialization-api branch from 6038ee0 to 671bf1e Compare August 14, 2024 09:50

github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Aug 14, 2024

wprzytula force-pushed the new-deserialization-api branch from 671bf1e to 774e839 Compare August 14, 2024 10:01

wprzytula mentioned this pull request Aug 19, 2024

Destruction of collections of CqlValue is needlessly costly #1059

Open

wprzytula force-pushed the new-deserialization-api branch from 774e839 to e5982ba Compare August 19, 2024 11:38

wprzytula force-pushed the new-deserialization-api branch from e5982ba to 088a9ab Compare August 19, 2024 11:44

wprzytula modified the milestones: 0.14.0, 0.15.0 Aug 20, 2024

Lorak-mmk requested changes Aug 21, 2024

View reviewed changes

wprzytula marked this pull request as draft August 22, 2024 16:58

wprzytula mentioned this pull request Aug 22, 2024

New deserialization API - preparations #1065

Merged

5 tasks

wprzytula force-pushed the new-deserialization-api branch from 088a9ab to a0e7f2f Compare September 18, 2024 12:58

wprzytula force-pushed the new-deserialization-api branch from a0e7f2f to aef9803 Compare September 19, 2024 09:33

wprzytula force-pushed the new-deserialization-api branch from aef9803 to 93f3174 Compare September 19, 2024 10:00

wprzytula force-pushed the new-deserialization-api branch from 93f3174 to f104336 Compare October 2, 2024 12:03

wprzytula force-pushed the new-deserialization-api branch from f104336 to 611e48c Compare October 15, 2024 14:19

piodul and others added 8 commits November 12, 2024 15:05

session: add interface methods for the new deser API

22f28cd

Implements methods related to sending queries for the new Session. Co-authored-by: Wojciech Przytuła <[email protected]>

caching_session: make generic over session APIs

b3f4a04

In a similar fashion to Session, CachingSession was also made generic over the session kind. Co-authored-by: Wojciech Przytuła <[email protected]>

caching_session: fix docstring references

001b5bb

After Session was made generic wrt deserialization API, the references got broken.

caching_session: modernize tests

db6bee0

Adjusts the CachingSession tests to use the new deserialization interface. Co-authored-by: Wojciech Przytuła <[email protected]>

connection: migrate query_iter to new deserialization framework

6d9d971

The Connection::query_iter method is changed to use the new deserialization framework. All the internal uses of it in topology.rs are adjusted. Co-authored-by: Piotr Dulikowski <[email protected]>

{session,tracing}: switch to the new deser framework for tracing info

2b5f386

Adjusts the Session::try_getting_tracing_info method to use the new deserialization framework. Co-authored-by: Wojciech Przytuła <[email protected]>

wprzytula force-pushed the new-deserialization-api branch from 8384730 to 11c8406 Compare November 12, 2024 14:09

wprzytula requested a review from Lorak-mmk November 12, 2024 14:10

muzarski approved these changes Nov 12, 2024

View reviewed changes

wprzytula and others added 6 commits November 12, 2024 16:07

examples: adjust to use the new interface

9a092f9

This commit goes over all unadjusted examples and changes them to use the new deserialization framework. Again, it contains a lot of changes, but they are quite simple. Co-authored-by: Wojciech Przytuła <[email protected]>

codewide: migrate doctests to new deser API

a204a7b

iterator: rename RowIteratorWorker to PagerWorker

98b382d

Not to use legacy naming, (SingleConnection)RowIteratorWorker is renamed to (SingleConnection)PagerWorker.

wprzytula force-pushed the new-deserialization-api branch from 4971667 to d342dfe Compare November 12, 2024 15:14

wprzytula force-pushed the new-deserialization-api branch from d342dfe to d4a222c Compare November 12, 2024 15:18

Lorak-mmk approved these changes Nov 12, 2024

View reviewed changes

wprzytula merged commit e99d697 into scylladb:main Nov 12, 2024
10 of 11 checks passed

wprzytula deleted the new-deserialization-api branch November 12, 2024 15:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce new deserialization API #1057

Introduce new deserialization API #1057

wprzytula commented Aug 14, 2024 •

edited

Loading

wprzytula commented Aug 14, 2024

github-actions bot commented Aug 14, 2024 •

edited

Loading

wprzytula commented Aug 14, 2024

wprzytula commented Aug 19, 2024

wprzytula commented Aug 19, 2024

Lorak-mmk left a comment

wprzytula commented Sep 18, 2024

wprzytula commented Sep 19, 2024

wprzytula commented Sep 19, 2024

wprzytula commented Nov 12, 2024

muzarski left a comment

wprzytula commented Nov 12, 2024

Introduce new deserialization API #1057

Introduce new deserialization API #1057

Conversation

wprzytula commented Aug 14, 2024 • edited Loading

Here it is, finally!

The Big Picture

Details

Achieving old behaviour for dynamic deserialization purposes

TODO

Pre-review checklist

wprzytula commented Aug 14, 2024

github-actions bot commented Aug 14, 2024 • edited Loading

wprzytula commented Aug 14, 2024

wprzytula commented Aug 19, 2024

wprzytula commented Aug 19, 2024

Lorak-mmk left a comment

Choose a reason for hiding this comment

wprzytula commented Sep 18, 2024

wprzytula commented Sep 19, 2024

wprzytula commented Sep 19, 2024

wprzytula commented Nov 12, 2024

muzarski left a comment

Choose a reason for hiding this comment

wprzytula commented Nov 12, 2024

wprzytula commented Aug 14, 2024 •

edited

Loading

github-actions bot commented Aug 14, 2024 •

edited

Loading