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

frame/result: allow differing TableSpecs in PreparedMetadata #1135

Merged
merged 3 commits into from
Dec 5, 2024
Merged

frame/result: allow differing TableSpecs in PreparedMetadata #1135

merged 3 commits into from
Dec 5, 2024

Conversation

wprzytula
Copy link
Collaborator

@wprzytula wprzytula commented Dec 2, 2024 •
edited
Loading

As noted in #1134, when preparing batches containing requests to multiple to different tables, the PreparedMetadata received upon preparation contains differing TableSpecs (i.e., TableSpecs with more than table mentioned).
This fails the check that we introduced with ResultMetadata in mind: we've been assuming that all TableSpecs are the same, because ScyllaDB/ Cassandra has no support for JOINs.
Not to fail preparation of cross-table batches, the check is now only turned on for ResultMetadata and turned off for PreparedMetadata.

Fixes: #1134

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 wprzytula added the bug Something isn't working label Dec 2, 2024
@wprzytula wprzytula self-assigned this Dec 2, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 2, 2024 •
edited
Loading

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

muzarski
muzarski reviewed Dec 2, 2024
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.

Looks good, but can we add a regression test for the scenario mentioned in issue (i.e., where we prepare a batch statement with bind markers)?

@wprzytula wprzytula force-pushed the fix-batches-to-different-tables branch from 6b72adb to eade6b6 Compare December 2, 2024 14:14
@wprzytula
Copy link
Collaborator Author

Rebased on main.

muzarski
muzarski approved these changes Dec 2, 2024
View reviewed changes
scylla/src/statement/batch.rs Outdated Show resolved Hide resolved
scylla/src/transport/session_test.rs Outdated Show resolved Hide resolved
Lorak-mmk
Lorak-mmk requested changes Dec 3, 2024
View reviewed changes
Copy link
Collaborator

@Lorak-mmk Lorak-mmk left a comment

Choose a reason for hiding this comment

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

Now that the metadata supports zero-copy deserialization, what is the benefit of having this check at all? If the only benefit is that some objects are a bit smaller, then this is imo not enough to justify additional complexity.
Diverging from CQL protocol have just hurt us, so we should avoid letting it hurt us again in the future.

@wprzytula
Copy link
Collaborator Author

Now that the metadata supports zero-copy deserialization, what is the benefit of having this check at all? If the only benefit is that some objects are a bit smaller, then this is imo not enough to justify additional complexity. Diverging from CQL protocol have just hurt us, so we should avoid letting it hurt us again in the future.

OK, I'll remove the check at all.

@Lorak-mmk
Copy link
Collaborator

Ok. If there are other considerations here (e.g. simpler API) then it may be worth keeping the check. So the question remains - is it only a performance opt or something more?

@wprzytula
Copy link
Collaborator Author

Ok. If there are other considerations here (e.g. simpler API) then it may be worth keeping the check. So the question remains - is it only a performance opt or something more?

What is the "it" that you are referring to? What do you consider to be a performance optimisation? Because the check itself is for sure not an optimisation.

@wprzytula wprzytula force-pushed the fix-batches-to-different-tables branch from 2100859 to 0dc7867 Compare December 4, 2024 08:50
@wprzytula
Copy link
Collaborator Author

v2:

  • addressed @muzarski's comments,
  • deleted the check (and the corresponding error variant) completely. All things considered, it's no use having it, especially once we established that we are not going to decouple TableSpec from ColumnSpec. We also no longer have strong motivation for it, because the ResultMetadata is now always deserialized in a borrowed form, which is much more lightweight.

A step that should actually be performed in that regard is pushing scylladb/scylladb#17788 forwards, because the current way it's done is a dramatic waste.

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 4, 2024
@Lorak-mmk
Copy link
Collaborator

Ok. If there are other considerations here (e.g. simpler API) then it may be worth keeping the check. So the question remains - is it only a performance opt or something more?

What is the "it" that you are referring to? What do you consider to be a performance optimisation? Because the check itself is for sure not an optimisation.

Sorry, I was writing the comment quickly and hastily.
The thing is I don't remember all the discussion and API changes around this.

What I meant is that if assuming the same table spec for columns gives us some API advantages, like being able to expose a table spec directly on query response (compared to expising it on the column metadata of the response), then it may be worth considering.

Getting rid of the assumption also lets us get rid of some types (like ColumnSpecView / ColumnSpecs), right? Then imo it is argument for that because it removes and simplifies code.

@wprzytula
Copy link
Collaborator Author

wprzytula commented Dec 4, 2024 •
edited
Loading

What I like about ColumnSpecs is that it contains logic for get_by_name retrieval, which is absent on plain &[ColumnSpec]. I'd like to keep ColumnSpecs as such in our codebase. We could indeed remove TableSpecView and ColumnSpecView, and also provide an inner method for accessing &[ColumnSpec] from ColumnSpecs.

cc @muzarski

@Lorak-mmk
Copy link
Collaborator

What I like about ColumnSpecs is that it contains logic for get_by_name retrieval, which is absent on plain &[ColumnSpec]. I'd like to keep ColumnSpecs as such in our codebase. We could indeed remove TableSpecView and ColumnSpecView, and also provide an inner method for accessing &[ColumnSpec] from ColumnSpecs.

cc @muzarski

Ok, makes sense.

@wprzytula
Copy link
Collaborator Author

wprzytula commented Dec 4, 2024 •
edited
Loading

This is API-breaking due to an error variant being removed. I suggest leaving the error variant for now under a TODO and removing it in the next major release. FYI @Lorak-mmk

@Lorak-mmk
Copy link
Collaborator

Yes, lets do that so we can release 0.15.1

@wprzytula wprzytula force-pushed the fix-batches-to-different-tables branch from 0dc7867 to e5d3f8c Compare December 5, 2024 13:47
@wprzytula
Copy link
Collaborator Author

Rebased on main.

@wprzytula wprzytula force-pushed the fix-batches-to-different-tables branch from e5d3f8c to 26485f8 Compare December 5, 2024 13:49
@wprzytula
Copy link
Collaborator Author

This is API-breaking due to an error variant being removed. I suggest leaving the error variant for now under a TODO and removing it in the next major release. FYI @Lorak-mmk

Done.

@github-actions github-actions bot removed the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 5, 2024
Lorak-mmk
Lorak-mmk previously approved these changes Dec 5, 2024
View reviewed changes
@muzarski
Copy link
Contributor

muzarski commented Dec 5, 2024

Actually - one more nit: aren't docstrings/comment related to this function outdated now?

@wprzytula
Copy link
Collaborator Author

Actually - one more nit: aren't docstrings/comment related to this function outdated now?

Yees, good catch!

@wprzytula
frame/result: allow differing TableSpecs
39327a3 
As noted in scylladb#1134, when preparing batches containing requests to
multiple to different tables, the PreparedMetadata received upon
preparation contains differing TableSpecs (i.e., TableSpecs with more
than table mentioned).
This fails the check that we introduced with ResultMetadata in mind:
we've been assuming that all TableSpecs are the same, because ScyllaDB/
Cassandra has no support for JOINs.
Not to fail preparation of cross-table batches, the check is disabled.
We decided that we should not rely on an assumption that is not
guaranteed by the CQL protocol.
@wprzytula wprzytula force-pushed the fix-batches-to-different-tables branch from 26485f8 to c3a16d9 Compare December 5, 2024 14:34
@wprzytula
Copy link
Collaborator Author

Actually - one more nit: aren't docstrings/comment related to this function outdated now?

Fixed.

Lorak-mmk
Lorak-mmk previously approved these changes Dec 5, 2024
View reviewed changes
@wprzytula
frame/result: inline deser_table_spec_for_col_spec
b1517fe 
After equality checks were removed between table specs in consecutive
column specs, deser_table_spec_for_col_spec has now trivial logic that
is concise enough to be inlined into deser_col_specs_generic.
@wprzytula
regression test for batches to multiple tables
693c9db 
The previous commit fixed the bug described in scylladb#1134. This commit adds
a regression test for that particular case, that is, for preparing a
batch that operates on multiple tables.
@wprzytula wprzytula force-pushed the fix-batches-to-different-tables branch from c3a16d9 to 693c9db Compare December 5, 2024 15:09
@wprzytula wprzytula requested a review from muzarski December 5, 2024 15:09
@wprzytula wprzytula merged commit 92fdd71 into scylladb:main Dec 5, 2024
11 checks passed
@wprzytula wprzytula deleted the fix-batches-to-different-tables branch December 5, 2024 15:26
wprzytula added a commit to wprzytula/scylla-rust-driver that referenced this pull request Dec 11, 2024
@wprzytula
Merge pull request scylladb#1135 from wprzytula/fix-batches-to-differ…
0c96ff0 
…ent-tables

frame/result: allow differing TableSpecs in PreparedMetadata

(cherry picked from commit 92fdd71)
@wprzytula wprzytula mentioned this pull request Dec 11, 2024
Merged
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

@Lorak-mmk Lorak-mmk Lorak-mmk approved these changes

Assignees

@wprzytula wprzytula

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TableSpecDiffersAcrossColumns when preparing a batch
3 participants
@wprzytula @Lorak-mmk @muzarski