cargo: update rust driver's version to 0.13.1 #138

muzarski · 2024-07-08T17:35:03Z

This PR updates the rust driver's version to 0.13.1

API breaking changes

First commit fixes some minor issues caused by API breaking changes in rust driver.

Serialization

Following commits focus on serialization. This version of rust driver already uses a new serialization framework which validates the types. In result, some integration tests are failing, since cpp-driver uses a bit different rust-to-CQL type mapping. A simple example is a i64 value which can be mapped to 4 CQL types: bigint/counter/time/timestamp.

In this PR we address and fix this issue. We introduce a CassCqlValue enum which is a narrower version of rust driver's CqlValue with a custom SerializeValue (still called SerializeCql in 0.13) implementation. The serialization DOES NOT contain any typechecks. The typechecks will be introduced in a follow-up PR. We want to follow the same approach as cpp-driver, and do the typechecks during bindings (i.e. binding value to collection, tuple, UDT or statement).

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.~~

muzarski · 2024-07-08T18:08:28Z

@Lorak-mmk @wprzytula Any ideas why the unit tests using scylla-proxy fail? In both tests, cass_session_execute_batch seems to return CASS_ERROR_LIB_UNABLE_TO_CONNECT for some weird reason.

wprzytula · 2024-07-08T18:33:13Z

For some reason, I could not add a driver's dependency with a standard version = "0.13" notation. I had to use a VCS path to a specific commit. The reason is that, otherwise, rust compiler would complain about scylla-rust-wrapper and scylla-proxy using different versions of DbError enum - even though, I changed the version of scylla-proxy, and it points to the commit of 0.13 release (scylladb/scylla-rust-driver@e68adf5).

My idea why this happens is: if you choose a specific commit for scylla-proxy, then scylla-proxy (together with scylla-cql it depends on) is fetched from GH. Choosing version for scylla instead of a commit, makes scylla (together with scylla-cql it depends on) come from crates.io, this way making those DB errors different (coming from two different source code definitions - one in GH scylla-cql and another in crates.io scylla-cql).

wprzytula · 2024-07-08T19:00:21Z

@Lorak-mmk @wprzytula Any ideas why the unit tests using scylla-proxy fail? In both tests, cass_session_execute_batch seems to return CASS_ERROR_LIB_UNABLE_TO_CONNECT for some weird reason.

The culprit is this: scylladb/scylla-rust-driver#770.
Logs show (I recommend running one of those failing tests in two terminals side by side, one before the commit that updates the driver and one after) that the updated driver, having failed the initial metadata fetch, starts the re-resolution procedure. This leads to the problem. Further investigation TODO.

wprzytula · 2024-07-08T19:33:34Z

OK, a solution is to alter the proxy rules in the test code not to drop the control connection, but instead return a DB error on attempt to fetch metadata. This way the driver does not suffer from lack of connections in the pool.

Just change this function in a following way:

    fn generic_drop_queries_rules() -> impl IntoIterator<Item = RequestRule> {
        [RequestRule(
            Condition::RequestOpcode(RequestOpcode::Query),
            // We won't respond to any queries (including metadata fetch),
            // but the driver will manage to continue with dummy metadata.
            // RequestReaction::drop_connection(), <--- this is removed
            RequestReaction::forge().server_error(), // <---- this is added instead
        )]
    }

scylla-rust-wrapper/src/user_type.rs

scylla-rust-wrapper/src/tuple.rs

scylla-rust-wrapper/src/user_type.rs

scylla-rust-wrapper/src/value.rs

wprzytula · 2024-07-09T05:29:30Z

scylla-rust-wrapper/src/value.rs

+            CassCqlValue::Tuple(t) => {
+                // We allow serializing tuples that have less fields
+                // than the database tuple, but not the other way around.


Why so? Source? cpp-driver or Rust driver?

This is a logic from rust-driver. I'll check how cpp-driver behaves in this case.

equals implementation for Tuples in cpp-driver:

virtual bool equals(const DataType::ConstPtr& data_type) const { assert(value_type() == CASS_VALUE_TYPE_TUPLE); if (value_type() != data_type->value_type()) { return false; } const ConstPtr& tuple_type(data_type); // Only compare sub-types if both have sub-types if (!types_.empty() && !tuple_type->types_.empty()) { if (types_.size() != tuple_type->types_.size()) { return false; } for (size_t i = 0; i < types_.size(); ++i) { if (!types_[i]->equals(tuple_type->types_[i])) { return false; } } } return true; }

So we should not allow serializing tuples that have less fields than the DB tuple. However, notice that this check is performed during cass_statement_bind_tuple*. It is something that Karol mentioned in other comment. There are no checks during serialization itself.

As a side note: equals implementation for UDTs is a bit more flexible in cpp-driver:

virtual bool equals(const DataType::ConstPtr& data_type) const { assert(value_type() == CASS_VALUE_TYPE_UDT); if (data_type->value_type() != CASS_VALUE_TYPE_UDT) { return false; } const UserType::ConstPtr& user_type(data_type); if (!equals_both_not_empty(keyspace_, user_type->keyspace_)) { return false; } if (!equals_both_not_empty(type_name_, user_type->type_name_)) { return false; } // UDT's can be considered equal as long as the mutual first fields shared // between them are equal. UDT's are append only as far as fields go, so a // newer 'version' of the UDT data type after a schema change event should be // treated as equivalent in this scenario, by simply looking at the first N // mutual fields they should share. size_t min_fields_size = std::min(fields_.size(), user_type->fields_.size()); for (size_t i = 0; i < min_fields_size; ++i) { if (fields_[i].name != user_type->fields_[i].name || !fields_[i].type->equals(user_type->fields_[i].type)) { return false; } } return true; }

Regarding the tuples: as we discussed yesterday @muzarski types and sizes are only checked for typed tuple (cass_tuple_new_from_data_type), not for untyped tuple (cass_tuple_new)

Please make sure that semantics of tuples and UDTs match those in cpp-driver

scylla-rust-wrapper/src/value.rs

muzarski · 2024-07-09T10:18:01Z

One integration test failed:

/home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/objects/future.hpp:152: Failure
      Expected: CASS_OK
To be equal to: wait_code
      Which is: CASS_ERROR_SERVER_INVALID_QUERY [Invalid query]
Invalid query: Database returned an error: The query is syntactically correct but invalid, Error message: Cannot include a counter statement in a logged batch
/home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/tests/test_batch.cpp:316: Failure
      Expected: number_of_rows
      Which is: 100
To be equal to: result.row_count()
      Which is: 0
[  FAILED  ] BatchCounterThreeNodeClusterTests.Integration_Cassandra_MixedPreparedAndSimple (13988 ms)

Isn't it weird that we get a server error that never occured before, after bumping the driver's version?
I'll try to investigate it further.

muzarski · 2024-07-09T11:30:32Z

One integration test failed:

/home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/objects/future.hpp:152: Failure
      Expected: CASS_OK
To be equal to: wait_code
      Which is: CASS_ERROR_SERVER_INVALID_QUERY [Invalid query]
Invalid query: Database returned an error: The query is syntactically correct but invalid, Error message: Cannot include a counter statement in a logged batch
/home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/tests/test_batch.cpp:316: Failure
      Expected: number_of_rows
      Which is: 100
To be equal to: result.row_count()
      Which is: 0
[  FAILED  ] BatchCounterThreeNodeClusterTests.Integration_Cassandra_MixedPreparedAndSimple (13988 ms)

Isn't it weird that we get a server error that never occured before, after bumping the driver's version? I'll try to investigate it further.

The issue can be reproduced in rust driver:

#[tokio::test]
async fn test_counter_batch() {
    setup_tracing();
    let session = Arc::new(create_new_session_builder().build().await.unwrap());
    let ks = unique_keyspace_name();

    session.query(format!("CREATE KEYSPACE IF NOT EXISTS {} WITH REPLICATION = {{'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1}}", ks), &[]).await.unwrap();
    session
        .query(
            format!(
                "CREATE TABLE IF NOT EXISTS {}.t_batch (key int PRIMARY KEY, value counter)",
                ks
            ),
            &[],
        )
        .await
        .unwrap();

    let q = format!("UPDATE {}.t_batch SET value = value + ? WHERE key = ?", ks);

    let mut my_batch = Batch::new(BatchType::Counter);
    assert_eq!(BatchType::Counter, my_batch.get_type());
    my_batch.append_statement(&q[..]);
    my_batch.append_statement(&q[..]);
    my_batch.append_statement(&q[..]);

    session
        .batch(
            &my_batch,
            ((Counter(1), 2), (Counter(1), 2), (Counter(1), 2)),
        )
        .await
        .unwrap();
}

The error:

---- transport::session_test::test_my_batch stdout ----
Unique name: test_rust_1720524333_0
thread 'transport::session_test::test_counter_batch' panicked at scylla/src/transport/session_test.rs:404:10:
called `Result::unwrap()` on an `Err` value: DbError(Invalid, "Cannot include a counter statement in a logged batch")

This seems like a rust-driver issue, since you can clearly see that I've set the BatchType to BatchType::Counter. Previous version of the driver does not have this issue.

cc: @Lorak-mmk @wprzytula

wprzytula · 2024-07-09T11:35:27Z

One integration test failed:

/home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/objects/future.hpp:152: Failure
      Expected: CASS_OK
To be equal to: wait_code
      Which is: CASS_ERROR_SERVER_INVALID_QUERY [Invalid query]
Invalid query: Database returned an error: The query is syntactically correct but invalid, Error message: Cannot include a counter statement in a logged batch
/home/runner/work/cpp-rust-driver/cpp-rust-driver/tests/src/integration/tests/test_batch.cpp:316: Failure
      Expected: number_of_rows
      Which is: 100
To be equal to: result.row_count()
      Which is: 0
[  FAILED  ] BatchCounterThreeNodeClusterTests.Integration_Cassandra_MixedPreparedAndSimple (13988 ms)

Isn't it weird that we get a server error that never occured before, after bumping the driver's version? I'll try to investigate it further.

The issue can be reproduced in rust driver:

#[tokio::test]
async fn test_counter_batch() {
    setup_tracing();
    let session = Arc::new(create_new_session_builder().build().await.unwrap());
    let ks = unique_keyspace_name();

    session.query(format!("CREATE KEYSPACE IF NOT EXISTS {} WITH REPLICATION = {{'class' : 'NetworkTopologyStrategy', 'replication_factor' : 1}}", ks), &[]).await.unwrap();
    session
        .query(
            format!(
                "CREATE TABLE IF NOT EXISTS {}.t_batch (key int PRIMARY KEY, value counter)",
                ks
            ),
            &[],
        )
        .await
        .unwrap();

    let q = format!("UPDATE {}.t_batch SET value = value + ? WHERE key = ?", ks);

    let mut my_batch = Batch::new(BatchType::Counter);
    assert_eq!(BatchType::Counter, my_batch.get_type());
    my_batch.append_statement(&q[..]);
    my_batch.append_statement(&q[..]);
    my_batch.append_statement(&q[..]);

    session
        .batch(
            &my_batch,
            ((Counter(1), 2), (Counter(1), 2), (Counter(1), 2)),
        )
        .await
        .unwrap();
}

The error:

---- transport::session_test::test_my_batch stdout ----
Unique name: test_rust_1720524333_0
thread 'transport::session_test::test_counter_batch' panicked at scylla/src/transport/session_test.rs:404:10:
called `Result::unwrap()` on an `Err` value: DbError(Invalid, "Cannot include a counter statement in a logged batch")

This seems like a rust-driver issue, since you can clearly see that I've set the BatchType to BatchType::Counter. Previous version of the driver does not have this issue.

cc: @Lorak-mmk @wprzytula

Please open an issue in Rust driver repo. Also, specify what version of the driver behaves correctly and what version misbehaves. If you feel like, perform git bisect to find the culprit.

muzarski · 2024-07-10T15:50:17Z

@wprzytula I opened a PR in rust-driver that will fix the failing test: scylladb/scylla-rust-driver#1038. I think that we should filter out the failing test for now, and include it again when new release of rust-driver with a fix is out (unless we can wait for new release if it happens anytime soon, but I don't think so).

Lorak-mmk

There is a is_compatible_type function in binding module. Is it really necessary, when we already do a type checking in our SerializeCql implementation? Can this be removed?

Rust Driver's type checking happens when we serialize the values, which in cpp-rust-driver happens during a call to e.g. cass_session_execute.
is_compatible_type is called in binding functions like cass_statement_bind_*.

We have to either implement it at some point, or wait for Rust Driver to introduce BoundStatement and use it in cpp-rust-driver.

For some reason, I could not add a driver's dependency with a standard version = "0.13" notation. I had to use a VCS path to a specific commit. The reason is that, otherwise, rust compiler would complain about scylla-rust-wrapper and scylla-proxy using different versions of DbError enum - even though, I changed the version of scylla-proxy, and it points to the commit of 0.13 release (scylladb/scylla-rust-driver@e68adf5).

Can you use a tag when specifying dependency, instead of commit hash? Main disadvantage of specifying commit hash is that it's not obvious which version it follows, but tag would solve it.

scylla-rust-wrapper/src/value.rs

muzarski · 2024-07-11T12:35:34Z

v2: addressed review comments

muzarski · 2024-07-11T17:15:53Z

v2.1: bumped rust driver's version to 0.13.1

Co-authored-by: Karol Baryła <[email protected]>

Lorak-mmk · 2024-07-12T15:43:29Z

scylla-rust-wrapper/src/value.rs

+
+fn mk_typck_err<T>(
+    got: &ColumnType,
+    kind: impl Into<BuiltinTypeCheckErrorKind>,
+) -> SerializationError {
+    mk_typck_err_named(std::any::type_name::<T>(), got, kind)
+}
+


Please add a TODO (or even better: issue here and in rust driver) to export relevant functions from scylla-cql so there is not need for coppy-paste here.

I suggest exporting such internal utility functions shared between Rust driver and cpp-rust-driver using #[doc(hidden)] attribute, as this gives us freedom to modify this as non-public API.

Or we could add a feature cpp-rust-wrapper and only export if it is enabled.

Lorak-mmk · 2024-07-12T15:55:54Z

scylla-rust-wrapper/src/value.rs

+            CassCqlValue::Tuple(t) => {
+                // We allow serializing tuples that have less fields
+                // than the database tuple, but not the other way around.


Please make sure that semantics of tuples and UDTs match those in cpp-driver

Proxy would drop connection during metadata fetch which causes some tests to fail.

muzarski · 2024-07-16T19:30:10Z

v3: as discussed with @Lorak-mmk offline, we decided to follow the same approach as cpp-driver - we don't do any typechecks during serialization. The type checks are supposed to take place during binding (e.g. binding value to tuple or statement). This approach is better, since we can separate typecheck and serialization logic - this is really handful for untyped tuples and collections (cpp-driver allows users to define such objects).

In this PR, we only introduce a naive serialization logic (custom SerializeValue implementation for CassCqlValue). During serialization step, we assume that type checks were already done earlier (as of now, this is obviously not true - typecheck logic will be implemented in a follow-up PR).

It's worth noting that we do not lose anything, since type checks were not performed by previous version of rust driver anyway.

muzarski · 2024-07-16T19:45:51Z

Opened an issue on what should be implemented in follow-up PR with typechecks: #142.

muzarski requested review from wprzytula and Lorak-mmk July 8, 2024 17:35

muzarski force-pushed the update-013 branch from 0d5f7d2 to f84901a Compare July 8, 2024 17:40

muzarski self-assigned this Jul 8, 2024

wprzytula reviewed Jul 9, 2024

View reviewed changes

muzarski mentioned this pull request Jul 9, 2024

batch: counter batches are not working properly scylladb/scylla-rust-driver#1035

Closed

Lorak-mmk self-assigned this Jul 9, 2024

Lorak-mmk requested changes Jul 11, 2024

View reviewed changes

scylla-rust-wrapper/src/value.rs Outdated Show resolved Hide resolved

muzarski force-pushed the update-013 branch 2 times, most recently from 7bd9775 to ae38fe8 Compare July 11, 2024 12:35

muzarski force-pushed the update-013 branch from ae38fe8 to 5d133ac Compare July 11, 2024 17:15

muzarski changed the title ~~cargo: update rust driver's version to 0.13~~ cargo: update rust driver's version to 0.13.1 Jul 11, 2024

cargo: update rust driver to 0.13.1

e483a34

Co-authored-by: Karol Baryła <[email protected]>

muzarski force-pushed the update-013 branch from 5d133ac to ec6f1d9 Compare July 11, 2024 17:21

muzarski requested a review from Lorak-mmk July 12, 2024 08:05

Lorak-mmk requested changes Jul 12, 2024

View reviewed changes

muzarski added 4 commits July 16, 2024 15:15

value: introduce CassCqlValue

e95a154

CassUserType: set_option_* => set_field_*

bb4f878

value: prepare ser util functions

922c697

ser: implement serialization for simple types

11e35cb

muzarski added 5 commits July 16, 2024 21:09

ser: Tuple

bff2ae8

ser: list and set

65e59dc

ser: map

18109f6

ser: udt

c704a0b

fix: proxy tests

b0aa444

Proxy would drop connection during metadata fetch which causes some tests to fail.

muzarski force-pushed the update-013 branch from ec6f1d9 to b0aa444 Compare July 16, 2024 19:15

muzarski requested a review from Lorak-mmk July 16, 2024 19:30

Lorak-mmk approved these changes Jul 17, 2024

View reviewed changes

muzarski merged commit e02cad5 into scylladb:master Jul 18, 2024
5 checks passed

muzarski mentioned this pull request Oct 3, 2024

bump version to 0.2.0 #184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo: update rust driver's version to 0.13.1 #138

cargo: update rust driver's version to 0.13.1 #138

muzarski commented Jul 8, 2024 •

edited

Loading

muzarski commented Jul 8, 2024

wprzytula commented Jul 8, 2024

wprzytula commented Jul 8, 2024

wprzytula commented Jul 8, 2024

wprzytula Jul 9, 2024

muzarski Jul 11, 2024

muzarski Jul 11, 2024

muzarski Jul 11, 2024

Lorak-mmk Jul 12, 2024

Lorak-mmk Jul 12, 2024

muzarski commented Jul 9, 2024

muzarski commented Jul 9, 2024

wprzytula commented Jul 9, 2024

muzarski commented Jul 10, 2024

Lorak-mmk left a comment

muzarski commented Jul 11, 2024

muzarski commented Jul 11, 2024

Lorak-mmk Jul 12, 2024

wprzytula Jul 12, 2024

Lorak-mmk Jul 12, 2024

Lorak-mmk Jul 12, 2024

muzarski commented Jul 16, 2024

muzarski commented Jul 16, 2024

cargo: update rust driver's version to 0.13.1 #138

cargo: update rust driver's version to 0.13.1 #138

Conversation

muzarski commented Jul 8, 2024 • edited Loading

API breaking changes

Serialization

Pre-review checklist

muzarski commented Jul 8, 2024

wprzytula commented Jul 8, 2024

wprzytula commented Jul 8, 2024

wprzytula commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Jul 9, 2024

muzarski commented Jul 9, 2024

wprzytula commented Jul 9, 2024

muzarski commented Jul 10, 2024

Lorak-mmk left a comment

Choose a reason for hiding this comment

muzarski commented Jul 11, 2024

muzarski commented Jul 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muzarski commented Jul 16, 2024

muzarski commented Jul 16, 2024

muzarski commented Jul 8, 2024 •

edited

Loading