From 235b51972e3838ee59bb6b18dd80a0971687320e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 20 Nov 2024 17:19:45 +0100 Subject: [PATCH 1/8] result: move tracing_id from CassResultData to CassResult CassResultData should represent a result metadata. Tracing id is not a part of it. --- scylla-rust-wrapper/src/future.rs | 2 +- scylla-rust-wrapper/src/query_result.rs | 8 +++----- scylla-rust-wrapper/src/session.rs | 4 ++-- 3 files changed, 6 insertions(+), 8 deletions(-) diff --git a/scylla-rust-wrapper/src/future.rs b/scylla-rust-wrapper/src/future.rs index c579dd5f..34df2b37 100644 --- a/scylla-rust-wrapper/src/future.rs +++ b/scylla-rust-wrapper/src/future.rs @@ -398,7 +398,7 @@ pub unsafe extern "C" fn cass_future_tracing_id( tracing_id: *mut CassUuid, ) -> CassError { ptr_to_ref(future).with_waited_result(|r: &mut CassFutureResult| match r { - Ok(CassResultValue::QueryResult(result)) => match result.metadata.tracing_id { + Ok(CassResultValue::QueryResult(result)) => match result.tracing_id { Some(id) => { *tracing_id = CassUuid::from(id); CassError::CASS_OK diff --git a/scylla-rust-wrapper/src/query_result.rs b/scylla-rust-wrapper/src/query_result.rs index 95487846..d73451eb 100644 --- a/scylla-rust-wrapper/src/query_result.rs +++ b/scylla-rust-wrapper/src/query_result.rs @@ -19,13 +19,13 @@ use uuid::Uuid; pub struct CassResult { pub rows: Option>, pub metadata: Arc, + pub tracing_id: Option, } pub struct CassResultData { pub paging_state_response: PagingStateResponse, pub col_specs: Vec>, pub col_data_types: Arc>>, - pub tracing_id: Option, } impl CassResultData { @@ -33,7 +33,6 @@ impl CassResultData { paging_state_response: PagingStateResponse, col_specs: Vec>, maybe_col_data_types: Option>>>, - tracing_id: Option, ) -> CassResultData { // `maybe_col_data_types` is: // - Some(_) for prepared statements executions @@ -52,7 +51,6 @@ impl CassResultData { paging_state_response, col_specs, col_data_types, - tracing_id, } } } @@ -1425,7 +1423,6 @@ mod tests { ), ], None, - None, )); let rows = create_cass_rows_from_rows( @@ -1446,6 +1443,7 @@ mod tests { CassResult { rows: Some(rows), metadata, + tracing_id: None, } } @@ -1536,11 +1534,11 @@ mod tests { PagingStateResponse::NoMorePages, vec![], None, - None, )); CassResult { rows: None, metadata, + tracing_id: None, } } diff --git a/scylla-rust-wrapper/src/session.rs b/scylla-rust-wrapper/src/session.rs index 51cdca94..174e6174 100644 --- a/scylla-rust-wrapper/src/session.rs +++ b/scylla-rust-wrapper/src/session.rs @@ -224,8 +224,8 @@ pub unsafe extern "C" fn cass_session_execute_batch( PagingStateResponse::NoMorePages, vec![], None, - None, )), + tracing_id: None, }))), Err(err) => Ok(CassResultValue::QueryError(Arc::new(err))), } @@ -361,7 +361,6 @@ pub unsafe extern "C" fn cass_session_execute( paging_state_response, result.col_specs().to_vec(), maybe_col_data_types, - result.tracing_id, )); let cass_rows = result .rows @@ -369,6 +368,7 @@ pub unsafe extern "C" fn cass_session_execute( let cass_result = Arc::new(CassResult { rows: cass_rows, metadata, + tracing_id: result.tracing_id, }); Ok(CassResultValue::QueryResult(cass_result)) From ba499847acb7c6d1c76032c0f0c9421d18a4a5e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 20 Nov 2024 17:30:19 +0100 Subject: [PATCH 2/8] result: move paging_state_response from CassResultData to CassResult Again, paging state response has nothing to do with the metadata needed to deserialize the rows. --- scylla-rust-wrapper/src/query_result.rs | 17 ++++++----------- scylla-rust-wrapper/src/session.rs | 9 +++------ scylla-rust-wrapper/src/statement.rs | 2 +- 3 files changed, 10 insertions(+), 18 deletions(-) diff --git a/scylla-rust-wrapper/src/query_result.rs b/scylla-rust-wrapper/src/query_result.rs index d73451eb..641525f0 100644 --- a/scylla-rust-wrapper/src/query_result.rs +++ b/scylla-rust-wrapper/src/query_result.rs @@ -20,17 +20,16 @@ pub struct CassResult { pub rows: Option>, pub metadata: Arc, pub tracing_id: Option, + pub paging_state_response: PagingStateResponse, } pub struct CassResultData { - pub paging_state_response: PagingStateResponse, pub col_specs: Vec>, pub col_data_types: Arc>>, } impl CassResultData { pub fn from_result_payload( - paging_state_response: PagingStateResponse, col_specs: Vec>, maybe_col_data_types: Option>>>, ) -> CassResultData { @@ -48,7 +47,6 @@ impl CassResultData { }); CassResultData { - paging_state_response, col_specs, col_data_types, } @@ -849,7 +847,7 @@ pub unsafe extern "C" fn cass_result_free(result_raw: *const CassResult) { #[no_mangle] pub unsafe extern "C" fn cass_result_has_more_pages(result: *const CassResult) -> cass_bool_t { let result = ptr_to_ref(result); - (!result.metadata.paging_state_response.finished()) as cass_bool_t + (!result.paging_state_response.finished()) as cass_bool_t } #[no_mangle] @@ -1363,7 +1361,7 @@ pub unsafe extern "C" fn cass_result_paging_state_token( let result_from_raw = ptr_to_ref(result); - match &result_from_raw.metadata.paging_state_response { + match &result_from_raw.paging_state_response { PagingStateResponse::HasMorePages { state } => match state.as_bytes_slice() { Some(result_paging_state) => { *paging_state_size = result_paging_state.len() as u64; @@ -1413,7 +1411,6 @@ mod tests { const THIRD_COLUMN_NAME: &str = "list_double_col"; fn create_cass_rows_result() -> CassResult { let metadata = Arc::new(CassResultData::from_result_payload( - PagingStateResponse::NoMorePages, vec![ col_spec(FIRST_COLUMN_NAME, ColumnType::BigInt), col_spec(SECOND_COLUMN_NAME, ColumnType::Varint), @@ -1444,6 +1441,7 @@ mod tests { rows: Some(rows), metadata, tracing_id: None, + paging_state_response: PagingStateResponse::NoMorePages, } } @@ -1530,15 +1528,12 @@ mod tests { } fn create_non_rows_cass_result() -> CassResult { - let metadata = Arc::new(CassResultData::from_result_payload( - PagingStateResponse::NoMorePages, - vec![], - None, - )); + let metadata = Arc::new(CassResultData::from_result_payload(vec![], None)); CassResult { rows: None, metadata, tracing_id: None, + paging_state_response: PagingStateResponse::NoMorePages, } } diff --git a/scylla-rust-wrapper/src/session.rs b/scylla-rust-wrapper/src/session.rs index 174e6174..96f62710 100644 --- a/scylla-rust-wrapper/src/session.rs +++ b/scylla-rust-wrapper/src/session.rs @@ -220,12 +220,9 @@ pub unsafe extern "C" fn cass_session_execute_batch( match query_res { Ok(_result) => Ok(CassResultValue::QueryResult(Arc::new(CassResult { rows: None, - metadata: Arc::new(CassResultData::from_result_payload( - PagingStateResponse::NoMorePages, - vec![], - None, - )), + metadata: Arc::new(CassResultData::from_result_payload(vec![], None)), tracing_id: None, + paging_state_response: PagingStateResponse::NoMorePages, }))), Err(err) => Ok(CassResultValue::QueryError(Arc::new(err))), } @@ -358,7 +355,6 @@ pub unsafe extern "C" fn cass_session_execute( match query_res { Ok((result, paging_state_response, maybe_col_data_types)) => { let metadata = Arc::new(CassResultData::from_result_payload( - paging_state_response, result.col_specs().to_vec(), maybe_col_data_types, )); @@ -369,6 +365,7 @@ pub unsafe extern "C" fn cass_session_execute( rows: cass_rows, metadata, tracing_id: result.tracing_id, + paging_state_response, }); Ok(CassResultValue::QueryResult(cass_result)) diff --git a/scylla-rust-wrapper/src/statement.rs b/scylla-rust-wrapper/src/statement.rs index b8de7395..22c4f23b 100644 --- a/scylla-rust-wrapper/src/statement.rs +++ b/scylla-rust-wrapper/src/statement.rs @@ -245,7 +245,7 @@ pub unsafe extern "C" fn cass_statement_set_paging_state( let statement = ptr_to_ref_mut(statement); let result = ptr_to_ref(result); - match &result.metadata.paging_state_response { + match &result.paging_state_response { PagingStateResponse::HasMorePages { state } => statement.paging_state.clone_from(state), PagingStateResponse::NoMorePages => statement.paging_state = PagingState::start(), } From 5e5f58514bda151bc3fa1f586cc763b1b3306572 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 20 Nov 2024 18:33:59 +0100 Subject: [PATCH 3/8] tests: disable AlterProperlyUpdatesColumnCount This test should not be enabled! See the description: * Verify that the column count of a bound statement's result metadata is * properly updated for newer protocol versions (v5 and greater) when a table's * schema is altered. This test will not work after next commit, thus it's being disabled. On the other hand, the test AlterDoesntUpdateColumnCount should be enabled. * Verify that the column count of a bound statement's result metadata doesn't * change for older protocol versions (v4 and less) when a table's schema is altered. It will be enabled later in this PR, once following issue is addressed: Currently, the `column_specs` and `col_data_types` may have different sizes for prepared statements. This is because, `col_data_types` is created based on prepared statement's cached metadata, and `column_specs` are taken from the QueryResult. --- Makefile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Makefile b/Makefile index 7e49405c..6afcbdcb 100644 --- a/Makefile +++ b/Makefile @@ -27,6 +27,7 @@ SCYLLA_TEST_FILTER := $(subst ${SPACE},${EMPTY},ClusterTests.*\ :ExecutionProfileTest.InvalidName\ :*NoCompactEnabledConnection\ :PreparedMetadataTests.Integration_Cassandra_AlterDoesntUpdateColumnCount\ +:PreparedMetadataTests.Integration_Cassandra_AlterProperlyUpdatesColumnCount\ :UseKeyspaceCaseSensitiveTests.Integration_Cassandra_ConnectWithKeyspace) endif @@ -57,6 +58,7 @@ CASSANDRA_TEST_FILTER := $(subst ${SPACE},${EMPTY},ClusterTests.*\ :ExecutionProfileTest.InvalidName\ :*NoCompactEnabledConnection\ :PreparedMetadataTests.Integration_Cassandra_AlterDoesntUpdateColumnCount\ +:PreparedMetadataTests.Integration_Cassandra_AlterProperlyUpdatesColumnCount\ :UseKeyspaceCaseSensitiveTests.Integration_Cassandra_ConnectWithKeyspace) endif From 904a971ca13dbeb33faf7e1f41b462d4d59caf3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 20 Nov 2024 17:41:04 +0100 Subject: [PATCH 4/8] result: replace col_specs with col_names Currently, we heavily depend on the fact that `QueryResult::col_specs()` returns a `&[ColumnSpec<'static>]`. This won't be true once we bump rust-driver dependency to 0.15 - the lifetime of ColumnSpec will be tied to the lifetime of `QueryRowsResult` object. It looks like most of the information held by `col_spec` was reduntant. It was only used to retrieve the name of the column. This is why, we will only hold the information about the column names. The column types are stored in `col_data_types`. This also allows us to get rid of `'static` bound from `CassResultData::from_result_payload`. It will be helpful when bumping to 0.15. As a bonus, we can also replace the `Vec<>` with a slice `&[]` in `from_result_payload`, since we do not need to own the column specs anymore. --- scylla-rust-wrapper/src/query_result.rs | 30 ++++++++++++++----------- scylla-rust-wrapper/src/session.rs | 4 ++-- 2 files changed, 19 insertions(+), 15 deletions(-) diff --git a/scylla-rust-wrapper/src/query_result.rs b/scylla-rust-wrapper/src/query_result.rs index 641525f0..a8932d93 100644 --- a/scylla-rust-wrapper/src/query_result.rs +++ b/scylla-rust-wrapper/src/query_result.rs @@ -24,13 +24,13 @@ pub struct CassResult { } pub struct CassResultData { - pub col_specs: Vec>, + pub col_names: Vec, pub col_data_types: Arc>>, } impl CassResultData { pub fn from_result_payload( - col_specs: Vec>, + col_specs: &[ColumnSpec<'_>], maybe_col_data_types: Option>>>, ) -> CassResultData { // `maybe_col_data_types` is: @@ -46,8 +46,13 @@ impl CassResultData { ) }); + let col_names = col_specs + .iter() + .map(|col_spec| col_spec.name().to_owned()) + .collect(); + CassResultData { - col_specs, + col_names, col_data_types, } } @@ -895,12 +900,12 @@ pub unsafe extern "C" fn cass_row_get_column_by_name_n( return row_from_raw .result_metadata - .col_specs + .col_names .iter() .enumerate() - .find(|(_, spec)| { - is_case_sensitive && spec.name() == name_str - || !is_case_sensitive && spec.name().eq_ignore_ascii_case(name_str) + .find(|(_, col_name)| { + is_case_sensitive && *col_name == name_str + || !is_case_sensitive && col_name.eq_ignore_ascii_case(name_str) }) .map(|(index, _)| { return match row_from_raw.columns.get(index) { @@ -921,12 +926,11 @@ pub unsafe extern "C" fn cass_result_column_name( let result_from_raw = ptr_to_ref(result); let index_usize: usize = index.try_into().unwrap(); - if index_usize >= result_from_raw.metadata.col_specs.len() { + if index_usize >= result_from_raw.metadata.col_data_types.len() { return CassError::CASS_ERROR_LIB_INDEX_OUT_OF_BOUNDS; } - let column_spec: &ColumnSpec = result_from_raw.metadata.col_specs.get(index_usize).unwrap(); - let column_name = column_spec.name(); + let column_name = result_from_raw.metadata.col_names.get(index_usize).unwrap(); write_str_to_c(column_name, name, name_length); @@ -1334,7 +1338,7 @@ pub unsafe extern "C" fn cass_result_row_count(result_raw: *const CassResult) -> pub unsafe extern "C" fn cass_result_column_count(result_raw: *const CassResult) -> size_t { let result = ptr_to_ref(result_raw); - result.metadata.col_specs.len() as size_t + result.metadata.col_data_types.len() as size_t } #[no_mangle] @@ -1411,7 +1415,7 @@ mod tests { const THIRD_COLUMN_NAME: &str = "list_double_col"; fn create_cass_rows_result() -> CassResult { let metadata = Arc::new(CassResultData::from_result_payload( - vec![ + &[ col_spec(FIRST_COLUMN_NAME, ColumnType::BigInt), col_spec(SECOND_COLUMN_NAME, ColumnType::Varint), col_spec( @@ -1528,7 +1532,7 @@ mod tests { } fn create_non_rows_cass_result() -> CassResult { - let metadata = Arc::new(CassResultData::from_result_payload(vec![], None)); + let metadata = Arc::new(CassResultData::from_result_payload(&[], None)); CassResult { rows: None, metadata, diff --git a/scylla-rust-wrapper/src/session.rs b/scylla-rust-wrapper/src/session.rs index 96f62710..44134582 100644 --- a/scylla-rust-wrapper/src/session.rs +++ b/scylla-rust-wrapper/src/session.rs @@ -220,7 +220,7 @@ pub unsafe extern "C" fn cass_session_execute_batch( match query_res { Ok(_result) => Ok(CassResultValue::QueryResult(Arc::new(CassResult { rows: None, - metadata: Arc::new(CassResultData::from_result_payload(vec![], None)), + metadata: Arc::new(CassResultData::from_result_payload(&[], None)), tracing_id: None, paging_state_response: PagingStateResponse::NoMorePages, }))), @@ -355,7 +355,7 @@ pub unsafe extern "C" fn cass_session_execute( match query_res { Ok((result, paging_state_response, maybe_col_data_types)) => { let metadata = Arc::new(CassResultData::from_result_payload( - result.col_specs().to_vec(), + result.col_specs(), maybe_col_data_types, )); let cass_rows = result From 52b2b14cc694acc80b1dd515fd2688e7ad5bf9c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 20 Nov 2024 19:14:00 +0100 Subject: [PATCH 5/8] metadata: introduce CassColumnSpec This structure will hold information about the column name and the column data type. Refactored the `CassResultData`, so now it holds one vector of `CassColumnSpec` instead of two separate vectors for names and data types. Previous version was buggy for prepared statements: - data types came from cached metadata from the statement - column names came from metadata provided by query result Because of this, the vector could be of a different length, when someone ALTERed the corresponding table. Notice that after this commit, we don't reuse cached metadata from prepared statement and reuse the metadata provided from the server. Next commits will fix that. --- scylla-rust-wrapper/src/cass_types.rs | 5 ++ scylla-rust-wrapper/src/query_result.rs | 81 +++++++++++-------------- scylla-rust-wrapper/src/session.rs | 15 ++--- 3 files changed, 45 insertions(+), 56 deletions(-) diff --git a/scylla-rust-wrapper/src/cass_types.rs b/scylla-rust-wrapper/src/cass_types.rs index dc52598b..a83451a1 100644 --- a/scylla-rust-wrapper/src/cass_types.rs +++ b/scylla-rust-wrapper/src/cass_types.rs @@ -138,6 +138,11 @@ pub enum MapDataType { KeyAndValue(Arc, Arc), } +pub struct CassColumnSpec { + pub name: String, + pub data_type: Arc, +} + #[derive(Clone, Debug, PartialEq, Eq)] pub enum CassDataType { Value(CassValueType), diff --git a/scylla-rust-wrapper/src/query_result.rs b/scylla-rust-wrapper/src/query_result.rs index a8932d93..bd74abb5 100644 --- a/scylla-rust-wrapper/src/query_result.rs +++ b/scylla-rust-wrapper/src/query_result.rs @@ -1,7 +1,7 @@ use crate::argconv::*; use crate::cass_error::CassError; use crate::cass_types::{ - cass_data_type_type, get_column_type, CassDataType, CassValueType, MapDataType, + cass_data_type_type, get_column_type, CassColumnSpec, CassDataType, CassValueType, MapDataType, }; use crate::inet::CassInet; use crate::metadata::{ @@ -24,37 +24,22 @@ pub struct CassResult { } pub struct CassResultData { - pub col_names: Vec, - pub col_data_types: Arc>>, + pub col_specs: Vec, } impl CassResultData { - pub fn from_result_payload( - col_specs: &[ColumnSpec<'_>], - maybe_col_data_types: Option>>>, - ) -> CassResultData { - // `maybe_col_data_types` is: - // - Some(_) for prepared statements executions - // - None for unprepared (simple) queries executions - let col_data_types = maybe_col_data_types.unwrap_or_else(|| { - // This allocation is unfortunately necessary, because of the type of CassResultData::col_data_types. - Arc::new( - col_specs - .iter() - .map(|col_spec| Arc::new(get_column_type(col_spec.typ()))) - .collect(), - ) - }); - - let col_names = col_specs + pub fn from_column_specs(col_specs: &[ColumnSpec<'_>]) -> CassResultData { + let col_specs = col_specs .iter() - .map(|col_spec| col_spec.name().to_owned()) + .map(|col_spec| { + let name = col_spec.name().to_owned(); + let data_type = Arc::new(get_column_type(col_spec.typ())); + + CassColumnSpec { name, data_type } + }) .collect(); - CassResultData { - col_names, - col_data_types, - } + CassResultData { col_specs } } } @@ -900,12 +885,12 @@ pub unsafe extern "C" fn cass_row_get_column_by_name_n( return row_from_raw .result_metadata - .col_names + .col_specs .iter() .enumerate() - .find(|(_, col_name)| { - is_case_sensitive && *col_name == name_str - || !is_case_sensitive && col_name.eq_ignore_ascii_case(name_str) + .find(|(_, col_spec)| { + is_case_sensitive && col_spec.name == name_str + || !is_case_sensitive && col_spec.name.eq_ignore_ascii_case(name_str) }) .map(|(index, _)| { return match row_from_raw.columns.get(index) { @@ -926,11 +911,16 @@ pub unsafe extern "C" fn cass_result_column_name( let result_from_raw = ptr_to_ref(result); let index_usize: usize = index.try_into().unwrap(); - if index_usize >= result_from_raw.metadata.col_data_types.len() { + if index_usize >= result_from_raw.metadata.col_specs.len() { return CassError::CASS_ERROR_LIB_INDEX_OUT_OF_BOUNDS; } - let column_name = result_from_raw.metadata.col_names.get(index_usize).unwrap(); + let column_name = &result_from_raw + .metadata + .col_specs + .get(index_usize) + .unwrap() + .name; write_str_to_c(column_name, name, name_length); @@ -961,9 +951,9 @@ pub unsafe extern "C" fn cass_result_column_data_type( result_from_raw .metadata - .col_data_types + .col_specs .get(index_usize) - .map(Arc::as_ptr) + .map(|col_spec| Arc::as_ptr(&col_spec.data_type)) .unwrap_or(std::ptr::null()) } @@ -1338,7 +1328,7 @@ pub unsafe extern "C" fn cass_result_row_count(result_raw: *const CassResult) -> pub unsafe extern "C" fn cass_result_column_count(result_raw: *const CassResult) -> size_t { let result = ptr_to_ref(result_raw); - result.metadata.col_data_types.len() as size_t + result.metadata.col_specs.len() as size_t } #[no_mangle] @@ -1414,17 +1404,14 @@ mod tests { const SECOND_COLUMN_NAME: &str = "varint_col"; const THIRD_COLUMN_NAME: &str = "list_double_col"; fn create_cass_rows_result() -> CassResult { - let metadata = Arc::new(CassResultData::from_result_payload( - &[ - col_spec(FIRST_COLUMN_NAME, ColumnType::BigInt), - col_spec(SECOND_COLUMN_NAME, ColumnType::Varint), - col_spec( - THIRD_COLUMN_NAME, - ColumnType::List(Box::new(ColumnType::Double)), - ), - ], - None, - )); + let metadata = Arc::new(CassResultData::from_column_specs(&[ + col_spec(FIRST_COLUMN_NAME, ColumnType::BigInt), + col_spec(SECOND_COLUMN_NAME, ColumnType::Varint), + col_spec( + THIRD_COLUMN_NAME, + ColumnType::List(Box::new(ColumnType::Double)), + ), + ])); let rows = create_cass_rows_from_rows( vec![Row { @@ -1532,7 +1519,7 @@ mod tests { } fn create_non_rows_cass_result() -> CassResult { - let metadata = Arc::new(CassResultData::from_result_payload(&[], None)); + let metadata = Arc::new(CassResultData::from_column_specs(&[])); CassResult { rows: None, metadata, diff --git a/scylla-rust-wrapper/src/session.rs b/scylla-rust-wrapper/src/session.rs index 44134582..e0672cd2 100644 --- a/scylla-rust-wrapper/src/session.rs +++ b/scylla-rust-wrapper/src/session.rs @@ -220,7 +220,7 @@ pub unsafe extern "C" fn cass_session_execute_batch( match query_res { Ok(_result) => Ok(CassResultValue::QueryResult(Arc::new(CassResult { rows: None, - metadata: Arc::new(CassResultData::from_result_payload(&[], None)), + metadata: Arc::new(CassResultData::from_column_specs(&[])), tracing_id: None, paging_state_response: PagingStateResponse::NoMorePages, }))), @@ -353,11 +353,8 @@ pub unsafe extern "C" fn cass_session_execute( }; match query_res { - Ok((result, paging_state_response, maybe_col_data_types)) => { - let metadata = Arc::new(CassResultData::from_result_payload( - result.col_specs(), - maybe_col_data_types, - )); + Ok((result, paging_state_response, _maybe_col_data_types)) => { + let metadata = Arc::new(CassResultData::from_column_specs(result.col_specs())); let cass_rows = result .rows .map(|rows| create_cass_rows_from_rows(rows, &metadata)); @@ -397,9 +394,9 @@ pub(crate) fn create_cass_rows_from_rows( fn create_cass_row_columns(row: Row, metadata: &Arc) -> Vec { row.columns .into_iter() - .zip(metadata.col_data_types.iter()) - .map(|(val, col_data_type)| { - let column_type = Arc::clone(col_data_type); + .zip(metadata.col_specs.iter()) + .map(|(val, col_spec)| { + let column_type = Arc::clone(&col_spec.data_type); CassValue { value: val.map(|col_val| get_column_value(col_val, &column_type)), value_type: column_type, From e8f92ef7abaf2a1ced788e2b0ecaa8441a68af7e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 20 Nov 2024 19:23:12 +0100 Subject: [PATCH 6/8] prepared: reuse prepared statement's result metadata --- scylla-rust-wrapper/src/cass_types.rs | 1 + scylla-rust-wrapper/src/prepared.rs | 22 +++++++++----------- scylla-rust-wrapper/src/query_result.rs | 1 + scylla-rust-wrapper/src/session.rs | 27 +++++++++++++++---------- 4 files changed, 27 insertions(+), 24 deletions(-) diff --git a/scylla-rust-wrapper/src/cass_types.rs b/scylla-rust-wrapper/src/cass_types.rs index a83451a1..cae85d42 100644 --- a/scylla-rust-wrapper/src/cass_types.rs +++ b/scylla-rust-wrapper/src/cass_types.rs @@ -138,6 +138,7 @@ pub enum MapDataType { KeyAndValue(Arc, Arc), } +#[derive(Debug)] pub struct CassColumnSpec { pub name: String, pub data_type: Arc, diff --git a/scylla-rust-wrapper/src/prepared.rs b/scylla-rust-wrapper/src/prepared.rs index f36142c3..0fb7cdb5 100644 --- a/scylla-rust-wrapper/src/prepared.rs +++ b/scylla-rust-wrapper/src/prepared.rs @@ -5,6 +5,7 @@ use crate::{ argconv::*, cass_error::CassError, cass_types::{get_column_type, CassDataType}, + query_result::CassResultData, statement::{CassStatement, Statement}, types::size_t, }; @@ -14,11 +15,10 @@ use scylla::prepared_statement::PreparedStatement; pub struct CassPrepared { // Data types of columns from PreparedMetadata. pub variable_col_data_types: Vec>, - // Data types of columns from ResultMetadata. - // - // Arc -> to share each data type with other structs such as `CassValue` - // Arc> -> to share the whole vector with `CassResultData`. - pub result_col_data_types: Arc>>, + + // Cached result metadata. Arc'ed since we want to share it + // with result metadata after execution. + pub result_metadata: Arc, pub statement: PreparedStatement, } @@ -30,17 +30,13 @@ impl CassPrepared { .map(|col_spec| Arc::new(get_column_type(col_spec.typ()))) .collect(); - let result_col_data_types: Arc>> = Arc::new( - statement - .get_result_set_col_specs() - .iter() - .map(|col_spec| Arc::new(get_column_type(col_spec.typ()))) - .collect(), - ); + let result_metadata = Arc::new(CassResultData::from_column_specs( + statement.get_result_set_col_specs(), + )); Self { variable_col_data_types, - result_col_data_types, + result_metadata, statement, } } diff --git a/scylla-rust-wrapper/src/query_result.rs b/scylla-rust-wrapper/src/query_result.rs index bd74abb5..0295f476 100644 --- a/scylla-rust-wrapper/src/query_result.rs +++ b/scylla-rust-wrapper/src/query_result.rs @@ -23,6 +23,7 @@ pub struct CassResult { pub paging_state_response: PagingStateResponse, } +#[derive(Debug)] pub struct CassResultData { pub col_specs: Vec, } diff --git a/scylla-rust-wrapper/src/session.rs b/scylla-rust-wrapper/src/session.rs index e0672cd2..5e492243 100644 --- a/scylla-rust-wrapper/src/session.rs +++ b/scylla-rust-wrapper/src/session.rs @@ -300,20 +300,20 @@ pub unsafe extern "C" fn cass_session_execute( // Since `query.query` is consumed, we cannot match the statement // after execution, to retrieve the cached metadata in case // of prepared statements. - Option>>>, + Option>, ), QueryError, >; let query_res: QueryRes = match statement { Statement::Simple(query) => { // We don't store result metadata for Queries - return None. - let maybe_result_col_data_types = None; + let maybe_result_metadata = None; if paging_enabled { session .query_single_page(query.query, bound_values, paging_state) .await - .map(|(qr, psr)| (qr, psr, maybe_result_col_data_types)) + .map(|(qr, psr)| (qr, psr, maybe_result_metadata)) } else { session .query_unpaged(query.query, bound_values) @@ -322,21 +322,21 @@ pub unsafe extern "C" fn cass_session_execute( ( result, PagingStateResponse::NoMorePages, - maybe_result_col_data_types, + maybe_result_metadata, ) }) } } Statement::Prepared(prepared) => { - // Clone vector of the Arc, so we don't do additional allocations when constructing - // CassDataTypes in `CassResultData::from_result_payload`. - let maybe_result_col_data_types = Some(prepared.result_col_data_types.clone()); + // Clone result metadata, so we don't need to construct it from scratch in + // `CassResultData::from_column_specs` - it requires a lot of allocations for complex types. + let maybe_result_metadata = Some(Arc::clone(&prepared.result_metadata)); if paging_enabled { session .execute_single_page(&prepared.statement, bound_values, paging_state) .await - .map(|(qr, psr)| (qr, psr, maybe_result_col_data_types)) + .map(|(qr, psr)| (qr, psr, maybe_result_metadata)) } else { session .execute_unpaged(&prepared.statement, bound_values) @@ -345,7 +345,7 @@ pub unsafe extern "C" fn cass_session_execute( ( result, PagingStateResponse::NoMorePages, - maybe_result_col_data_types, + maybe_result_metadata, ) }) } @@ -353,8 +353,13 @@ pub unsafe extern "C" fn cass_session_execute( }; match query_res { - Ok((result, paging_state_response, _maybe_col_data_types)) => { - let metadata = Arc::new(CassResultData::from_column_specs(result.col_specs())); + Ok((result, paging_state_response, maybe_result_metadata)) => { + // maybe_result_metadata is: + // - Some(_) for prepared statements + // - None for unprepared statements + let metadata = maybe_result_metadata.unwrap_or_else(|| { + Arc::new(CassResultData::from_column_specs(result.col_specs())) + }); let cass_rows = result .rows .map(|rows| create_cass_rows_from_rows(rows, &metadata)); From 2f50cd06967c424824dae12c62fed7ea18ee60b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Wed, 20 Nov 2024 19:23:49 +0100 Subject: [PATCH 7/8] ci: enable AlterDoesntUpdateColumnCount test It is now enabled in favor of AlterProperlyUpdatesColumnCount. From now on, the metadata in `CassResultData` is consistent, and the behaviour is that metadata is not updated after altering the table. Original cpp-driver expects this behaviour for CQL v4 and less: * Verify that the column count of a bound statement's result metadata doesn't * change for older protocol versions (v4 and less) when a table's schema is altered. --- Makefile | 2 -- 1 file changed, 2 deletions(-) diff --git a/Makefile b/Makefile index 6afcbdcb..83530513 100644 --- a/Makefile +++ b/Makefile @@ -26,7 +26,6 @@ SCYLLA_TEST_FILTER := $(subst ${SPACE},${EMPTY},ClusterTests.*\ :HeartbeatTests.Integration_Cassandra_HeartbeatFailed\ :ExecutionProfileTest.InvalidName\ :*NoCompactEnabledConnection\ -:PreparedMetadataTests.Integration_Cassandra_AlterDoesntUpdateColumnCount\ :PreparedMetadataTests.Integration_Cassandra_AlterProperlyUpdatesColumnCount\ :UseKeyspaceCaseSensitiveTests.Integration_Cassandra_ConnectWithKeyspace) endif @@ -57,7 +56,6 @@ CASSANDRA_TEST_FILTER := $(subst ${SPACE},${EMPTY},ClusterTests.*\ :SslTests.Integration_Cassandra_ReconnectAfterClusterCrashAndRestart\ :ExecutionProfileTest.InvalidName\ :*NoCompactEnabledConnection\ -:PreparedMetadataTests.Integration_Cassandra_AlterDoesntUpdateColumnCount\ :PreparedMetadataTests.Integration_Cassandra_AlterProperlyUpdatesColumnCount\ :UseKeyspaceCaseSensitiveTests.Integration_Cassandra_ConnectWithKeyspace) endif From b249bfffce2c5002b444da7180a6c0833bd1ece5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miko=C5=82aj=20Uzarski?= Date: Thu, 21 Nov 2024 15:07:13 +0100 Subject: [PATCH 8/8] metadata: rename CassResultData -> CassResultMetadata --- scylla-rust-wrapper/src/prepared.rs | 6 +++--- scylla-rust-wrapper/src/query_result.rs | 20 +++++++++++--------- scylla-rust-wrapper/src/session.rs | 14 +++++++------- 3 files changed, 21 insertions(+), 19 deletions(-) diff --git a/scylla-rust-wrapper/src/prepared.rs b/scylla-rust-wrapper/src/prepared.rs index 0fb7cdb5..457526d1 100644 --- a/scylla-rust-wrapper/src/prepared.rs +++ b/scylla-rust-wrapper/src/prepared.rs @@ -5,7 +5,7 @@ use crate::{ argconv::*, cass_error::CassError, cass_types::{get_column_type, CassDataType}, - query_result::CassResultData, + query_result::CassResultMetadata, statement::{CassStatement, Statement}, types::size_t, }; @@ -18,7 +18,7 @@ pub struct CassPrepared { // Cached result metadata. Arc'ed since we want to share it // with result metadata after execution. - pub result_metadata: Arc, + pub result_metadata: Arc, pub statement: PreparedStatement, } @@ -30,7 +30,7 @@ impl CassPrepared { .map(|col_spec| Arc::new(get_column_type(col_spec.typ()))) .collect(); - let result_metadata = Arc::new(CassResultData::from_column_specs( + let result_metadata = Arc::new(CassResultMetadata::from_column_specs( statement.get_result_set_col_specs(), )); diff --git a/scylla-rust-wrapper/src/query_result.rs b/scylla-rust-wrapper/src/query_result.rs index 0295f476..cdf3bd93 100644 --- a/scylla-rust-wrapper/src/query_result.rs +++ b/scylla-rust-wrapper/src/query_result.rs @@ -18,18 +18,18 @@ use uuid::Uuid; pub struct CassResult { pub rows: Option>, - pub metadata: Arc, + pub metadata: Arc, pub tracing_id: Option, pub paging_state_response: PagingStateResponse, } #[derive(Debug)] -pub struct CassResultData { +pub struct CassResultMetadata { pub col_specs: Vec, } -impl CassResultData { - pub fn from_column_specs(col_specs: &[ColumnSpec<'_>]) -> CassResultData { +impl CassResultMetadata { + pub fn from_column_specs(col_specs: &[ColumnSpec<'_>]) -> CassResultMetadata { let col_specs = col_specs .iter() .map(|col_spec| { @@ -40,7 +40,7 @@ impl CassResultData { }) .collect(); - CassResultData { col_specs } + CassResultMetadata { col_specs } } } @@ -48,7 +48,7 @@ impl CassResultData { /// It will be freed, when CassResult is freed.(see #[cass_result_free]) pub struct CassRow { pub columns: Vec, - pub result_metadata: Arc, + pub result_metadata: Arc, } pub enum Value { @@ -1395,7 +1395,9 @@ mod tests { session::create_cass_rows_from_rows, }; - use super::{cass_result_column_count, cass_result_column_type, CassResult, CassResultData}; + use super::{ + cass_result_column_count, cass_result_column_type, CassResult, CassResultMetadata, + }; fn col_spec(name: &'static str, typ: ColumnType<'static>) -> ColumnSpec<'static> { ColumnSpec::borrowed(name, typ, TableSpec::borrowed("ks", "tbl")) @@ -1405,7 +1407,7 @@ mod tests { const SECOND_COLUMN_NAME: &str = "varint_col"; const THIRD_COLUMN_NAME: &str = "list_double_col"; fn create_cass_rows_result() -> CassResult { - let metadata = Arc::new(CassResultData::from_column_specs(&[ + let metadata = Arc::new(CassResultMetadata::from_column_specs(&[ col_spec(FIRST_COLUMN_NAME, ColumnType::BigInt), col_spec(SECOND_COLUMN_NAME, ColumnType::Varint), col_spec( @@ -1520,7 +1522,7 @@ mod tests { } fn create_non_rows_cass_result() -> CassResult { - let metadata = Arc::new(CassResultData::from_column_specs(&[])); + let metadata = Arc::new(CassResultMetadata::from_column_specs(&[])); CassResult { rows: None, metadata, diff --git a/scylla-rust-wrapper/src/session.rs b/scylla-rust-wrapper/src/session.rs index 5e492243..6de63835 100644 --- a/scylla-rust-wrapper/src/session.rs +++ b/scylla-rust-wrapper/src/session.rs @@ -10,7 +10,7 @@ use crate::metadata::create_table_metadata; use crate::metadata::{CassKeyspaceMeta, CassMaterializedViewMeta, CassSchemaMeta}; use crate::prepared::CassPrepared; use crate::query_result::Value::{CollectionValue, RegularValue}; -use crate::query_result::{CassResult, CassResultData, CassRow, CassValue, Collection, Value}; +use crate::query_result::{CassResult, CassResultMetadata, CassRow, CassValue, Collection, Value}; use crate::statement::CassStatement; use crate::statement::Statement; use crate::types::{cass_uint64_t, size_t}; @@ -220,7 +220,7 @@ pub unsafe extern "C" fn cass_session_execute_batch( match query_res { Ok(_result) => Ok(CassResultValue::QueryResult(Arc::new(CassResult { rows: None, - metadata: Arc::new(CassResultData::from_column_specs(&[])), + metadata: Arc::new(CassResultMetadata::from_column_specs(&[])), tracing_id: None, paging_state_response: PagingStateResponse::NoMorePages, }))), @@ -300,7 +300,7 @@ pub unsafe extern "C" fn cass_session_execute( // Since `query.query` is consumed, we cannot match the statement // after execution, to retrieve the cached metadata in case // of prepared statements. - Option>, + Option>, ), QueryError, >; @@ -329,7 +329,7 @@ pub unsafe extern "C" fn cass_session_execute( } Statement::Prepared(prepared) => { // Clone result metadata, so we don't need to construct it from scratch in - // `CassResultData::from_column_specs` - it requires a lot of allocations for complex types. + // `CassResultMetadata::from_column_specs` - it requires a lot of allocations for complex types. let maybe_result_metadata = Some(Arc::clone(&prepared.result_metadata)); if paging_enabled { @@ -358,7 +358,7 @@ pub unsafe extern "C" fn cass_session_execute( // - Some(_) for prepared statements // - None for unprepared statements let metadata = maybe_result_metadata.unwrap_or_else(|| { - Arc::new(CassResultData::from_column_specs(result.col_specs())) + Arc::new(CassResultMetadata::from_column_specs(result.col_specs())) }); let cass_rows = result .rows @@ -386,7 +386,7 @@ pub unsafe extern "C" fn cass_session_execute( pub(crate) fn create_cass_rows_from_rows( rows: Vec, - metadata: &Arc, + metadata: &Arc, ) -> Vec { rows.into_iter() .map(|r| CassRow { @@ -396,7 +396,7 @@ pub(crate) fn create_cass_rows_from_rows( .collect() } -fn create_cass_row_columns(row: Row, metadata: &Arc) -> Vec { +fn create_cass_row_columns(row: Row, metadata: &Arc) -> Vec { row.columns .into_iter() .zip(metadata.col_specs.iter())