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

rename Query to Statement #1250

Merged
merged 6 commits into from
Feb 27, 2025
Merged

rename Query to Statement #1250

merged 6 commits into from
Feb 27, 2025

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Feb 17, 2025

Fixes: #713

Changes

Renamed Query to Statement (first commit)

New name is definitely better than the old one. However, the question is if it's the best. Other candidates:

  • SimpleStatement - used in other drivers, but probably a bit too verbose (just Statement is Simple enough).
  • UnpreparedStatement - verbose, but OTOH immediately gives user all information required about the type - it's literally an unprepared statement. The main downside is that the name is pretty long.

Anyway, whatever the choice, the adjustment should be rather trivial (now that I spotted all mentions in the docs and docstrings).

Renamed query.rs to unprepared.rs

Adjusted docstrings and variable names to statement terminology (third commit)

Adjusted docs (book) to statement terminology

The word query was abused in the docs. In some cases, we would even call the INSERT ... CQL statement a query. The last commit hopefully fixes that.

Apart from that, regarding the docs structure:

  • renamed queries directory to statements
  • renamed queries/queries.md to statements/statements.md
  • renamed queries/simple.md to statements/unprepared.md

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.

@muzarski muzarski self-assigned this Feb 17, 2025
@muzarski muzarski mentioned this pull request Feb 17, 2025
8 tasks
Copy link

github-actions bot commented Feb 17, 2025

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

See the following report for details:

cargo semver-checks output
./scripts/semver-checks.sh --baseline-rev 58d1aef7452dca2d2393ce7579011618d6d566f9
+ cargo semver-checks -p scylla -p scylla-cql --baseline-rev 58d1aef7452dca2d2393ce7579011618d6d566f9
     Cloning 58d1aef7452dca2d2393ce7579011618d6d566f9
    Building scylla v0.15.0 (current)
       Built [  36.506s] (current)
     Parsing scylla v0.15.0 (current)
      Parsed [   0.050s] (current)
    Building scylla v0.15.0 (baseline)
       Built [  34.529s] (baseline)
     Parsing scylla v0.15.0 (baseline)
      Parsed [   0.048s] (baseline)
    Checking scylla v0.15.0 -> v0.15.0 (no change)
     Checked [   0.111s] 127 checks: 124 pass, 3 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::statement::prepared_statement::PartitionKeyExtractionError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:469
  enum scylla::prepared_statement::PartitionKeyExtractionError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:469
  enum scylla::statement::prepared_statement::PartitionKeyError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:483
  enum scylla::prepared_statement::PartitionKeyError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:483
  enum scylla::batch::BatchStatement, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/batch.rs:170
  enum scylla::statement::prepared_statement::TokenCalculationError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:475
  enum scylla::prepared_statement::TokenCalculationError, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:475

--- 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::batch, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/batch.rs:1
  mod scylla::statement::query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/query.rs:1
  mod scylla::query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/query.rs:1
  mod scylla::statement::prepared_statement, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:1
  mod scylla::prepared_statement, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.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::statement::query::Query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/query.rs:13
  struct scylla::query::Query, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/query.rs:13
  struct scylla::statement::prepared_statement::PreparedStatement, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:92
  struct scylla::prepared_statement::PreparedStatement, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/prepared_statement.rs:92
  struct scylla::batch::Batch, previously in file /home/runner/work/scylla-rust-driver/scylla-rust-driver/target/semver-checks/git-58d1aef7452dca2d2393ce7579011618d6d566f9/2cd0e1a20212d9bae3993d1e1046de754523753c/scylla/src/statement/batch.rs:17

     Summary semver requires new major version: 3 major and 0 minor checks failed
    Finished [  72.384s] scylla
    Building scylla-cql v0.4.0 (current)
       Built [  11.073s] (current)
     Parsing scylla-cql v0.4.0 (current)
      Parsed [   0.029s] (current)
    Building scylla-cql v0.4.0 (baseline)
       Built [  10.981s] (baseline)
     Parsing scylla-cql v0.4.0 (baseline)
      Parsed [   0.029s] (baseline)
    Checking scylla-cql v0.4.0 -> v0.4.0 (no change)
     Checked [   0.112s] 127 checks: 127 pass, 0 skip
     Summary no semver update required
    Finished [  22.804s] 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 Feb 17, 2025
@muzarski muzarski requested a review from Lorak-mmk February 17, 2025 18:30
Comment on lines +1 to 4
//! `ExecutionProfile` is a grouping of configurable options regarding CQL statement execution.
//!
//! Profiles can be created to represent different workloads, which thanks to them
//! can be run conveniently on a single session.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Terminology: I'm not sure I like the term "statement execution". "request execution" seems better.
The way I understand the terminology we are aiming towards:

  • statement: a string with the CQL command or an object representing it and parameters (like consistency)
  • prepared statement: object representing the result of statement preparation and parameters
  • query: CQL SELECT statement
  • request: an abstract meaning some command that we want to execute on the server.

Given that terminlogy, "statement execution" doesn't make much sense because statement represents just a string with CQL (+ parameters).

Copy link
Collaborator

@wprzytula wprzytula Feb 25, 2025

Choose a reason for hiding this comment

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

Terminology: I'm not sure I like the term "statement execution". "request execution" seems better. The way I understand the terminology we are aiming towards:

  • statement: a string with the CQL command or an object representing it and parameters (like consistency)

  • prepared statement: object representing the result of statement preparation and parameters

  • query: CQL SELECT statement

  • request: an abstract meaning some command that we want to execute on the server.

Given that terminlogy, "statement execution" doesn't make much sense because statement represents just a string with CQL (+ parameters).

"statement execution" makes sense if applied to the very activity of the DB that performs actions described in the statement. OTOH, when referring to the driver's work around statements, it's definitely not executing statements - only executing requests that pass statements for execution on the server side.

Copy link
Collaborator

@wprzytula wprzytula Feb 25, 2025

Choose a reason for hiding this comment

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

request: an abstract meaning some command that we want to execute on the server.

My understanding of request is: a CQL protocol message, as well as the whole process of creating and sending it, possibly containing a CQL statement that is to be executed on server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep in mind, though, that some configuration options are set for the very statement execution on the server side. Examples include consistency, server-side timeouts, etc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, I believe the distinction between request execution and statement execution is not crucial and can be skipped over in majority of situations.

@muzarski muzarski requested a review from wprzytula February 25, 2025 13:36
Comment on lines +1 to 4
//! `ExecutionProfile` is a grouping of configurable options regarding CQL statement execution.
//!
//! Profiles can be created to represent different workloads, which thanks to them
//! can be run conveniently on a single session.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Keep in mind, though, that some configuration options are set for the very statement execution on the server side. Examples include consistency, server-side timeouts, etc.

Comment on lines +1 to 4
//! `ExecutionProfile` is a grouping of configurable options regarding CQL statement execution.
//!
//! Profiles can be created to represent different workloads, which thanks to them
//! can be run conveniently on a single session.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Overall, I believe the distinction between request execution and statement execution is not crucial and can be skipped over in majority of situations.

Comment on lines 349 to +370
/// Sends a request to the database and receives a response.\
/// Performs an unpaged query, i.e. all results are received in a single response.
/// Executes an unprepared CQL statement without paging, i.e. all results are received in a single response.
Copy link
Collaborator

Choose a reason for hiding this comment

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

💭 👍 This makes sense to me. We send a CQL (protocol) request and, this way, we execute a CQL (language) statement, or, more precisely, we have the CQL statement executed on the server.

@muzarski
Copy link
Contributor Author

Rebased on main

@muzarski muzarski force-pushed the rename-query branch 2 times, most recently from 53c5a33 to 809a700 Compare February 26, 2025 16:23
@muzarski
Copy link
Contributor Author

v2:

  • removed the top-level re-export for statement::* modules. The statement types now need to accessed using scylla::statement::* prefix
  • renamed prepared_statement.rs -> prepared.rs
  • renamed statement.rs -> unprepared.rs
  • adjusted docstrings and book (@wprzytula comments)

I did not abandon the "statement execution" terminology yet. I think this is still to be discussed.

Lorak-mmk
Lorak-mmk previously approved these changes Feb 26, 2025
wprzytula
wprzytula previously approved these changes Feb 27, 2025
@wprzytula wprzytula added this to the 1.0.0 milestone Feb 27, 2025
- renamed `queries` directory to `statements`.
- renamed `queries.md` -> `statements.md`.
- rename `simple.md` -> `unprepared.md`

Apart from that, I adjusted the documentation to use the new terminology.
@wprzytula wprzytula merged commit f591696 into scylladb:main Feb 27, 2025
12 checks passed
@Lorak-mmk Lorak-mmk mentioned this pull request Mar 4, 2025
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Replace Query with a more fitting name
3 participants