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.

Sign up for GitHub

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

Jump to bottom

scylla_cql cleanup after legacy APIs removal #1198

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

scylla_cql cleanup after legacy APIs removal #1198

wants to merge 15 commits into from

Conversation

Lorak-mmk
Copy link
Collaborator

@Lorak-mmk Lorak-mmk commented Feb 4, 2025 •
edited
Loading

After removing the legacy serialization / deserialization APIs it became clear that some things in scylla_cql are not placed where they should be.
This PR tries to address it.

PR based on #1184 . You can start the review with commit "scylla_cql/serialize: Extract value tests to a separate file".

Ser / deser tests

They are placed in several places, making it hard to follow what tests we actually have.
We should strive to put them together.
Where are the tests and how does this PR change it?

  • In deserialize module in files separate from implementation (row_tests.rs, value_tests.rs). This is good and so left unchanged.
  • In serialize module inside the files with implementation. I extracted those tests to separate files (row_tests.rs, value_tests.rs) to reduce file size and be more similar to deserialize module.
  • In frame/value_tests.rs file. Those were some of the serialization tests for both values, rows, and batches. I moved them to relevant files in serialize module.
  • In frame/result/response.rs file. Those were some of the deserialization tests for CqlValue. I moved them to deserialize/value_tests.rs file.

value module

This module in scylla_cql is placed in frame module and contained multiple things:

  1. Old serialization APIs (Value and ValueList traits and their implementations)
  2. Our ser/deser related types like CqlValue, CqlTimeuuid etc

After removing (1), and removing cql_to_rust module which was also in frame, I find it a bit weird
for this module to be placed in frame. Why? Previously types like CqlValue and CqlTimeuuid
were strongly tied to frame serialization / deserialization, because they were used in CqlValue,
which was always used when deserializing result values.
With the new API this is no longer the case, so there is no more reason to keep those types in frame module.

I moved this file into the root folder, so value is now a top-level module.

CqlValue and Row

Those were placed in scylla-cql/frame/response/result.rs. As described in the previous point, this location made
sense with the old deserialization API because CqlValue and Row were strongly tied to frame deserialization.

With the new API they are not anymore - they are normal types than can be serialized / deserialized, and are kept
mostly to address dynamic use cases. With this change, result.rs no longer seems like a good place for them.

I moved them to the newly created value.rs top-level module, so it is placed close to CqlTimeuuid and other such types.
This may be a bit of a weird place for a Row struct, but I don't have any better ideas.

Fixes #1167

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.

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Feb 4, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Feb 4, 2025 •
edited
Loading

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

See the following report for details:

cargo semver-checks output 
./scripts/semver-checks.sh --baseline-rev 253c514d6b3fdcae4be7c26580b89902f1fa9fee
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 253c514d6b3fdcae4be7c26580b89902f1fa9fee
     Cloning 253c514d6b3fdcae4be7c26580b89902f1fa9fee
    Building scylla v0.15.0 (current)
       Built [  24.441s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.049s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  22.752s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.045s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.118s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  48.404s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.475s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.028s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  11.158s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.027s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.103s] 127 checks: 123 pass, 4 fail, 0 warn, 0 skip

--- failure enum_missing: pub enum removed or renamed ---

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

Failed in:
  enum scylla_cql::frame::value::MaybeUnset, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:18
  enum scylla_cql::frame::response::result::CqlValue, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/response/result.rs:191

--- failure function_missing: pub fn removed or renamed ---

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

Failed in:
  function scylla_cql::frame::response::result::deser_cql_value, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/response/result.rs:1308

--- failure module_missing: pub module removed or renamed ---

Description:
A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public.
        ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.39.0/src/lints/module_missing.ron

Failed in:
  mod scylla_cql::frame::value, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:1

--- 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.39.0/src/lints/struct_missing.ron

Failed in:
  struct scylla_cql::frame::value::CqlDecimal, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:450
  struct scylla_cql::frame::value::ValueOverflow, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:6
  struct scylla_cql::frame::value::CqlTimestamp, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:577
  struct scylla_cql::frame::value::Counter, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:14
  struct scylla_cql::frame::value::CqlDecimalBorrowed, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:464
  struct scylla_cql::frame::value::CqlVarintBorrowed, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:223
  struct scylla_cql::frame::value::CqlVarint, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:216
  struct scylla_cql::frame::response::result::Row, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/response/result.rs:657
  struct scylla_cql::frame::value::CqlTimeuuid, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:29
  struct scylla_cql::frame::value::Unset, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:10
  struct scylla_cql::frame::value::CqlTime, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:583
  struct scylla_cql::frame::value::CqlDate, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:571
  struct scylla_cql::frame::value::CqlDuration, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-253c514d6b3fdcae4be7c26580b89902f1fa9fee/73f2898b5ba3f8848afe690cace1bdedca96bc82/scylla-cql/src/frame/value.rs:777

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

@Lorak-mmk Lorak-mmk mentioned this pull request Feb 4, 2025
Merged
8 tasks
@Lorak-mmk Lorak-mmk force-pushed the scylla_cql_cleanup branch 3 times, most recently from 6b48f80 to 030bf81 Compare February 4, 2025 15:33
muzarski
muzarski approved these changes Feb 4, 2025
View reviewed changes
Copy link
Contributor

@muzarski muzarski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea: maybe extend test_types_imports test? Currently we only test for _scylla::value::CqlValue, but now that we explicitly re-export types from this module it makes sense to test imports for them as well.

@Lorak-mmk
Copy link
Collaborator Author

Makes sense, I'll do it soon

@muzarski muzarski mentioned this pull request Feb 4, 2025
Open
8 tasks
@Lorak-mmk
Copy link
Collaborator Author

Added the rest of the types to the test

@Lorak-mmk
scylla_cql/serialize: Extract value tests to a separate file
406e323
@Lorak-mmk
scylla_cql/serialize: Extract row tests to a separate file
b1a8cec
@Lorak-mmk
frame/value_tests: Use do_serialize from serialization tests
6120dcd 
We want to move tests from this file into serialization module.
In order for the move commit to be simpler, the tests should be made
more similar before.
@Lorak-mmk
frame/value_tests: Use other utils from serialization tests
a096005 
We want to move tests from this file into serialization module.
In order for the move commit to be simpler, the tests should be made
more similar before.
@Lorak-mmk
Move relevant tests tests from frame/value_tests to serialize/value_t…
29c097d 
…ests
@Lorak-mmk
row_tests.rs: Remove col_spec helper
af47071 
There is another equivalent function already: `col`.
@Lorak-mmk
row_tests.rs: Move test_dyn_serialize_row after util functions
c6eb54f
@Lorak-mmk
frame/value_tests.rs: Rename col_spec to col
fbb32dd 
That's how this helper function is named in serialize/row_tests.rs.
@Lorak-mmk
Move relevant tests tests from frame/value_tests to serialize/row_tests
eb52270
@Lorak-mmk
Move batch tests from frame/value_tests.rs to serialize/batch_tests.rs
e9349dc 
This allows us to finally get rid of frame/value_tests.rs!
@Lorak-mmk
Move scylla-cql/frame/value.rs to scylla-cql/value.rs
3e4cb64
@Lorak-mmk
Move tests tests from frame/response/result.rs to deserialize/value_t…
587ff9e 
…ests

Those tests only test CqlValue deserialization, and we want to have
serialization / deserialization tests grouped together.
@Lorak-mmk
Move CqlValue and Row to scylla_cql::value module
c3b1fc1 
Previously those types were located in scylla_cql::frame::response::result.
This is because in old framework they were strongly coupled to the
result frame: values were always deserialized into `Vec` of `Row`s, which
in turn were `Vec`s of `CqlValue`.

In the new framework this coupling is gone. New traits allow deserialization
from raw bytes instead of `CqlValue`, which means that those types are
no more important than any other, and can be moved to the module with
other values.

scylla_cql::values may be a bit of a weird place for a `Row` struct,
but I don't see any better place.
@Lorak-mmk
scylla/src/lib.rs: Don't re-export whole scylla_cql::value
7986a62 
Other scylla_cql re-exports don't usually re-export whole modules, only
what is necessary for scylla crate public API. This commit does the same
for `scylla_cql::value` re-export. All public items apart from
deser_cql_value are re-exported.
@Lorak-mmk
Hygiene test: Extend import test
3db46f6 
Now that values (CqlValue etc) are manually re-exported in lib.rs
we should test the correctness of this re-export.
@Lorak-mmk
Copy link
Collaborator Author

Rebased on main

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@muzarski muzarski muzarski approved these changes

@wprzytula wprzytula Awaiting requested review from wprzytula

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove legacy serialization and deserialization frameworks
2 participants
@Lorak-mmk @muzarski