diff --git a/Makefile b/Makefile index 7e49405c..83530513 100644 --- a/Makefile +++ b/Makefile @@ -26,7 +26,7 @@ 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 @@ -56,7 +56,7 @@ 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 diff --git a/scylla-rust-wrapper/src/cass_types.rs b/scylla-rust-wrapper/src/cass_types.rs index dc52598b..cae85d42 100644 --- a/scylla-rust-wrapper/src/cass_types.rs +++ b/scylla-rust-wrapper/src/cass_types.rs @@ -138,6 +138,12 @@ pub enum MapDataType { KeyAndValue(Arc, Arc), } +#[derive(Debug)] +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/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/prepared.rs b/scylla-rust-wrapper/src/prepared.rs index f36142c3..457526d1 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::CassResultMetadata, 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(CassResultMetadata::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 95487846..cdf3bd93 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::{ @@ -18,42 +18,29 @@ use uuid::Uuid; pub struct CassResult { pub rows: Option>, - pub metadata: Arc, + 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>>, - pub tracing_id: Option, +#[derive(Debug)] +pub struct CassResultMetadata { + pub col_specs: Vec, } -impl CassResultData { - pub fn from_result_payload( - 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 - // - 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(), - ) - }); - - CassResultData { - paging_state_response, - col_specs, - col_data_types, - tracing_id, - } +impl CassResultMetadata { + pub fn from_column_specs(col_specs: &[ColumnSpec<'_>]) -> CassResultMetadata { + let col_specs = col_specs + .iter() + .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(); + + CassResultMetadata { col_specs } } } @@ -61,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 { @@ -851,7 +838,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] @@ -902,9 +889,9 @@ pub unsafe extern "C" fn cass_row_get_column_by_name_n( .col_specs .iter() .enumerate() - .find(|(_, spec)| { - is_case_sensitive && spec.name() == name_str - || !is_case_sensitive && spec.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) { @@ -929,8 +916,12 @@ pub unsafe extern "C" fn cass_result_column_name( 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_specs + .get(index_usize) + .unwrap() + .name; write_str_to_c(column_name, name, name_length); @@ -961,9 +952,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()) } @@ -1365,7 +1356,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; @@ -1404,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")) @@ -1414,19 +1407,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( - PagingStateResponse::NoMorePages, - vec![ - 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, - None, - )); + let metadata = Arc::new(CassResultMetadata::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 { @@ -1446,6 +1434,8 @@ mod tests { CassResult { rows: Some(rows), metadata, + tracing_id: None, + paging_state_response: PagingStateResponse::NoMorePages, } } @@ -1532,15 +1522,12 @@ mod tests { } fn create_non_rows_cass_result() -> CassResult { - let metadata = Arc::new(CassResultData::from_result_payload( - PagingStateResponse::NoMorePages, - vec![], - None, - None, - )); + let metadata = Arc::new(CassResultMetadata::from_column_specs(&[])); 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 51cdca94..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,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, - None, - )), + metadata: Arc::new(CassResultMetadata::from_column_specs(&[])), + tracing_id: None, + paging_state_response: PagingStateResponse::NoMorePages, }))), Err(err) => Ok(CassResultValue::QueryError(Arc::new(err))), } @@ -303,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) @@ -325,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 + // `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 { 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) @@ -348,7 +345,7 @@ pub unsafe extern "C" fn cass_session_execute( ( result, PagingStateResponse::NoMorePages, - maybe_result_col_data_types, + maybe_result_metadata, ) }) } @@ -356,19 +353,21 @@ 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, - result.tracing_id, - )); + 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(CassResultMetadata::from_column_specs(result.col_specs())) + }); let cass_rows = result .rows .map(|rows| create_cass_rows_from_rows(rows, &metadata)); let cass_result = Arc::new(CassResult { rows: cass_rows, metadata, + tracing_id: result.tracing_id, + paging_state_response, }); Ok(CassResultValue::QueryResult(cass_result)) @@ -387,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 { @@ -397,12 +396,12 @@ 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_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, 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(), }