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

prepared: docs for PreparedStatement #986

Merged
merged 2 commits into from
May 14, 2024

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Apr 19, 2024

In response to #970 (comment).

Added docstring for PreparedStatement, which mentions:

  • how to prepare a statement
  • benefits of using prepared statements
  • statement repreparation that is handled by the driver
  • prepared statement cloning
  • correlation between prepared statements and schema altering

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 Apr 19, 2024
@muzarski muzarski requested review from Lorak-mmk and wprzytula April 19, 2024 07:54
Copy link

github-actions bot commented Apr 19, 2024

cargo semver-checks found no API-breaking changes in this PR! 🎉🥳
Checked commit: 10556e3

scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
@muzarski muzarski force-pushed the docstrings_for_cached_metadata branch from 4798ca3 to 89fcb27 Compare April 24, 2024 13:37
@muzarski muzarski changed the title prepared: docs mention about metadata immutability prepared: docs for PreparedStatement Apr 24, 2024
@muzarski muzarski force-pushed the docstrings_for_cached_metadata branch from 89fcb27 to 08baf66 Compare April 25, 2024 12:27
docs/source/queries/unprepared.md Outdated Show resolved Hide resolved
@@ -15,7 +15,7 @@ Although optimized for Scylla, the driver is also compatible with [Apache Cassan
* [Quick start](quickstart/quickstart.md) - Setting up a Rust project using `scylla-rust-driver` and running a few queries
* [Migration guides](migration-guides/migration-guides.md) - How to update the code that used an older version of this driver
* [Connecting to the cluster](connecting/connecting.md) - Configuring a connection to scylla cluster
* [Making queries](queries/queries.md) - Making different types of queries (simple, prepared, batch, paged)
* [Making queries](queries/queries.md) - Making different types of queries (unprepared, prepared, batch, paged)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's ditch the notion of query from the driver and use statement exclusively. Here, I would replace making queries with creating and executing statements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would require a change in naming (replacing Query with UnpreparedStatement), otherwise you would have a Query type but not of the docs would mention queries. What do yuo think about splitting in in 2 PRs - in this one let's improve prepared statement docs, and in the next one let's replace Query with UnpreparedStatement (and fix docs), which would fix #713

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, I can take care of that.

scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Show resolved Hide resolved
docs/source/queries/prepared.md Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
scylla/src/statement/prepared_statement.rs Outdated Show resolved Hide resolved
@muzarski muzarski force-pushed the docstrings_for_cached_metadata branch from 08baf66 to 0530445 Compare May 6, 2024 13:17
@muzarski
Copy link
Contributor Author

muzarski commented May 6, 2024

v2: addressed review comments:

  • removed simple -> unprepared commits (will be introduced in separate PR)
  • rephrased the part about type-safety - mentioned that it's the reason for difference in performance between prepared and unprepared statements (1RTT vs 2RTTs)
  • for now, decided to stick the old naming, i.e. simple query. This will be changed in other PR.

@muzarski muzarski requested review from Lorak-mmk and wprzytula May 6, 2024 13:21
@wprzytula wprzytula added the waiting-on-author Waiting for a response from issue/PR author label May 6, 2024
@muzarski muzarski force-pushed the docstrings_for_cached_metadata branch from 0530445 to 10556e3 Compare May 6, 2024 14:02
@muzarski muzarski requested a review from wprzytula May 6, 2024 14:03
@mykaul mykaul added the area/documentation Improvements or additions to documentation label May 13, 2024
@wprzytula wprzytula removed the waiting-on-author Waiting for a response from issue/PR author label May 14, 2024
@wprzytula wprzytula merged commit 1d78cbe into scylladb:main May 14, 2024
12 checks passed
@muzarski muzarski deleted the docstrings_for_cached_metadata branch October 29, 2024 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants