Prepared tests #68
Prepared tests #68
Conversation
Gor027
force-pushed
the
prepared_tests
branch
2 times, most recently
from
bba2e67 to
39f80c8
Compare
Gor027
force-pushed
the
prepared_tests
branch
2 times, most recently
from
9729e7b to
f2ccf44
Compare
scylla-rust-wrapper/src/statement.rs
Outdated
| Statement::Simple(inner) => inner.set_timestamp(Some(timeout_ms as i64)), | ||
| Statement::Prepared(inner) => Arc::make_mut(inner).set_timestamp(Some(timeout_ms as i64)), |
There was a problem hiding this comment.
Timestamp is a completely different thing than timeout.
- A CQL timestamp defines the order in which writes are applied to the database.
- Timeout (here, client-side timeout) means the time the driver will wait until response arrives from the database. If no response arrives within the timeout period, the driver is supposed to return an error.
Here, I think you have accidentally implemented cass_statement_set_timestamp.
Currently, the rust driver doesn't support client timeouts directly, but you can implement them yourself in the wrapper code. You need to keep the timeout in the CassStatement somewhere, wrap session.execute() etc. calls with tokio::time::timeout and handle the timeout error returned by the tokio::time::timeout call appropriately.
scylla-rust-wrapper/src/session.rs
Outdated
| let session_opt = ptr_to_ref(cass_session); | ||
| let statement_opt = ptr_to_ref(statement); |
There was a problem hiding this comment.
Why do those names have an _opt suffix? Those aren't Options?
scylla-rust-wrapper/src/session.rs
Outdated
| let query = match statement.clone() { | ||
| Statement::Simple(q) => q.contents, | ||
| Statement::Prepared(ps) => ps.get_statement().to_string(), | ||
| }; |
There was a problem hiding this comment.
By doing statement.clone(), you are cloning the whole statement, but then you only need a small part of it and you have unnecessarily cloned the whole statement.
Perhaps you can match on a reference (&statement) and move .clone() to the first match, like this:
let query = match &statement {
Statement::Simple(q) => q.contents.clone(),
Statement::Prepared(ps) => ps.get_statement().to_string(),
};
scylla-rust-wrapper/src/session.rs
Outdated
|
|
||
| let query = match statement.clone() { | ||
| Statement::Simple(q) => q.contents, | ||
| Statement::Prepared(ps) => ps.get_statement().to_string(), |
There was a problem hiding this comment.
I don't think it makes sense at all to prepare a prepared statement again. I think in this case it makes sense to just clone a prepared statement, put it into a box and immediately return it.
scylla-rust-wrapper/src/session.rs
Outdated
| } | ||
| let session = session_guard.as_ref().unwrap(); | ||
| let mut prepared = session | ||
| .prepare(query) |
There was a problem hiding this comment.
Instead of passing a string, you can put a Query here. AFAIK the resulting PreparedStatement should inherit all properties of the original Query, so setting them manually later shouldn't be necessary.
scylla-rust-wrapper/src/session.rs
Outdated
| @@ -134,6 +136,16 @@ fn create_cass_row_columns(row: Row, metadata: &Arc<CassResultData>) -> Vec<Cass | |||
| .collect() | |||
| } | |||
|
|
|||
| fn create_col_index_mapping(col_specs: &[ColumnSpec]) -> HashMap<String, usize> { | |||
There was a problem hiding this comment.
I think that constructing a hashmap to speed up lookup by name is an overkill. The number of columns is usually small, so linear lookup should be sufficient. Creating a hashmap allocates (at least one allocation for the hashmap itself + one allocation for each String), so it would be best to avoid it.
Gor027
force-pushed
the
prepared_tests
branch
2 times, most recently
from
0fd11f3 to
f1e1cec
Compare
scylla-rust-wrapper/src/statement.rs
Outdated
| @@ -25,6 +27,7 @@ pub struct CassStatement { | |||
| pub statement: Statement, | |||
| pub bound_values: Vec<MaybeUnset<Option<CqlValue>>>, | |||
| pub paging_state: Option<Bytes>, | |||
| pub request_timeout_ms: cass_uint64_t, | |||
There was a problem hiding this comment.
Maybe replace it with Option<cass_uint64_t> - if option is None then apply no timeout, instead of hardcoding 2 year timeout.
Add implementation for: * cass_statement_set_serial_consistency * cass_statement_set_consistency * cass_statement_set_timestamp * cass_statement_set_request_timout
Added retry policy definition: * constructor for Default retry policy: cass_retry_policy_default_new * destructor for CassRetryPolicy: cass_retry_policy_free Added implementation of cass_row_get_column_by_name. Modified PrepareFromExistingSimpleStatement and PrepareFromExistingBoundStatement tests to prepare statements with Default retry policy instead of DowngradingConsistency as the latter is deprecated and is not supported by rust driver. Statement setters validations are commented out in tests as their corresponding implementations in rust bindings are not present(src/testing.cpp). In FailFastWhenPreparedIDChangesDuringReprepare test, the result's error message is changed according to the error message the rust driver returns. PreparedIDUnchangedDuringReprepare test is ignored, as it is also ignored in cpp-driver.
Pre-review checklist
.github/workflows/build.yamlingtest_filter.This PR is dependent upon #65
Adds necessary implementations to pass all
PreparedTestsbesidesPreparedIDUnchangedDuringReprepare, which is also ignored in the C++ driver.Certain validations in tests are commented out for now, as their corresponding implementations in Rust are not yet present.
Also,
DowngradingConsistencyretry policies in those tests are changed to beDefaultretry consistency, asDowngradingConsistencyis deprecated and not supported by the Rust driver.