From 8c42aa212d4d13adf754b2c43d199bf910e7d110 Mon Sep 17 00:00:00 2001 From: kould Date: Fri, 19 Apr 2024 23:55:21 +0800 Subject: [PATCH 1/9] feat: add `ModifyColumn` for `AlterKind` --- .../src/ddl/alter_table/update_metadata.rs | 2 +- src/store-api/src/metadata.rs | 80 ++++++-- src/store-api/src/region_request.rs | 126 +++++++++++- src/table/src/error.rs | 12 ++ src/table/src/metadata.rs | 180 +++++++++++++++++- src/table/src/requests.rs | 9 + 6 files changed, 389 insertions(+), 20 deletions(-) diff --git a/src/common/meta/src/ddl/alter_table/update_metadata.rs b/src/common/meta/src/ddl/alter_table/update_metadata.rs index a2bc4448603d..cf9f1f226839 100644 --- a/src/common/meta/src/ddl/alter_table/update_metadata.rs +++ b/src/common/meta/src/ddl/alter_table/update_metadata.rs @@ -51,7 +51,7 @@ impl AlterTableProcedure { AlterKind::RenameTable { new_table_name } => { new_info.name = new_table_name.to_string(); } - AlterKind::DropColumns { .. } => {} + AlterKind::DropColumns { .. } | AlterKind::ModifyColumns { .. } => {} } Ok(new_info) diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 1c9c9700119d..184c90c645f6 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -23,7 +23,7 @@ use std::sync::Arc; use api::helper::ColumnDataTypeWrapper; use api::v1::region::RegionColumnDef; -use api::v1::SemanticType; +use api::v1::{ColumnDef, SemanticType}; use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; use common_macro::stack_trace_debug; @@ -33,7 +33,7 @@ use serde::de::Error; use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; -use crate::region_request::{AddColumn, AddColumnLocation, AlterKind}; +use crate::region_request::{AddColumn, AddColumnLocation, AlterKind, ModifyColumn}; use crate::storage::consts::is_internal_column; use crate::storage::{ColumnId, RegionId}; @@ -61,18 +61,7 @@ impl fmt::Debug for ColumnMetadata { } impl ColumnMetadata { - /// Construct `Self` from protobuf struct [RegionColumnDef] - pub fn try_from_column_def(column_def: RegionColumnDef) -> Result { - let column_id = column_def.column_id; - - let column_def = column_def - .column_def - .context(InvalidRawRegionRequestSnafu { - err: "column_def is absent", - })?; - - let semantic_type = column_def.semantic_type(); - + fn inner_try_from_column_def(column_def: ColumnDef) -> Result { let default_constrain = if column_def.default_constraint.is_empty() { None } else { @@ -86,9 +75,22 @@ impl ColumnMetadata { column_def.datatype_extension.clone(), ) .into(); - let column_schema = ColumnSchema::new(column_def.name, data_type, column_def.is_nullable) + ColumnSchema::new(column_def.name, data_type, column_def.is_nullable) .with_default_constraint(default_constrain) - .context(ConvertDatatypesSnafu)?; + .context(ConvertDatatypesSnafu) + } + + /// Construct `Self` from protobuf struct [RegionColumnDef] + pub fn try_from_column_def(column_def: RegionColumnDef) -> Result { + let column_id = column_def.column_id; + let column_def = column_def + .column_def + .context(InvalidRawRegionRequestSnafu { + err: "column_def is absent", + })?; + let semantic_type = column_def.semantic_type(); + let column_schema = Self::inner_try_from_column_def(column_def)?; + Ok(Self { column_schema, semantic_type, @@ -535,6 +537,7 @@ impl RegionMetadataBuilder { match kind { AlterKind::AddColumns { columns } => self.add_columns(columns)?, AlterKind::DropColumns { names } => self.drop_columns(&names), + AlterKind::ModifyColumns { columns } => self.modify_columns(columns), } Ok(self) } @@ -615,6 +618,25 @@ impl RegionMetadataBuilder { self.column_metadatas .retain(|col| !name_set.contains(&col.column_schema.name)); } + + /// Modifies columns to the metadata if exist. + fn modify_columns(&mut self, columns: Vec) { + let mut modify_map: HashMap<_, _> = columns + .into_iter() + .map( + |ModifyColumn { + column_name, + target_type, + }| (column_name, target_type), + ) + .collect(); + + for column_meta in self.column_metadatas.iter_mut() { + if let Some(target_type) = modify_map.remove(&column_meta.column_schema.name) { + column_meta.column_schema.data_type = target_type; + } + } + } } /// Fields skipped in serialization. @@ -707,6 +729,13 @@ pub enum MetadataError { #[snafu(display("Time index column not found"))] TimeIndexNotFound { location: Location }, + + #[snafu(display("Modify column {} not exists in region: {}", column_name, region_id))] + ModifyColumnNotFound { + column_name: String, + region_id: RegionId, + location: Location, + }, } impl ErrorExt for MetadataError { @@ -1112,7 +1141,7 @@ mod test { let metadata = builder.build().unwrap(); check_columns(&metadata, &["a", "b", "f", "c", "d"]); - let mut builder = RegionMetadataBuilder::from_existing(metadata); + let mut builder = RegionMetadataBuilder::from_existing(metadata.clone()); builder .alter(AlterKind::DropColumns { names: vec!["a".to_string()], @@ -1121,6 +1150,23 @@ mod test { // Build returns error as the primary key contains a. let err = builder.build().unwrap_err(); assert_eq!(StatusCode::InvalidArguments, err.status_code()); + + let mut builder = RegionMetadataBuilder::from_existing(metadata); + builder + .alter(AlterKind::ModifyColumns { + columns: vec![ModifyColumn { + column_name: "b".to_string(), + target_type: ConcreteDataType::string_datatype(), + }], + }) + .unwrap(); + let metadata = builder.build().unwrap(); + check_columns(&metadata, &["a", "b", "f", "c", "d"]); + let b_type = metadata + .column_by_name("b") + .map(|column_meta| column_meta.column_schema.data_type.clone()) + .unwrap(); + assert_eq!(ConcreteDataType::string_datatype(), b_type); } #[test] diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index f07b240b2a3e..9a6a187882a7 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -23,13 +23,14 @@ use api::v1::region::{ }; use api::v1::{self, Rows, SemanticType}; pub use common_base::AffectedRows; +use datatypes::data_type::ConcreteDataType; use snafu::{ensure, OptionExt}; use strum::IntoStaticStr; use crate::logstore::entry; use crate::metadata::{ ColumnMetadata, InvalidRawRegionRequestSnafu, InvalidRegionRequestSnafu, MetadataError, - RegionMetadata, Result, + ModifyColumnNotFoundSnafu, RegionMetadata, Result, }; use crate::path_utils::region_dir; use crate::storage::{ColumnId, RegionId, ScanRequest}; @@ -332,6 +333,9 @@ pub enum AlterKind { /// Name of columns to drop. names: Vec, }, + ModifyColumns { + columns: Vec, + }, } impl AlterKind { @@ -350,6 +354,11 @@ impl AlterKind { Self::validate_column_to_drop(name, metadata)?; } } + AlterKind::ModifyColumns { columns } => { + for col_to_modify in columns { + col_to_modify.validate(metadata)?; + } + } } Ok(()) } @@ -364,6 +373,9 @@ impl AlterKind { AlterKind::DropColumns { names } => names .iter() .any(|name| metadata.column_by_name(name).is_some()), + AlterKind::ModifyColumns { columns } => columns + .iter() + .any(|col_to_modify| col_to_modify.need_alter(metadata)), } } @@ -501,6 +513,60 @@ impl TryFrom for AddColumnLocation { } } +/// Modify a column. +#[derive(Debug, PartialEq, Eq, Clone)] +pub struct ModifyColumn { + /// Schema of the column to modify. + pub column_name: String, + /// Column will be modified to this type. + pub target_type: ConcreteDataType, +} + +impl ModifyColumn { + /// Returns an error if the column to modify is invalid. + pub fn validate(&self, metadata: &RegionMetadata) -> Result<()> { + let column_meta = + metadata + .column_by_name(&self.column_name) + .context(ModifyColumnNotFoundSnafu { + column_name: self.column_name.clone(), + region_id: metadata.region_id, + })?; + + ensure!( + !matches!( + column_meta.semantic_type, + SemanticType::Timestamp | SemanticType::Tag + ), + InvalidRegionRequestSnafu { + region_id: metadata.region_id, + err: "'timestamp' or 'tag' column cannot change type".to_string() + } + ); + ensure!( + column_meta + .column_schema + .data_type + .can_arrow_type_cast_to(&self.target_type), + InvalidRegionRequestSnafu { + region_id: metadata.region_id, + err: format!( + "column '{}' cannot be cast automatically to type '{}'", + self.column_name, self.target_type + ), + } + ); + + Ok(()) + } + + /// Returns true if no column to modify to the region. + pub fn need_alter(&self, metadata: &RegionMetadata) -> bool { + debug_assert!(self.validate(metadata).is_ok()); + metadata.column_by_name(&self.column_name).is_some() + } +} + #[derive(Debug, Default)] pub struct RegionFlushRequest { pub row_group_size: Option, @@ -678,6 +744,15 @@ mod tests { semantic_type: SemanticType::Field, column_id: 3, }) + .push_column_metadata(ColumnMetadata { + column_schema: ColumnSchema::new( + "field_1", + ConcreteDataType::boolean_datatype(), + true, + ), + semantic_type: SemanticType::Field, + column_id: 4, + }) .primary_key(vec![2]); builder.build().unwrap() } @@ -790,6 +865,55 @@ mod tests { assert!(kind.need_alter(&metadata)); } + #[test] + fn test_validate_modify_column() { + let metadata = new_metadata(); + AlterKind::ModifyColumns { + columns: vec![ModifyColumn { + column_name: "xxxx".to_string(), + target_type: ConcreteDataType::string_datatype(), + }], + } + .validate(&metadata) + .unwrap_err(); + + AlterKind::ModifyColumns { + columns: vec![ModifyColumn { + column_name: "field_1".to_string(), + target_type: ConcreteDataType::date_datatype(), + }], + } + .validate(&metadata) + .unwrap_err(); + + AlterKind::ModifyColumns { + columns: vec![ModifyColumn { + column_name: "ts".to_string(), + target_type: ConcreteDataType::date_datatype(), + }], + } + .validate(&metadata) + .unwrap_err(); + + AlterKind::ModifyColumns { + columns: vec![ModifyColumn { + column_name: "tag_0".to_string(), + target_type: ConcreteDataType::date_datatype(), + }], + } + .validate(&metadata) + .unwrap_err(); + + let kind = AlterKind::ModifyColumns { + columns: vec![ModifyColumn { + column_name: "field_0".to_string(), + target_type: ConcreteDataType::int32_datatype(), + }], + }; + kind.validate(&metadata).unwrap(); + assert!(kind.need_alter(&metadata)); + } + #[test] fn test_validate_schema_version() { let mut metadata = new_metadata(); diff --git a/src/table/src/error.rs b/src/table/src/error.rs index b67dbac5e2f5..c95756f0d0d5 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -91,6 +91,17 @@ pub enum Error { location: Location, }, + #[snafu(display( + "Not allowed to modify index column '{}' from table {}", + column_name, + table_name + ))] + ModifyColumnInIndex { + column_name: String, + table_name: String, + location: Location, + }, + #[snafu(display( "Failed to build column descriptor for table: {}, column: {}", table_name, @@ -147,6 +158,7 @@ impl ErrorExt for Error { | Error::SchemaConversion { .. } | Error::TableProjection { .. } => StatusCode::EngineExecuteQuery, Error::RemoveColumnInIndex { .. } + | Error::ModifyColumnInIndex { .. } | Error::BuildColumnDescriptor { .. } | Error::InvalidAlterRequest { .. } => StatusCode::InvalidArguments, Error::TablesRecordBatch { .. } | Error::DuplicatedExecuteCall { .. } => { diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 4746a17bcf7b..43400afddea2 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -27,7 +27,7 @@ use snafu::{ensure, ResultExt}; use store_api::storage::{ColumnDescriptor, ColumnDescriptorBuilder, ColumnId, RegionId}; use crate::error::{self, Result}; -use crate::requests::{AddColumnRequest, AlterKind, TableOptions}; +use crate::requests::{AddColumnRequest, AlterKind, ModifyColumnRequest, TableOptions}; pub type TableId = u32; pub type TableVersion = u64; @@ -195,6 +195,7 @@ impl TableMeta { self.add_columns(table_name, columns, add_if_not_exists) } AlterKind::DropColumns { names } => self.remove_columns(table_name, names), + AlterKind::ModifyColumns { columns } => self.modify_columns(table_name, columns), // No need to rebuild table meta when renaming tables. AlterKind::RenameTable { .. } => { let mut meta_builder = TableMetaBuilder::default(); @@ -458,6 +459,118 @@ impl TableMeta { Ok(meta_builder) } + fn modify_columns( + &self, + table_name: &str, + requests: &[ModifyColumnRequest], + ) -> Result { + let table_schema = &self.schema; + let mut meta_builder = self.new_meta_builder(); + + let mut modify_columns: HashMap<_, _> = HashMap::with_capacity(requests.len()); + let timestamp_index = table_schema.timestamp_index(); + + for col_to_modify in requests { + let modify_column_name = &col_to_modify.column_name; + + if let Some(index) = table_schema.column_index_by_name(modify_column_name) { + let data_type = &table_schema.column_schemas()[index].data_type; + + ensure!( + !self.primary_key_indices.contains(&index), + error::ModifyColumnInIndexSnafu { + column_name: modify_column_name, + table_name, + } + ); + + if let Some(ts_index) = timestamp_index { + // Not allowed to modify column in timestamp index. + ensure!( + index != ts_index, + error::ModifyColumnInIndexSnafu { + column_name: table_schema.column_name_by_index(ts_index), + table_name, + } + ); + } + + ensure!( + modify_columns + .insert(&col_to_modify.column_name, col_to_modify) + .is_none(), + error::InvalidAlterRequestSnafu { + table: table_name, + err: format!("modify column {} more than once", col_to_modify.column_name), + } + ); + + ensure!( + table_schema.contains_column(&col_to_modify.column_name), + error::ColumnExistsSnafu { + table_name, + column_name: col_to_modify.column_name.to_string() + }, + ); + + ensure!( + data_type.can_arrow_type_cast_to(&col_to_modify.target_type), + error::InvalidAlterRequestSnafu { + table: table_name, + err: format!( + "column '{}' cannot be cast automatically to type '{}'", + col_to_modify.column_name, col_to_modify.target_type, + ), + } + ); + } else { + return error::ColumnNotExistsSnafu { + column_name: modify_column_name, + table_name, + } + .fail()?; + } + } + // Collect columns after modified. + let columns: Vec<_> = table_schema + .column_schemas() + .iter() + .cloned() + .map(|mut column| { + if let Some(modify_column) = modify_columns.get(&column.name) { + column.data_type = modify_column.target_type.clone(); + } + column + }) + .collect(); + + let mut builder = SchemaBuilder::try_from_columns(columns) + .with_context(|_| error::SchemaBuildSnafu { + msg: format!("Failed to convert column schemas into schema for table {table_name}"), + })? + // Also bump the schema version. + .version(table_schema.version() + 1); + for (k, v) in table_schema.metadata().iter() { + builder = builder.add_metadata(k, v); + } + let new_schema = builder.build().with_context(|_| { + let column_names: Vec<_> = requests + .iter() + .map(|request| &request.column_name) + .collect(); + + error::SchemaBuildSnafu { + msg: format!("Table {table_name} cannot modify columns {column_names:?}"), + } + })?; + + let _ = meta_builder + .schema(Arc::new(new_schema)) + .primary_key_indices(self.primary_key_indices.clone()); + + Ok(meta_builder) + } + /// Split requests into different groups using column location info. fn split_requests_by_column_location<'a>( &self, @@ -1027,6 +1140,31 @@ mod tests { assert_eq!(StatusCode::TableColumnNotFound, err.status_code()); } + #[test] + fn test_modify_unknown_column() { + let schema = Arc::new(new_test_schema()); + let meta = TableMetaBuilder::default() + .schema(schema) + .primary_key_indices(vec![0]) + .engine("engine") + .next_column_id(3) + .build() + .unwrap(); + + let alter_kind = AlterKind::ModifyColumns { + columns: vec![ModifyColumnRequest { + column_name: "unknown".to_string(), + target_type: ConcreteDataType::string_datatype(), + }], + }; + + let err = meta + .builder_with_alter_kind("my_table", &alter_kind, false) + .err() + .unwrap(); + assert_eq!(StatusCode::TableColumnNotFound, err.status_code()); + } + #[test] fn test_remove_key_column() { let schema = Arc::new(new_test_schema()); @@ -1061,6 +1199,46 @@ mod tests { assert_eq!(StatusCode::InvalidArguments, err.status_code()); } + #[test] + fn test_modify_key_column() { + let schema = Arc::new(new_test_schema()); + let meta = TableMetaBuilder::default() + .schema(schema) + .primary_key_indices(vec![0]) + .engine("engine") + .next_column_id(3) + .build() + .unwrap(); + + // Remove column in primary key. + let alter_kind = AlterKind::ModifyColumns { + columns: vec![ModifyColumnRequest { + column_name: "col1".to_string(), + target_type: ConcreteDataType::string_datatype(), + }], + }; + + let err = meta + .builder_with_alter_kind("my_table", &alter_kind, false) + .err() + .unwrap(); + assert_eq!(StatusCode::InvalidArguments, err.status_code()); + + // Remove timestamp column. + let alter_kind = AlterKind::ModifyColumns { + columns: vec![ModifyColumnRequest { + column_name: "ts".to_string(), + target_type: ConcreteDataType::string_datatype(), + }], + }; + + let err = meta + .builder_with_alter_kind("my_table", &alter_kind, false) + .err() + .unwrap(); + assert_eq!(StatusCode::InvalidArguments, err.status_code()); + } + #[test] fn test_alloc_new_column() { let schema = Arc::new(new_test_schema()); diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 0c1e322952ac..3badf4d8fcc7 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -22,6 +22,7 @@ use common_base::readable_size::ReadableSize; use common_datasource::object_store::s3::is_supported_in_s3; use common_query::AddColumnLocation; use common_time::range::TimestampRange; +use datatypes::data_type::ConcreteDataType; use datatypes::prelude::VectorRef; use datatypes::schema::ColumnSchema; use serde::{Deserialize, Serialize}; @@ -164,10 +165,18 @@ pub struct AddColumnRequest { pub location: Option, } +/// Modify column request +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ModifyColumnRequest { + pub column_name: String, + pub target_type: ConcreteDataType, +} + #[derive(Debug, Clone, Serialize, Deserialize)] pub enum AlterKind { AddColumns { columns: Vec }, DropColumns { names: Vec }, + ModifyColumns { columns: Vec }, RenameTable { new_table_name: String }, } From 1e8fed60af1e718e3fdc22c8051d29f7c0a5d263 Mon Sep 17 00:00:00 2001 From: kould Date: Sun, 21 Apr 2024 15:36:32 +0800 Subject: [PATCH 2/9] chore: additional code comments for `AlterKind::ModifyColumns` --- src/store-api/src/region_request.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 9a6a187882a7..3bf1e2697c61 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -333,7 +333,9 @@ pub enum AlterKind { /// Name of columns to drop. names: Vec, }, + /// Modify columns form the region, only fields are allowed to modify. ModifyColumns { + /// Columns to modify. columns: Vec, }, } From 9003afc9876665dfb71f2eb1e482b4cda7579480 Mon Sep 17 00:00:00 2001 From: kould Date: Mon, 22 Apr 2024 13:11:58 +0800 Subject: [PATCH 3/9] fix: add nullable check on `ModifyColumn` --- src/table/src/metadata.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 43400afddea2..008a566cc9c9 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -474,7 +474,7 @@ impl TableMeta { let modify_column_name = &col_to_modify.column_name; if let Some(index) = table_schema.column_index_by_name(modify_column_name) { - let data_type = &table_schema.column_schemas()[index].data_type; + let column = &table_schema.column_schemas()[index]; ensure!( !self.primary_key_indices.contains(&index), @@ -514,7 +514,7 @@ impl TableMeta { ); ensure!( - data_type.can_arrow_type_cast_to(&col_to_modify.target_type), + column.data_type.can_arrow_type_cast_to(&col_to_modify.target_type), error::InvalidAlterRequestSnafu { table: table_name, err: format!( @@ -523,6 +523,17 @@ impl TableMeta { ), } ); + + ensure!( + column.is_nullable(), + error::InvalidAlterRequestSnafu { + table: table_name, + err: format!( + "column '{}' must be nullable to ensure safe conversion.", + col_to_modify.column_name, + ), + } + ); } else { return error::ColumnNotExistsSnafu { column_name: modify_column_name, From 9e31c4e05997ea4330184e9e4696aed180066df2 Mon Sep 17 00:00:00 2001 From: kould Date: Mon, 22 Apr 2024 13:17:44 +0800 Subject: [PATCH 4/9] style: codefmt --- src/table/src/metadata.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 008a566cc9c9..fde46cd6043e 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -514,7 +514,9 @@ impl TableMeta { ); ensure!( - column.data_type.can_arrow_type_cast_to(&col_to_modify.target_type), + column + .data_type + .can_arrow_type_cast_to(&col_to_modify.target_type), error::InvalidAlterRequestSnafu { table: table_name, err: format!( From bb8a866ea9a601fb364dac4299b0a58ff47ca859 Mon Sep 17 00:00:00 2001 From: kould Date: Mon, 22 Apr 2024 19:09:29 +0800 Subject: [PATCH 5/9] style: fix the code based on review suggestions --- src/store-api/src/metadata.rs | 9 ++- src/store-api/src/region_request.rs | 6 +- src/table/src/error.rs | 12 --- src/table/src/metadata.rs | 115 ++++++++++++++-------------- 4 files changed, 64 insertions(+), 78 deletions(-) diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 184c90c645f6..7f4da9f97e27 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -1162,11 +1162,12 @@ mod test { .unwrap(); let metadata = builder.build().unwrap(); check_columns(&metadata, &["a", "b", "f", "c", "d"]); - let b_type = metadata + let b_type = &metadata .column_by_name("b") - .map(|column_meta| column_meta.column_schema.data_type.clone()) - .unwrap(); - assert_eq!(ConcreteDataType::string_datatype(), b_type); + .unwrap() + .column_schema + .data_type; + assert_eq!(ConcreteDataType::string_datatype(), *b_type); } #[test] diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 3bf1e2697c61..9583673ab56b 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -30,7 +30,7 @@ use strum::IntoStaticStr; use crate::logstore::entry; use crate::metadata::{ ColumnMetadata, InvalidRawRegionRequestSnafu, InvalidRegionRequestSnafu, MetadataError, - ModifyColumnNotFoundSnafu, RegionMetadata, Result, + RegionMetadata, Result, }; use crate::path_utils::region_dir; use crate::storage::{ColumnId, RegionId, ScanRequest}; @@ -530,9 +530,9 @@ impl ModifyColumn { let column_meta = metadata .column_by_name(&self.column_name) - .context(ModifyColumnNotFoundSnafu { - column_name: self.column_name.clone(), + .context(InvalidRegionRequestSnafu { region_id: metadata.region_id, + err: format!("column {} not found", self.column_name), })?; ensure!( diff --git a/src/table/src/error.rs b/src/table/src/error.rs index c95756f0d0d5..b67dbac5e2f5 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -91,17 +91,6 @@ pub enum Error { location: Location, }, - #[snafu(display( - "Not allowed to modify index column '{}' from table {}", - column_name, - table_name - ))] - ModifyColumnInIndex { - column_name: String, - table_name: String, - location: Location, - }, - #[snafu(display( "Failed to build column descriptor for table: {}, column: {}", table_name, @@ -158,7 +147,6 @@ impl ErrorExt for Error { | Error::SchemaConversion { .. } | Error::TableProjection { .. } => StatusCode::EngineExecuteQuery, Error::RemoveColumnInIndex { .. } - | Error::ModifyColumnInIndex { .. } | Error::BuildColumnDescriptor { .. } | Error::InvalidAlterRequest { .. } => StatusCode::InvalidArguments, Error::TablesRecordBatch { .. } | Error::DuplicatedExecuteCall { .. } => { diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index fde46cd6043e..c489801fd3fc 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -23,7 +23,7 @@ pub use datatypes::error::{Error as ConvertError, Result as ConvertResult}; use datatypes::schema::{ColumnSchema, RawSchema, Schema, SchemaBuilder, SchemaRef}; use derive_builder::Builder; use serde::{Deserialize, Serialize}; -use snafu::{ensure, ResultExt}; +use snafu::{ensure, OptionExt, ResultExt}; use store_api::storage::{ColumnDescriptor, ColumnDescriptorBuilder, ColumnId, RegionId}; use crate::error::{self, Result}; @@ -467,82 +467,79 @@ impl TableMeta { let table_schema = &self.schema; let mut meta_builder = self.new_meta_builder(); - let mut modify_columns: HashMap<_, _> = HashMap::with_capacity(requests.len()); + let mut modify_columns = HashMap::with_capacity(requests.len()); let timestamp_index = table_schema.timestamp_index(); for col_to_modify in requests { let modify_column_name = &col_to_modify.column_name; - if let Some(index) = table_schema.column_index_by_name(modify_column_name) { - let column = &table_schema.column_schemas()[index]; + let index = table_schema + .column_index_by_name(modify_column_name) + .with_context(|| error::ColumnNotExistsSnafu { + column_name: modify_column_name, + table_name, + })?; - ensure!( - !self.primary_key_indices.contains(&index), - error::ModifyColumnInIndexSnafu { - column_name: modify_column_name, - table_name, - } - ); + let column = &table_schema.column_schemas()[index]; - if let Some(ts_index) = timestamp_index { - // Not allowed to modify column in timestamp index. - ensure!( - index != ts_index, - error::ModifyColumnInIndexSnafu { - column_name: table_schema.column_name_by_index(ts_index), - table_name, - } - ); + ensure!( + !self.primary_key_indices.contains(&index), + error::InvalidAlterRequestSnafu { + table: table_name, + err: format!( + "Not allowed to modify primary key index column '{}'", + column.name + ) } + ); + if let Some(ts_index) = timestamp_index { + // Not allowed to modify column in timestamp index. ensure!( - modify_columns - .insert(&col_to_modify.column_name, col_to_modify) - .is_none(), + index != ts_index, error::InvalidAlterRequestSnafu { table: table_name, - err: format!("modify column {} more than once", col_to_modify.column_name), + err: format!( + "Not allowed to modify timestamp index column '{}'", + column.name + ) } ); + } - ensure!( - table_schema.contains_column(&col_to_modify.column_name), - error::ColumnExistsSnafu { - table_name, - column_name: col_to_modify.column_name.to_string() - }, - ); + ensure!( + modify_columns + .insert(&col_to_modify.column_name, col_to_modify) + .is_none(), + error::InvalidAlterRequestSnafu { + table: table_name, + err: format!("modify column {} more than once", col_to_modify.column_name), + } + ); - ensure!( - column - .data_type - .can_arrow_type_cast_to(&col_to_modify.target_type), - error::InvalidAlterRequestSnafu { - table: table_name, - err: format!( - "column '{}' cannot be cast automatically to type '{}'", - col_to_modify.column_name, col_to_modify.target_type, - ), - } - ); + ensure!( + column + .data_type + .can_arrow_type_cast_to(&col_to_modify.target_type), + error::InvalidAlterRequestSnafu { + table: table_name, + err: format!( + "column '{}' cannot be cast automatically to type '{}'", + col_to_modify.column_name, col_to_modify.target_type, + ), + } + ); - ensure!( - column.is_nullable(), - error::InvalidAlterRequestSnafu { - table: table_name, - err: format!( - "column '{}' must be nullable to ensure safe conversion.", - col_to_modify.column_name, - ), - } - ); - } else { - return error::ColumnNotExistsSnafu { - column_name: modify_column_name, - table_name, + ensure!( + column.is_nullable(), + error::InvalidAlterRequestSnafu { + table: table_name, + err: format!( + "column '{}' must be nullable to ensure safe conversion.", + col_to_modify.column_name, + ), } - .fail()?; - } + ); } // Collect columns after modified. let columns: Vec<_> = table_schema From 1e3fa5b8ff2d5a78c8c2a5be453a013a964af74c Mon Sep 17 00:00:00 2001 From: kould Date: Tue, 23 Apr 2024 18:02:28 +0800 Subject: [PATCH 6/9] style: fix the code based on review suggestions --- src/store-api/src/region_request.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 9583673ab56b..5bc1b6752c3e 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -527,19 +527,15 @@ pub struct ModifyColumn { impl ModifyColumn { /// Returns an error if the column to modify is invalid. pub fn validate(&self, metadata: &RegionMetadata) -> Result<()> { - let column_meta = - metadata - .column_by_name(&self.column_name) - .context(InvalidRegionRequestSnafu { - region_id: metadata.region_id, - err: format!("column {} not found", self.column_name), - })?; + let column_meta = metadata + .column_by_name(&self.column_name) + .with_context(|| InvalidRegionRequestSnafu { + region_id: metadata.region_id, + err: format!("column {} not found", self.column_name), + })?; ensure!( - !matches!( - column_meta.semantic_type, - SemanticType::Timestamp | SemanticType::Tag - ), + matches!(column_meta.semantic_type, SemanticType::Field), InvalidRegionRequestSnafu { region_id: metadata.region_id, err: "'timestamp' or 'tag' column cannot change type".to_string() From 2849fa2be5c7e1f40d3dfa3e65d04eefd6e661ac Mon Sep 17 00:00:00 2001 From: kould Date: Tue, 23 Apr 2024 19:51:00 +0800 Subject: [PATCH 7/9] style: rename `ModifyColumn` -> `ChangeColumnType` --- .../src/ddl/alter_table/update_metadata.rs | 2 +- src/store-api/src/metadata.rs | 22 +++---- src/store-api/src/region_request.rs | 52 ++++++++-------- src/table/src/metadata.rs | 62 ++++++++++--------- src/table/src/requests.rs | 20 ++++-- 5 files changed, 84 insertions(+), 74 deletions(-) diff --git a/src/common/meta/src/ddl/alter_table/update_metadata.rs b/src/common/meta/src/ddl/alter_table/update_metadata.rs index cf9f1f226839..6ef6e2e7fc88 100644 --- a/src/common/meta/src/ddl/alter_table/update_metadata.rs +++ b/src/common/meta/src/ddl/alter_table/update_metadata.rs @@ -51,7 +51,7 @@ impl AlterTableProcedure { AlterKind::RenameTable { new_table_name } => { new_info.name = new_table_name.to_string(); } - AlterKind::DropColumns { .. } | AlterKind::ModifyColumns { .. } => {} + AlterKind::DropColumns { .. } | AlterKind::ChangeColumnTypes { .. } => {} } Ok(new_info) diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 7f4da9f97e27..bdb02e39ef72 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -33,7 +33,7 @@ use serde::de::Error; use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; -use crate::region_request::{AddColumn, AddColumnLocation, AlterKind, ModifyColumn}; +use crate::region_request::{AddColumn, AddColumnLocation, AlterKind, ChangeColumnType}; use crate::storage::consts::is_internal_column; use crate::storage::{ColumnId, RegionId}; @@ -537,7 +537,7 @@ impl RegionMetadataBuilder { match kind { AlterKind::AddColumns { columns } => self.add_columns(columns)?, AlterKind::DropColumns { names } => self.drop_columns(&names), - AlterKind::ModifyColumns { columns } => self.modify_columns(columns), + AlterKind::ChangeColumnTypes { columns } => self.change_columns_type(columns), } Ok(self) } @@ -619,12 +619,12 @@ impl RegionMetadataBuilder { .retain(|col| !name_set.contains(&col.column_schema.name)); } - /// Modifies columns to the metadata if exist. - fn modify_columns(&mut self, columns: Vec) { - let mut modify_map: HashMap<_, _> = columns + /// Changes columns type to the metadata if exist. + fn change_columns_type(&mut self, columns: Vec) { + let mut change_type_map: HashMap<_, _> = columns .into_iter() .map( - |ModifyColumn { + |ChangeColumnType { column_name, target_type, }| (column_name, target_type), @@ -632,7 +632,7 @@ impl RegionMetadataBuilder { .collect(); for column_meta in self.column_metadatas.iter_mut() { - if let Some(target_type) = modify_map.remove(&column_meta.column_schema.name) { + if let Some(target_type) = change_type_map.remove(&column_meta.column_schema.name) { column_meta.column_schema.data_type = target_type; } } @@ -730,8 +730,8 @@ pub enum MetadataError { #[snafu(display("Time index column not found"))] TimeIndexNotFound { location: Location }, - #[snafu(display("Modify column {} not exists in region: {}", column_name, region_id))] - ModifyColumnNotFound { + #[snafu(display("Change column {} not exists in region: {}", column_name, region_id))] + ChangeColumnNotFound { column_name: String, region_id: RegionId, location: Location, @@ -1153,8 +1153,8 @@ mod test { let mut builder = RegionMetadataBuilder::from_existing(metadata); builder - .alter(AlterKind::ModifyColumns { - columns: vec![ModifyColumn { + .alter(AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnType { column_name: "b".to_string(), target_type: ConcreteDataType::string_datatype(), }], diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 5bc1b6752c3e..b98c3951d523 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -333,10 +333,10 @@ pub enum AlterKind { /// Name of columns to drop. names: Vec, }, - /// Modify columns form the region, only fields are allowed to modify. - ModifyColumns { - /// Columns to modify. - columns: Vec, + /// Change columns datatype form the region, only fields are allowed to change. + ChangeColumnTypes { + /// Columns to change. + columns: Vec, }, } @@ -356,9 +356,9 @@ impl AlterKind { Self::validate_column_to_drop(name, metadata)?; } } - AlterKind::ModifyColumns { columns } => { - for col_to_modify in columns { - col_to_modify.validate(metadata)?; + AlterKind::ChangeColumnTypes { columns } => { + for col_to_change in columns { + col_to_change.validate(metadata)?; } } } @@ -375,9 +375,9 @@ impl AlterKind { AlterKind::DropColumns { names } => names .iter() .any(|name| metadata.column_by_name(name).is_some()), - AlterKind::ModifyColumns { columns } => columns + AlterKind::ChangeColumnTypes { columns } => columns .iter() - .any(|col_to_modify| col_to_modify.need_alter(metadata)), + .any(|col_to_change| col_to_change.need_alter(metadata)), } } @@ -515,17 +515,17 @@ impl TryFrom for AddColumnLocation { } } -/// Modify a column. +/// Change a column's datatype. #[derive(Debug, PartialEq, Eq, Clone)] -pub struct ModifyColumn { +pub struct ChangeColumnType { /// Schema of the column to modify. pub column_name: String, - /// Column will be modified to this type. + /// Column will be changed to this type. pub target_type: ConcreteDataType, } -impl ModifyColumn { - /// Returns an error if the column to modify is invalid. +impl ChangeColumnType { + /// Returns an error if the column's datatype to change is invalid. pub fn validate(&self, metadata: &RegionMetadata) -> Result<()> { let column_meta = metadata .column_by_name(&self.column_name) @@ -558,7 +558,7 @@ impl ModifyColumn { Ok(()) } - /// Returns true if no column to modify to the region. + /// Returns true if no column's datatype to change to the region. pub fn need_alter(&self, metadata: &RegionMetadata) -> bool { debug_assert!(self.validate(metadata).is_ok()); metadata.column_by_name(&self.column_name).is_some() @@ -864,10 +864,10 @@ mod tests { } #[test] - fn test_validate_modify_column() { + fn test_validate_change_column_type() { let metadata = new_metadata(); - AlterKind::ModifyColumns { - columns: vec![ModifyColumn { + AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnType { column_name: "xxxx".to_string(), target_type: ConcreteDataType::string_datatype(), }], @@ -875,8 +875,8 @@ mod tests { .validate(&metadata) .unwrap_err(); - AlterKind::ModifyColumns { - columns: vec![ModifyColumn { + AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnType { column_name: "field_1".to_string(), target_type: ConcreteDataType::date_datatype(), }], @@ -884,8 +884,8 @@ mod tests { .validate(&metadata) .unwrap_err(); - AlterKind::ModifyColumns { - columns: vec![ModifyColumn { + AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnType { column_name: "ts".to_string(), target_type: ConcreteDataType::date_datatype(), }], @@ -893,8 +893,8 @@ mod tests { .validate(&metadata) .unwrap_err(); - AlterKind::ModifyColumns { - columns: vec![ModifyColumn { + AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnType { column_name: "tag_0".to_string(), target_type: ConcreteDataType::date_datatype(), }], @@ -902,8 +902,8 @@ mod tests { .validate(&metadata) .unwrap_err(); - let kind = AlterKind::ModifyColumns { - columns: vec![ModifyColumn { + let kind = AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnType { column_name: "field_0".to_string(), target_type: ConcreteDataType::int32_datatype(), }], diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index c489801fd3fc..10efb1e0b244 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -27,7 +27,7 @@ use snafu::{ensure, OptionExt, ResultExt}; use store_api::storage::{ColumnDescriptor, ColumnDescriptorBuilder, ColumnId, RegionId}; use crate::error::{self, Result}; -use crate::requests::{AddColumnRequest, AlterKind, ModifyColumnRequest, TableOptions}; +use crate::requests::{AddColumnRequest, AlterKind, ChangeColumnTypeRequest, TableOptions}; pub type TableId = u32; pub type TableVersion = u64; @@ -195,7 +195,9 @@ impl TableMeta { self.add_columns(table_name, columns, add_if_not_exists) } AlterKind::DropColumns { names } => self.remove_columns(table_name, names), - AlterKind::ModifyColumns { columns } => self.modify_columns(table_name, columns), + AlterKind::ChangeColumnTypes { columns } => { + self.change_column_types(table_name, columns) + } // No need to rebuild table meta when renaming tables. AlterKind::RenameTable { .. } => { let mut meta_builder = TableMetaBuilder::default(); @@ -459,24 +461,24 @@ impl TableMeta { Ok(meta_builder) } - fn modify_columns( + fn change_column_types( &self, table_name: &str, - requests: &[ModifyColumnRequest], + requests: &[ChangeColumnTypeRequest], ) -> Result { let table_schema = &self.schema; let mut meta_builder = self.new_meta_builder(); - let mut modify_columns = HashMap::with_capacity(requests.len()); + let mut change_column_types = HashMap::with_capacity(requests.len()); let timestamp_index = table_schema.timestamp_index(); - for col_to_modify in requests { - let modify_column_name = &col_to_modify.column_name; + for col_to_change in requests { + let change_column_name = &col_to_change.column_name; let index = table_schema - .column_index_by_name(modify_column_name) + .column_index_by_name(change_column_name) .with_context(|| error::ColumnNotExistsSnafu { - column_name: modify_column_name, + column_name: change_column_name, table_name, })?; @@ -487,20 +489,20 @@ impl TableMeta { error::InvalidAlterRequestSnafu { table: table_name, err: format!( - "Not allowed to modify primary key index column '{}'", + "Not allowed to change primary key index column '{}'", column.name ) } ); if let Some(ts_index) = timestamp_index { - // Not allowed to modify column in timestamp index. + // Not allowed to change column datatype in timestamp index. ensure!( index != ts_index, error::InvalidAlterRequestSnafu { table: table_name, err: format!( - "Not allowed to modify timestamp index column '{}'", + "Not allowed to change timestamp index column '{}' datatype", column.name ) } @@ -508,24 +510,24 @@ impl TableMeta { } ensure!( - modify_columns - .insert(&col_to_modify.column_name, col_to_modify) + change_column_types + .insert(&col_to_change.column_name, col_to_change) .is_none(), error::InvalidAlterRequestSnafu { table: table_name, - err: format!("modify column {} more than once", col_to_modify.column_name), + err: format!("change column datatype {} more than once", col_to_change.column_name), } ); ensure!( column .data_type - .can_arrow_type_cast_to(&col_to_modify.target_type), + .can_arrow_type_cast_to(&col_to_change.target_type), error::InvalidAlterRequestSnafu { table: table_name, err: format!( "column '{}' cannot be cast automatically to type '{}'", - col_to_modify.column_name, col_to_modify.target_type, + col_to_change.column_name, col_to_change.target_type, ), } ); @@ -536,19 +538,19 @@ impl TableMeta { table: table_name, err: format!( "column '{}' must be nullable to ensure safe conversion.", - col_to_modify.column_name, + col_to_change.column_name, ), } ); } - // Collect columns after modified. + // Collect columns after changed. let columns: Vec<_> = table_schema .column_schemas() .iter() .cloned() .map(|mut column| { - if let Some(modify_column) = modify_columns.get(&column.name) { - column.data_type = modify_column.target_type.clone(); + if let Some(change_column) = change_column_types.get(&column.name) { + column.data_type = change_column.target_type.clone(); } column }) @@ -570,7 +572,7 @@ impl TableMeta { .collect(); error::SchemaBuildSnafu { - msg: format!("Table {table_name} cannot modify columns {column_names:?}"), + msg: format!("Table {table_name} cannot change datatype with columns {column_names:?}"), } })?; @@ -1151,7 +1153,7 @@ mod tests { } #[test] - fn test_modify_unknown_column() { + fn test_change_unknown_column_data_type() { let schema = Arc::new(new_test_schema()); let meta = TableMetaBuilder::default() .schema(schema) @@ -1161,8 +1163,8 @@ mod tests { .build() .unwrap(); - let alter_kind = AlterKind::ModifyColumns { - columns: vec![ModifyColumnRequest { + let alter_kind = AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnTypeRequest { column_name: "unknown".to_string(), target_type: ConcreteDataType::string_datatype(), }], @@ -1210,7 +1212,7 @@ mod tests { } #[test] - fn test_modify_key_column() { + fn test_change_key_column_data_type() { let schema = Arc::new(new_test_schema()); let meta = TableMetaBuilder::default() .schema(schema) @@ -1221,8 +1223,8 @@ mod tests { .unwrap(); // Remove column in primary key. - let alter_kind = AlterKind::ModifyColumns { - columns: vec![ModifyColumnRequest { + let alter_kind = AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnTypeRequest { column_name: "col1".to_string(), target_type: ConcreteDataType::string_datatype(), }], @@ -1235,8 +1237,8 @@ mod tests { assert_eq!(StatusCode::InvalidArguments, err.status_code()); // Remove timestamp column. - let alter_kind = AlterKind::ModifyColumns { - columns: vec![ModifyColumnRequest { + let alter_kind = AlterKind::ChangeColumnTypes { + columns: vec![ChangeColumnTypeRequest { column_name: "ts".to_string(), target_type: ConcreteDataType::string_datatype(), }], diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 3badf4d8fcc7..9472491bcbe9 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -165,19 +165,27 @@ pub struct AddColumnRequest { pub location: Option, } -/// Modify column request +/// Change column datatype request #[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ModifyColumnRequest { +pub struct ChangeColumnTypeRequest { pub column_name: String, pub target_type: ConcreteDataType, } #[derive(Debug, Clone, Serialize, Deserialize)] pub enum AlterKind { - AddColumns { columns: Vec }, - DropColumns { names: Vec }, - ModifyColumns { columns: Vec }, - RenameTable { new_table_name: String }, + AddColumns { + columns: Vec, + }, + DropColumns { + names: Vec, + }, + ChangeColumnTypes { + columns: Vec, + }, + RenameTable { + new_table_name: String, + }, } #[derive(Debug)] From d81de1f81c07451e7f9654780e314414bfabe353 Mon Sep 17 00:00:00 2001 From: kould Date: Tue, 23 Apr 2024 19:54:30 +0800 Subject: [PATCH 8/9] style: code fmt --- src/table/src/metadata.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 10efb1e0b244..6e91b37fd84f 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -515,7 +515,10 @@ impl TableMeta { .is_none(), error::InvalidAlterRequestSnafu { table: table_name, - err: format!("change column datatype {} more than once", col_to_change.column_name), + err: format!( + "change column datatype {} more than once", + col_to_change.column_name + ), } ); @@ -572,7 +575,9 @@ impl TableMeta { .collect(); error::SchemaBuildSnafu { - msg: format!("Table {table_name} cannot change datatype with columns {column_names:?}"), + msg: format!( + "Table {table_name} cannot change datatype with columns {column_names:?}" + ), } })?; From d871718e6501ffd953daacfec352cad20b96d144 Mon Sep 17 00:00:00 2001 From: kould Date: Tue, 23 Apr 2024 22:45:05 +0800 Subject: [PATCH 9/9] style: `change_columns_type` -> `change_column_types` --- src/store-api/src/metadata.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index bdb02e39ef72..5874f23da491 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -537,7 +537,7 @@ impl RegionMetadataBuilder { match kind { AlterKind::AddColumns { columns } => self.add_columns(columns)?, AlterKind::DropColumns { names } => self.drop_columns(&names), - AlterKind::ChangeColumnTypes { columns } => self.change_columns_type(columns), + AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns), } Ok(self) } @@ -620,7 +620,7 @@ impl RegionMetadataBuilder { } /// Changes columns type to the metadata if exist. - fn change_columns_type(&mut self, columns: Vec) { + fn change_column_types(&mut self, columns: Vec) { let mut change_type_map: HashMap<_, _> = columns .into_iter() .map(