deser: lazy CassRow construction #213
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
44fd342 to
d0a9ca9
Compare
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
d0a9ca9 to
4dfdf42
Compare
Closed
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
2 times, most recently
from
40cf28e to
da5ce8e
Compare
There was a problem hiding this comment.
PR Overview
This PR updates the FFI interface across multiple modules by replacing raw pointer types with explicit smart pointer wrappers (e.g. CassSharedPtr, CassExclusiveMutPtr, etc.) and updating the associated usage of BoxFFI/ArcFFI throughout the codebase. Key changes include:
- Converting function signatures from raw pointers (e.g. *mut CassCluster) to well‐typed smart pointers.
- Updating FFI-related macros and helper calls to use the new pointer types with explicit unwrap calls.
- Adjusting tests and integration functions to use the new pointer conversion and cloning semantics.
Reviewed Changes
| File | Description |
|---|---|
| scylla-rust-wrapper/src/cluster.rs | Updated pointer types and conversion logic in cluster setup functions. |
| scylla-rust-wrapper/src/prepared.rs | Converted prepared statement FFI functions to use smart pointers. |
| scylla-rust-wrapper/src/exec_profile.rs | Updated execution profile functions to work with new pointer wrappers. |
| ... (other files changed) | Similar changes applied for future, binding, logging, metadata, batch, statement, query_error, ssl, retry_policy, integration_testing, cass_error, and session modules. |
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
4 times, most recently
from
73d4b45 to
4950df5
Compare
|
This one is ready for review. #208 needs to be merged first, though. |
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
4950df5 to
4d0f7c5
Compare
Lorak-mmk
requested changes
Apr 1, 2025
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
4d0f7c5 to
e66b4bd
Compare
|
Rebased on master (to include #232) |
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
e66b4bd to
700cf6e
Compare
|
v1.1: Adjusted safety comments and commit messages as @Lorak-mmk suggested. The CI will fail - still waiting for #233 to finish (cmake version bump) |
|
WTF, |
Lorak-mmk
requested changes
Apr 1, 2025
Just introducing the config file is unfortunately not enough. CMakeRust does not seem to pick up the config file. Even if it did (after some adjustments), it defines the RUSTFLAGS env variable which takes precedence over the config file. The value of RUSTFLAGS changes depending on the platform (so we can't just put them in the config file).
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
700cf6e to
d2d910c
Compare
|
v1.2: Rebased on master (cmake version bump) and addressed @Lorak-mmk comments |
wprzytula
requested changes
Apr 1, 2025
|
Regarding the self-reference problem: maybe we could use |
I considered this. I'd need to investigate how easy it would be to implement it. I'm not that familiar with |
It is to distinguish between the two types of iterators that we have: - iterator over non-rows result (always returning null) - iterator over rows result In one of the next commits, we will modify `CassRowsResultIterator`, so it holds `TypedRowIterator` instead of reference to CassRowsResult. This change would be necessary then anyway, so it's better to do it now and reduce the noise during review in later commit.
If we look at the CassResult methods, there is no method that requests a CassRow at given index. It means, that we can lazily deserialize rows during iteration (`cass_iterator_from_result`). The only exception is `cass_result_first_row`. The result should hold first deserialized row, and simply return it. We should not deserialize the row each time this method is called. This is exactly what this commit does. It adjusts both `CassRowsResult` and `CassRowsResultIterator`: - CassRowsResult now holds only first row, and all deserialization payload (i.e. metadata and serialized rows). - CassRowsResultIterator from now on holds a TypedRowIterator, instead of reference to CassRowsResult. This semantic is more efficient, and is employed by cpp-driver as well.
In the later commit, this PR introduces self-borrowing to the codebase. As of now, it's not possible to express it using safe Rust. Luckily, there is a yoke crate that will allow us to do that.
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
d2d910c to
1b4b176
Compare
|
v2.0:
|
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
1b4b176 to
1db2e37
Compare
Lorak-mmk
approved these changes
Apr 2, 2025
wprzytula
requested changes
Apr 2, 2025
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
1db2e37 to
4edfb2e
Compare
|
v2.1: Addressed @wprzytula comments |
wprzytula
requested changes
Apr 2, 2025
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
4edfb2e to
e8e910f
Compare
|
v2.2: Renamed the constructor so it indicates that it constructs first row only. |
Because of that, we need to introduce a lifetime parameter to CassRow. CassRow will from now on borrow metadata from CassResult (CassRowsResult). OTOH, notice that CassRowsResult need to hold the deserialized first row. This is because `cass_result_first_row` method exists. We don't want to deserialize the row each time this method is called. Because of that, the self-borrowing appears in our codebase. We have a CassRowsResult, which holds a CassRow that borrows CassResultMetadata from the CassRowsResult. It's not possible to express it in safe Rust, thus we make use of yoke crate which wraps unsafe code and exposes safe abstractions on top of it.
I extracted the logic for each iterator to corresponding methods.
QueryError was replaced with ExecutionError in Rust driver 1.0.
muzarski
force-pushed
the
bump-cpp-rust-cfg
branch
from
e8e910f to
b6e1f85
Compare
|
v2.3: Reduced visibility of |
wprzytula
approved these changes
Apr 2, 2025
Lorak-mmk
approved these changes
Apr 2, 2025
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Depends on: #208
Motivation
Currently, we unnecessarily construct
CassRows eagerly. What we can do (and what cpp-driver does) is to deserialize rows lazily, during iteration using iterator created viacass_iterator_from_result.Why?
Notice that
CassResulthas no methods that request a row at given index. The only exception iscass_result_first_row- but we can simply deserialize only first row, and cache it inCassResultso the call tocass_result_first_rowis always fast.Changes
CassRowsResult, so now it only holds deserialized first row (instead of all rows), and payload required for deserialization (i.e.DeserializedMetadataAndRawRowsandCassResultMetadata).CassResultIterator, so now it is an enum - to distinguish whether it's an iterator created from either non-rows result or rows result.CassRowsResultIterator,which now usesTypedRowIteratorunder the hood. The rows are now deserialized on the fly, duringcass_iterator_nextcall.CassRow, and newly introducedCassRowsResultIterator, so they now borrowCassResultMetadatafromCassResult(instead of sharing ownership viaArc). Their lifetime is already bound by the lifetime ofCassResult, so there is no point in cloning an Arc.Pre-review checklist
[ ] I have implemented Rust unit tests for the features/changes introduced.[ ] I have enabled appropriate tests in.github/workflows/build.ymlingtest_filter.[ ] I have enabled appropriate tests in.github/workflows/cassandra.ymlingtest_filter.