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

value: Cql[Varint/Decimal]Borrowed #1148

Merged
merged 3 commits into from
Dec 15, 2024

Conversation

muzarski
Copy link
Contributor

@muzarski muzarski commented Dec 11, 2024

This PR introduces borrowed versions of CqlDecimal and CqlVarint. It implements SerializeValue and DeserializeValue for new types.

There is a use case for this in cpp-rust-driver - we need to set the pointer user provided, so it points to the underlying data. After lazy-deserialization refactor in cpp-rust-driver, CqlDecimal is not useful anymore. If we deserialize to CqlDecimal, it owns the bytes and is a stack variable - thus, we cannot set the pointer to its underlying data (it will be freed after function call).

Questions for reviewers:

  • Should I put it under #[cfg(cpp_rust_unstable]? I think it may be useful for other users as well.
  • I only implemente From<Cql[Varint/Decimal]Borrowed> for Big[Int/Decimal]. I didn't implement conversions in other direction, since new types borrow bytes (and From consumes). Do you think it's ok to make an exception here, and implement From<&Big[Int/Decimal]> (from reference)?

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.

I believe this is more readable, and should simplify the diff
from the following commit.
@muzarski muzarski self-assigned this Dec 11, 2024
This is a native representation of varint that holds borrowed bytes.

Implemented SerializeValue and DeserializeValue for new type.
Has the same semantics as CqlDecimal, but borrows the bytes.
@muzarski muzarski force-pushed the cql-varint-decimal-borrowed branch from f3fc875 to 5808f5a Compare December 11, 2024 11:38
@muzarski muzarski added cpp-rust-driver-p0 Functionality required by cpp-rust-driver area/deserialization labels Dec 11, 2024
Copy link

github-actions bot commented Dec 11, 2024

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

@Lorak-mmk
Copy link
Collaborator

Now that I think of it: can we just use Bytes for CqlVarint?

The only downside I see is that it would make the whole frame alive as long as CqlVarint is alive, but it is also the case for CqlVarintBorrowed (as it borrows the frame) - the only difference is that the latter is enforced at compile time, but complicates the code.

This is a tradeoff, and I'm not sure which solution is better.

@muzarski
Copy link
Contributor Author

Now that I think of it: can we just use Bytes for CqlVarint?

The only downside I see is that it would make the whole frame alive as long as CqlVarint is alive, but it is also the case for CqlVarintBorrowed (as it borrows the frame) - the only difference is that the latter is enforced at compile time, but complicates the code.

This is a tradeoff, and I'm not sure which solution is better.

I like this idea. This simplifies code by a lot.

@muzarski muzarski force-pushed the cql-varint-decimal-borrowed branch from 5808f5a to b76399f Compare December 11, 2024 15:21
@muzarski
Copy link
Contributor Author

I applied @Lorak-mmk suggestion - replaced Vec<u8> with bytes::Bytes in CqlVarint.

@github-actions github-actions bot added the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 11, 2024
@muzarski muzarski force-pushed the cql-varint-decimal-borrowed branch from b76399f to 5808f5a Compare December 11, 2024 15:37
@muzarski
Copy link
Contributor Author

Reverted the changes with Bytes. After a discussion, we decided that CqlVarint should not keep whole frame alive (and this is what it does when it holds Bytes).

@github-actions github-actions bot removed the semver-checks-breaking cargo-semver-checks reports that this PR introduces breaking API changes label Dec 11, 2024
@wprzytula wprzytula merged commit 847e44d into scylladb:main Dec 15, 2024
24 checks passed
@muzarski muzarski deleted the cql-varint-decimal-borrowed branch December 19, 2024 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deserialization cpp-rust-driver-p0 Functionality required by cpp-rust-driver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants