From 6ae407c596be7005d0e6c489b5002b49323b02b2 Mon Sep 17 00:00:00 2001 From: irenjj Date: Thu, 19 Sep 2024 23:57:12 +0800 Subject: [PATCH] fix --- src/api/src/v1/column_def.rs | 2 +- src/datatypes/src/error.rs | 10 +++- src/datatypes/src/schema.rs | 5 +- src/datatypes/src/schema/column_schema.rs | 20 +++++--- src/mito2/src/sst/index.rs | 2 +- .../src/sst/index/fulltext_index/creator.rs | 6 +-- src/query/src/sql/show_create_table.rs | 2 +- src/sql/src/statements.rs | 2 +- src/store-api/src/metadata.rs | 48 ++++++++++------- src/store-api/src/region_request.rs | 29 ++++++++--- src/table/src/error.rs | 12 ++--- src/table/src/metadata.rs | 51 +++++++++++-------- .../alter/change_fulltext_options.result | 8 +-- 13 files changed, 125 insertions(+), 72 deletions(-) diff --git a/src/api/src/v1/column_def.rs b/src/api/src/v1/column_def.rs index b4d3425215c8..9b05ad3c2f9c 100644 --- a/src/api/src/v1/column_def.rs +++ b/src/api/src/v1/column_def.rs @@ -148,7 +148,7 @@ mod tests { assert!(options.is_none()); let schema = ColumnSchema::new("test", ConcreteDataType::string_datatype(), true) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::English, case_sensitive: false, diff --git a/src/datatypes/src/error.rs b/src/datatypes/src/error.rs index 5a255dc0a644..4efc1a647185 100644 --- a/src/datatypes/src/error.rs +++ b/src/datatypes/src/error.rs @@ -205,6 +205,13 @@ pub enum Error { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Invalid data type {} for fulltext", data_type))] + InvalidFulltextDataType { + data_type: String, + #[snafu(implicit)] + location: Location, + }, } impl ErrorExt for Error { @@ -222,7 +229,8 @@ impl ErrorExt for Error { | DefaultValueType { .. } | DuplicateMeta { .. } | InvalidTimestampPrecision { .. } - | InvalidPrecisionOrScale { .. } => StatusCode::InvalidArguments, + | InvalidPrecisionOrScale { .. } + | InvalidFulltextDataType { .. } => StatusCode::InvalidArguments, ValueExceedsPrecision { .. } | CastType { .. } diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index 1ab87239cb49..fd4ccf7caab3 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -320,7 +320,6 @@ fn validate_timestamp_index(column_schemas: &[ColumnSchema], timestamp_index: us /// - `None` if the options are invalid or do not contain full-text related keys. pub fn parse_fulltext_options(options: &HashMap) -> Option { let mut fulltext = FulltextOptions::default(); - let mut is_fulltext_key_exist = false; // Check and parse the "analyzer" option if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { @@ -338,7 +337,6 @@ pub fn parse_fulltext_options(options: &HashMap) -> Option) -> Option Result { + pub fn with_fulltext_options(mut self, options: &FulltextOptions) -> Result { self.set_fulltext_options(options)?; Ok(self) } - pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> { - if self.data_type == ConcreteDataType::string_datatype() { - self.metadata.insert( - FULLTEXT_KEY.to_string(), - serde_json::to_string(&options).context(error::SerializeSnafu)?, - ); + pub fn set_fulltext_options(&mut self, options: &FulltextOptions) -> Result<()> { + if self.data_type != ConcreteDataType::string_datatype() { + return error::InvalidFulltextDataTypeSnafu { + data_type: self.data_type.to_string(), + } + .fail(); } + + self.metadata.insert( + FULLTEXT_KEY.to_string(), + serde_json::to_string(options).context(error::SerializeSnafu)?, + ); + Ok(()) } } diff --git a/src/mito2/src/sst/index.rs b/src/mito2/src/sst/index.rs index b73bd0df7ee6..f9a6913c5f65 100644 --- a/src/mito2/src/sst/index.rs +++ b/src/mito2/src/sst/index.rs @@ -349,7 +349,7 @@ mod tests { if with_fulltext { let column_schema = ColumnSchema::new("text", ConcreteDataType::string_datatype(), true) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, ..Default::default() }) diff --git a/src/mito2/src/sst/index/fulltext_index/creator.rs b/src/mito2/src/sst/index/fulltext_index/creator.rs index 416e39d9dd5e..7bcb89fcc3b7 100644 --- a/src/mito2/src/sst/index/fulltext_index/creator.rs +++ b/src/mito2/src/sst/index/fulltext_index/creator.rs @@ -329,7 +329,7 @@ mod tests { ConcreteDataType::string_datatype(), true, ) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::English, case_sensitive: true, @@ -344,7 +344,7 @@ mod tests { ConcreteDataType::string_datatype(), true, ) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::English, case_sensitive: false, @@ -359,7 +359,7 @@ mod tests { ConcreteDataType::string_datatype(), true, ) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::Chinese, case_sensitive: false, diff --git a/src/query/src/sql/show_create_table.rs b/src/query/src/sql/show_create_table.rs index 2c560bd36013..35b61503808d 100644 --- a/src/query/src/sql/show_create_table.rs +++ b/src/query/src/sql/show_create_table.rs @@ -222,7 +222,7 @@ mod tests { ColumnSchema::new("cpu", ConcreteDataType::float64_datatype(), true), ColumnSchema::new("disk", ConcreteDataType::float32_datatype(), true), ColumnSchema::new("msg", ConcreteDataType::string_datatype(), true) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, ..Default::default() }) diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs index 7568fcb24f2a..7e2fcd36f161 100644 --- a/src/sql/src/statements.rs +++ b/src/sql/src/statements.rs @@ -463,7 +463,7 @@ pub fn column_to_schema( if let Some(options) = column.extensions.build_fulltext_options()? { column_schema = column_schema - .with_fulltext_options(options) + .with_fulltext_options(&options) .context(SetFulltextOptionSnafu)?; } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 32f956b7d67c..3907807daa49 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -524,9 +524,9 @@ impl RegionMetadataBuilder { AlterKind::DropColumns { names } => self.drop_columns(&names), AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns), AlterKind::ChangeColumnFulltext { - column_name: _, + column_name, options, - } => self.change_column_fulltext(options)?, + } => self.change_column_fulltext(column_name, options)?, } Ok(self) } @@ -628,22 +628,29 @@ impl RegionMetadataBuilder { } /// Changes column fulltext option. - fn change_column_fulltext(&mut self, options: HashMap) -> Result<()> { + fn change_column_fulltext( + &mut self, + column_name: String, + options: HashMap, + ) -> Result<()> { for column_meta in self.column_metadatas.iter_mut() { - let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() { - f - } else { - FulltextOptions::default() - }; - - if let Some(f) = parse_fulltext_options(&options) { - fulltext = f; - } + if column_name.eq(&column_meta.column_schema.name) { + let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() + { + f + } else { + FulltextOptions::default() + }; - column_meta - .column_schema - .set_fulltext_options(fulltext) - .expect("set fulltext"); + if let Some(f) = parse_fulltext_options(&options) { + fulltext = f; + } + + column_meta + .column_schema + .set_fulltext_options(&fulltext) + .expect("set fulltext"); + } } Ok(()) @@ -765,11 +772,18 @@ pub enum MetadataError { location: Location, }, - #[snafu(display("Invalid fulltext options, column: {}", column_name))] + #[snafu(display( + "Invalid fulltext options, column: {}, key: {}, value: {}", + column_name, + key, + value + ))] InvalidFulltextOptions { #[snafu(implicit)] location: Location, column_name: String, + key: String, + value: String, }, } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 7e87f2f5f17a..69d90bfbef14 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -492,15 +492,20 @@ impl AlterKind { })?; // Validate the full-text options in a simplified manner - for (k, v) in options { - let value_lower = v.to_ascii_lowercase(); - match k.to_ascii_lowercase().as_str() { + for (key, value) in options { + let value_lower = value.to_ascii_lowercase(); + match key.to_ascii_lowercase().as_str() { // Validate the "analyzer" option COLUMN_FULLTEXT_OPT_KEY_ANALYZER => { if !matches!(value_lower.as_str(), "english") && !matches!(value_lower.as_str(), "chinese") { - return InvalidFulltextOptionsSnafu { column_name }.fail(); + return InvalidFulltextOptionsSnafu { + column_name, + key, + value, + } + .fail(); } } // Validate the "case_sensitive" option @@ -508,11 +513,23 @@ impl AlterKind { if !matches!(value_lower.as_str(), "true") && !matches!(value_lower.as_str(), "false") { - return InvalidFulltextOptionsSnafu { column_name }.fail(); + return InvalidFulltextOptionsSnafu { + column_name, + key, + value, + } + .fail(); } } // Return an error for any unknown options - _ => return InvalidFulltextOptionsSnafu { column_name }.fail(), + _ => { + return InvalidFulltextOptionsSnafu { + column_name, + key, + value, + } + .fail() + } } } diff --git a/src/table/src/error.rs b/src/table/src/error.rs index 8488eba9e8cc..11324b5dcb88 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -138,11 +138,12 @@ pub enum Error { location: Location, }, - #[snafu(display("Invalid fulltext options, column: {}", column_name))] - InvalidFulltextOptions { + #[snafu(display("column {} has invalid colum type to set fulltext", column_name))] + InvalidFulltextColumnType { + column_name: String, + source: datatypes::error::Error, #[snafu(implicit)] location: Location, - column_name: String, }, } @@ -160,11 +161,10 @@ impl ErrorExt for Error { Error::ColumnExists { .. } => StatusCode::TableColumnExists, Error::SchemaBuild { source, .. } => source.status_code(), Error::TableOperation { source } => source.status_code(), + Error::InvalidFulltextColumnType { source, .. } => source.status_code(), Error::ColumnNotExists { .. } => StatusCode::TableColumnNotFound, Error::Unsupported { .. } => StatusCode::Unsupported, - Error::ParseTableOption { .. } | Error::InvalidFulltextOptions { .. } => { - StatusCode::InvalidArguments - } + Error::ParseTableOption { .. } => StatusCode::InvalidArguments, Error::MissingTimeIndexColumn { .. } => StatusCode::IllegalState, } } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index af07a9f92b87..4f59f6947205 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -619,7 +619,7 @@ impl TableMeta { fulltext = f; } - let columns: Vec<_> = table_schema + let columns: Result> = table_schema .column_schemas() .iter() .cloned() @@ -627,31 +627,42 @@ impl TableMeta { if column.name == *column_name { column = column .clone() - .with_fulltext_options(fulltext.clone()) - .unwrap(); + .with_fulltext_options(&fulltext) + .context(error::InvalidFulltextColumnTypeSnafu { column_name })?; } - column + Ok(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}"), - })? - .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(|_| error::SchemaBuildSnafu { - msg: format!("Table {table_name} cannot change datatype with columns {column_name}"), - })?; + match columns { + Ok(columns) => { + 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}" + ), + })? + .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(|_| error::SchemaBuildSnafu { + msg: format!( + "Table {table_name} cannot change datatype with columns {column_name}" + ), + })?; - let _ = meta_builder - .schema(Arc::new(new_schema)) - .primary_key_indices(self.primary_key_indices.clone()); + let _ = meta_builder + .schema(Arc::new(new_schema)) + .primary_key_indices(self.primary_key_indices.clone()); - Ok(meta_builder) + Ok(meta_builder) + } + Err(e) => { + return Err(e); + } + } } /// Split requests into different groups using column location info. diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result index 58f7b9bb1177..688e9b8af27f 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -111,7 +111,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'yes'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: message +Error: 1004(InvalidArguments), Invalid fulltext options, column: message, key: case_sensitive, value: yes SHOW CREATE TABLE test; @@ -132,7 +132,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinglish', case_sensitive = 'false'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: message +Error: 1004(InvalidArguments), Invalid fulltext options, column: message, key: analyzer, value: Chinglish SHOW CREATE TABLE test; @@ -153,7 +153,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); -Affected Rows: 0 +Error: 1004(InvalidArguments), Invalid data type TimestampMillisecond for fulltext SHOW CREATE TABLE test; @@ -174,7 +174,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'no'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: time +Error: 1004(InvalidArguments), Invalid data type TimestampMillisecond for fulltext SHOW CREATE TABLE test;