Skip to content

ser: use rust-driver's builtin serialization for simple types #152

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

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

muzarski
Copy link
Collaborator

Previously, we would reimplement a serialization logic. It was a copy from rust driver, with the typechecks excluded.

In this PR, we get rid of the repeated code. To skip the typechecks (or rather, to make the typechecks always pass), we simply provide a valid ColumnType to SerializeValue::serialize().

This is a kind of hack, but thanks to that we don't have to reimplement serialization logic.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • PR description sums up the changes and reasons why they should be introduced.
  • [ ] I have enabled appropriate tests in .github/workflows/build.yml in gtest_filter.
  • [ ] I have enabled appropriate tests in .github/workflows/cassandra.yml in gtest_filter.

Previously, we would reimplement a serialization logic. It was a copy
from rust driver, with the typechecks excluded.

In this commit, we get rid of the repeated code. To skip the
typechecks (or rather, to make the typechecks always pass), we simply
provide a valid ColumnType to SerializeValue::serialize().

This is a kind of hack, but thanks to that we don't have to reimplement
serialization logic.
@muzarski muzarski requested review from Lorak-mmk and wprzytula July 31, 2024 13:44
@muzarski muzarski self-assigned this Jul 31, 2024
@muzarski muzarski changed the title ser: use rust builtin serialization for simple types ser: use rust-driver's builtin serialization for simple types Jul 31, 2024
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.

Isn't it possible to remove CassCqlValue entirely?

@muzarski
Copy link
Collaborator Author

Isn't it possible to remove CassCqlValue entirely?

I think it is possible. The main obstacle are udts, collections and tuples. We would need to construct a valid ColumnType for each of these, and a nested ColumnType may be expensive to construct (some allocations). I think that there is no other way to use a builtin rust-driver's serialization with typechecks skipped for these types.

@Lorak-mmk
Copy link
Collaborator

Ok, let's keep the approach from this PR

@muzarski
Copy link
Collaborator Author

Actually, I think we could simply have a wrapper over CqlValue.

@muzarski
Copy link
Collaborator Author

Actually, I think we could simply have a wrapper over CqlValue.

Give me a second, I'll investigate it and update this PR in case it's possible.

@muzarski
Copy link
Collaborator Author

muzarski commented Jul 31, 2024

Nvm, I don't think there is much sense to introduce a wrapper over CqlValue, since we would need to match variants that can never be created via cpp-rust-driver API (e.g. there is no way to create a CqlValue::Counter() value).

I think that we can merge it as is.

@muzarski muzarski merged commit c7bb334 into scylladb:master Jul 31, 2024
5 checks passed
@muzarski muzarski mentioned this pull request Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants