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

Introduce BoundStatement #941

Open
Lorak-mmk opened this issue Feb 27, 2024 · 1 comment
Open

Introduce BoundStatement #941

Lorak-mmk opened this issue Feb 27, 2024 · 1 comment
Labels
enhancement New feature or request
Milestone

Comments

@Lorak-mmk
Copy link
Collaborator

After releasing serialization refactor several users reported use cases that are not well-supported by our new APIs.
The problems come from the need to have specific (non-generic) owned type for query values.
Previously SerializedValues was such a type. You could use it to serialize values ahead of time and then pass it around as you wish.
After refactor you would need to use types like Vec<Box<dyn SerializeCql>> or Box<dyn SerializeRow>, depending on your use case. There are some disadvantages

  • Additional allocations.
  • Need the values type to be owned - which is a problem if they come from e.g. zero-copy deserialization because you need to clone them.
  • If you need specific values to also be dynamic or add them one-by-one, then you need to use something like Vec<Box<dyn SerializeCql>> and put that in a box - that's a lot more allocations and indirections.

So in short: migration to new API is possible in every case (I think), but sometimes the performance cost is high.

I propose to introduce new query type (like Query and PreparedStatement): BoundStatement. This is a prepared statement that also stores serialized values.
This would allow users to serialize parameters ahead of time while remaining type-safe and avoiding unnecessary allocations.

My first idea for an API is a a bind(&self, values: impl SerializeRow) -> BoundStatement method on PreparedStatement.
We can also think about allowing to bind values one by one. This would complicate the API and implementation and introduce some runtime errors (e.g. if user first binds value by order and then by name), but would be more flexible.

Main problem I see is how to adapt our Session API to this - creating another set of methods, like for Query and PreparedStatement seems bloaty.
There are more reasons to work on our Session API (potentially removing Query with values, bad naming etc), so I think we should discuss Session API changes in #713
Right now probably the most reasonable solution that would allow us to support BoundStatement is Executable trait suggested in this issue.

Relevant issues / discussions:
#890
#713
https://scylladb-users.slack.com/archives/C01D7LCL44D/p1708948081839569

@wprzytula
Copy link
Collaborator

I'm convinced that with the new strongly typed serialization framework we really need BoundStatement for both compatibility and performance purposes.
I believe we should work on it together with exploring possible implementation of Executable trait (#713), so that both those new things are compatible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants