From 55ced9aa7178c08d7c613fc9d034098446f0e251 Mon Sep 17 00:00:00 2001 From: Yohan Wal <1035325592@qq.com> Date: Wed, 20 Nov 2024 14:02:51 +0800 Subject: [PATCH] refactor: split up different stmts (#4997) * refactor: set and unset * chore: error message * fix: reset Cargo.lock * Apply suggestions from code review Co-authored-by: jeremyhi * Apply suggestions from code review Co-authored-by: Weny Xu --------- Co-authored-by: jeremyhi Co-authored-by: Weny Xu --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/common/grpc-expr/src/alter.rs | 5 +- .../src/ddl/alter_table/region_request.rs | 5 +- .../src/ddl/alter_table/update_metadata.rs | 3 +- src/mito2/src/engine/alter_test.rs | 2 +- src/operator/src/expr_factory.rs | 17 ++- src/sql/src/parsers/alter_parser.rs | 42 ++---- src/sql/src/statements/alter.rs | 45 +++++-- src/store-api/src/metadata.rs | 119 +++++++++++++++-- src/store-api/src/region_request.rs | 37 +++++- src/table/src/metadata.rs | 120 +++++++++++++++--- src/table/src/requests.rs | 5 +- .../alter/change_col_fulltext_options.result | 10 +- .../alter/change_col_fulltext_options.sql | 4 +- 15 files changed, 319 insertions(+), 99 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index d0f9464054..7d2ec67baf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4596,7 +4596,7 @@ dependencies = [ [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=fb4e2146b1ea8816148304f1f258e7a73736a905#fb4e2146b1ea8816148304f1f258e7a73736a905" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=58f8f18215e800b240bae81ac45678d75417d7f5#58f8f18215e800b240bae81ac45678d75417d7f5" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index c683c54412..c8a665f825 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -122,7 +122,7 @@ etcd-client = "0.13" fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "fb4e2146b1ea8816148304f1f258e7a73736a905" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "58f8f18215e800b240bae81ac45678d75417d7f5" } hex = "0.4" humantime = "2.1" humantime-serde = "1.1" diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index 5627e4fd69..7b4c040679 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -104,7 +104,7 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result, _>>() .context(InvalidChangeTableOptionRequestSnafu)?, }, - Kind::ChangeColumnFulltext(c) => AlterKind::ChangeColumnFulltext { + Kind::SetColumnFulltext(c) => AlterKind::SetColumnFulltext { column_name: c.column_name, options: FulltextOptions { enable: c.enable, @@ -115,6 +115,9 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result AlterKind::UnsetColumnFulltext { + column_name: c.column_name, + }, }; let request = AlterTableRequest { diff --git a/src/common/meta/src/ddl/alter_table/region_request.rs b/src/common/meta/src/ddl/alter_table/region_request.rs index 8d8092cf64..fd523bbfe2 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -107,8 +107,9 @@ fn create_proto_alter_kind( } Kind::RenameTable(_) => Ok(None), Kind::ChangeTableOptions(v) => Ok(Some(alter_request::Kind::ChangeTableOptions(v.clone()))), - Kind::ChangeColumnFulltext(v) => { - Ok(Some(alter_request::Kind::ChangeColumnFulltext(v.clone()))) + Kind::SetColumnFulltext(v) => Ok(Some(alter_request::Kind::SetColumnFulltext(v.clone()))), + Kind::UnsetColumnFulltext(v) => { + Ok(Some(alter_request::Kind::UnsetColumnFulltext(v.clone()))) } } } 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 06fd1832f7..c37fb55da4 100644 --- a/src/common/meta/src/ddl/alter_table/update_metadata.rs +++ b/src/common/meta/src/ddl/alter_table/update_metadata.rs @@ -54,7 +54,8 @@ impl AlterTableProcedure { AlterKind::DropColumns { .. } | AlterKind::ModifyColumnTypes { .. } | AlterKind::ChangeTableOptions { .. } - | AlterKind::ChangeColumnFulltext { .. } => {} + | AlterKind::SetColumnFulltext { .. } + | AlterKind::UnsetColumnFulltext { .. } => {} } Ok(new_info) diff --git a/src/mito2/src/engine/alter_test.rs b/src/mito2/src/engine/alter_test.rs index 6f6649022a..68ae72d885 100644 --- a/src/mito2/src/engine/alter_test.rs +++ b/src/mito2/src/engine/alter_test.rs @@ -71,7 +71,7 @@ fn add_tag1() -> RegionAlterRequest { fn alter_column_fulltext_options() -> RegionAlterRequest { RegionAlterRequest { schema_version: 0, - kind: AlterKind::ChangeColumnFulltext { + kind: AlterKind::SetColumnFulltext { column_name: "tag_0".to_string(), options: FulltextOptions { enable: true, diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 540561b65b..7ac69fa309 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -18,10 +18,10 @@ use api::helper::ColumnDataTypeWrapper; use api::v1::alter_expr::Kind; use api::v1::column_def::options_from_column_schema; use api::v1::{ - AddColumn, AddColumns, AlterExpr, Analyzer, ChangeColumnFulltext, ChangeTableOptions, - ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, - DropColumn, DropColumns, ExpireAfter, ModifyColumnType, ModifyColumnTypes, RenameTable, - SemanticType, TableName, + AddColumn, AddColumns, AlterExpr, Analyzer, ChangeTableOptions, ColumnDataType, + ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropColumn, + DropColumns, ExpireAfter, ModifyColumnType, ModifyColumnTypes, RenameTable, SemanticType, + SetColumnFulltext, TableName, UnsetColumnFulltext, }; use common_error::ext::BoxedError; use common_grpc_expr::util::ColumnExpr; @@ -531,10 +531,10 @@ pub(crate) fn to_alter_expr( change_table_options: options.into_iter().map(Into::into).collect(), }) } - AlterTableOperation::ChangeColumnFulltext { + AlterTableOperation::SetColumnFulltext { column_name, options, - } => Kind::ChangeColumnFulltext(ChangeColumnFulltext { + } => Kind::SetColumnFulltext(SetColumnFulltext { column_name: column_name.value, enable: options.enable, analyzer: match options.analyzer { @@ -543,6 +543,11 @@ pub(crate) fn to_alter_expr( }, case_sensitive: options.case_sensitive, }), + AlterTableOperation::UnsetColumnFulltext { column_name } => { + Kind::UnsetColumnFulltext(UnsetColumnFulltext { + column_name: column_name.value, + }) + } }; Ok(AlterExpr { diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index dbf0fc3a6b..cc88f22257 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -15,7 +15,7 @@ use std::collections::HashMap; use common_query::AddColumnLocation; -use datatypes::schema::{FulltextOptions, COLUMN_FULLTEXT_CHANGE_OPT_KEY_ENABLE}; +use datatypes::schema::COLUMN_FULLTEXT_CHANGE_OPT_KEY_ENABLE; use snafu::{ensure, ResultExt}; use sqlparser::ast::Ident; use sqlparser::keywords::Keyword; @@ -159,13 +159,7 @@ impl ParserContext<'_> { .expect_keyword(Keyword::FULLTEXT) .context(error::SyntaxSnafu)?; - Ok(AlterTableOperation::ChangeColumnFulltext { - column_name, - options: FulltextOptions { - enable: false, - ..Default::default() - }, - }) + Ok(AlterTableOperation::UnsetColumnFulltext { column_name }) } else if w.keyword == Keyword::SET { self.parse_alter_column_fulltext(column_name) } else { @@ -213,7 +207,7 @@ impl ParserContext<'_> { "true".to_string(), ); - Ok(AlterTableOperation::ChangeColumnFulltext { + Ok(AlterTableOperation::SetColumnFulltext { column_name, options: options.try_into().context(SetFulltextOptionSnafu)?, }) @@ -602,10 +596,10 @@ mod tests { let alter_operation = alter_table.alter_operation(); assert_matches!( alter_operation, - AlterTableOperation::ChangeColumnFulltext { .. } + AlterTableOperation::SetColumnFulltext { .. } ); match alter_operation { - AlterTableOperation::ChangeColumnFulltext { + AlterTableOperation::SetColumnFulltext { column_name, options, } => { @@ -637,27 +631,15 @@ mod tests { assert_eq!("test_table", alter_table.table_name().0[0].value); let alter_operation = alter_table.alter_operation(); - assert_matches!( + assert_eq!( alter_operation, - AlterTableOperation::ChangeColumnFulltext { .. } - ); - match alter_operation { - AlterTableOperation::ChangeColumnFulltext { - column_name, - options, - } => { - assert_eq!("a", column_name.value); - assert_eq!( - FulltextOptions { - enable: false, - analyzer: FulltextAnalyzer::English, - case_sensitive: false - }, - *options - ); + &AlterTableOperation::UnsetColumnFulltext { + column_name: Ident { + value: "a".to_string(), + quote_style: None + } } - _ => unreachable!(), - } + ); } _ => unreachable!(), } diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index b75ec728e1..30e6fda0e7 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -76,11 +76,13 @@ pub enum AlterTableOperation { DropColumn { name: Ident }, /// `RENAME ` RenameTable { new_table_name: String }, - /// `MODIFY COLUMN [SET | UNSET] FULLTEXT [WITH ]` - ChangeColumnFulltext { + /// `MODIFY COLUMN SET FULLTEXT [WITH ]` + SetColumnFulltext { column_name: Ident, options: FulltextOptions, }, + /// `MODIFY COLUMN UNSET FULLTEXT` + UnsetColumnFulltext { column_name: Ident }, } impl Display for AlterTableOperation { @@ -123,19 +125,15 @@ impl Display for AlterTableOperation { Ok(()) } - AlterTableOperation::ChangeColumnFulltext { + AlterTableOperation::SetColumnFulltext { column_name, options, - } => match options.enable { - true => { - write!(f, "MODIFY COLUMN {column_name} SET FULLTEXT WITH(analyzer={0}, case_sensitive={1})", options.analyzer, options.case_sensitive)?; - Ok(()) - } - false => { - write!(f, "MODIFY COLUMN {column_name} UNSET FULLTEXT")?; - Ok(()) - } - }, + } => { + write!(f, "MODIFY COLUMN {column_name} SET FULLTEXT WITH(analyzer={0}, case_sensitive={1})", options.analyzer, options.case_sensitive) + } + AlterTableOperation::UnsetColumnFulltext { column_name } => { + write!(f, "MODIFY COLUMN {column_name} UNSET FULLTEXT") + } } } } @@ -269,5 +267,26 @@ ALTER TABLE monitor MODIFY COLUMN a SET FULLTEXT WITH(analyzer=English, case_sen unreachable!(); } } + + let sql = "ALTER TABLE monitor MODIFY COLUMN a UNSET FULLTEXT"; + let stmts = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(1, stmts.len()); + assert_matches!(&stmts[0], Statement::Alter { .. }); + + match &stmts[0] { + Statement::Alter(set) => { + let new_sql = format!("\n{}", set); + assert_eq!( + r#" +ALTER TABLE monitor MODIFY COLUMN a UNSET FULLTEXT"#, + &new_sql + ); + } + _ => { + unreachable!(); + } + } } } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 3a06b111d8..136cf61033 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -553,10 +553,13 @@ impl RegionMetadataBuilder { AlterKind::AddColumns { columns } => self.add_columns(columns)?, AlterKind::DropColumns { names } => self.drop_columns(&names), AlterKind::ModifyColumnTypes { columns } => self.modify_column_types(columns), - AlterKind::ChangeColumnFulltext { + AlterKind::SetColumnFulltext { column_name, options, - } => self.change_column_fulltext_options(column_name, options)?, + } => self.change_column_fulltext_options(column_name, true, Some(options))?, + AlterKind::UnsetColumnFulltext { column_name } => { + self.change_column_fulltext_options(column_name, false, None)? + } AlterKind::ChangeRegionOptions { options: _ } => { // nothing to be done with RegionMetadata } @@ -663,7 +666,8 @@ impl RegionMetadataBuilder { fn change_column_fulltext_options( &mut self, column_name: String, - options: FulltextOptions, + enable: bool, + options: Option, ) -> Result<()> { for column_meta in self.column_metadatas.iter_mut() { if column_meta.column_schema.name == column_name { @@ -682,18 +686,26 @@ impl RegionMetadataBuilder { column_name: column_name.clone(), })?; - // Don't allow to enable fulltext options if it is already enabled. - if current_fulltext_options.is_some_and(|o| o.enable) && options.enable { - return InvalidColumnOptionSnafu { + if enable { + ensure!( + options.is_some(), + InvalidColumnOptionSnafu { + column_name, + msg: "FULLTEXT index options must be provided", + } + ); + set_column_fulltext_options( + column_meta, column_name, - msg: "FULLTEXT index options already enabled".to_string(), - } - .fail(); + options.unwrap(), + current_fulltext_options, + )?; } else { - column_meta - .column_schema - .set_fulltext_options(&options) - .context(SetFulltextOptionsSnafu { column_name })?; + unset_column_fulltext_options( + column_meta, + column_name, + current_fulltext_options, + )?; } break; } @@ -860,6 +872,64 @@ impl ErrorExt for MetadataError { } } +fn set_column_fulltext_options( + column_meta: &mut ColumnMetadata, + column_name: String, + options: FulltextOptions, + current_options: Option, +) -> Result<()> { + if let Some(current_options) = current_options { + ensure!( + !current_options.enable, + InvalidColumnOptionSnafu { + column_name, + msg: "FULLTEXT index already enabled".to_string(), + } + ); + + ensure!( + current_options.analyzer == options.analyzer + && current_options.case_sensitive == options.case_sensitive, + InvalidColumnOptionSnafu { + column_name, + msg: format!("Cannot change analyzer or case_sensitive if FULLTEXT index is set before. Previous analyzer: {}, previous case_sensitive: {}", + current_options.analyzer, current_options.case_sensitive), + } + ); + } + + column_meta + .column_schema + .set_fulltext_options(&options) + .context(SetFulltextOptionsSnafu { column_name })?; + + Ok(()) +} + +fn unset_column_fulltext_options( + column_meta: &mut ColumnMetadata, + column_name: String, + current_options: Option, +) -> Result<()> { + if let Some(mut current_options) = current_options + && current_options.enable + { + current_options.enable = false; + column_meta + .column_schema + .set_fulltext_options(¤t_options) + .context(SetFulltextOptionsSnafu { column_name })?; + } else { + return InvalidColumnOptionSnafu { + column_name, + msg: "FULLTEXT index already disabled", + } + .fail(); + } + + Ok(()) +} + #[cfg(test)] mod test { use datatypes::prelude::ConcreteDataType; @@ -1283,7 +1353,7 @@ mod test { let mut builder = RegionMetadataBuilder::from_existing(metadata); builder - .alter(AlterKind::ChangeColumnFulltext { + .alter(AlterKind::SetColumnFulltext { column_name: "b".to_string(), options: FulltextOptions { enable: true, @@ -1306,6 +1376,27 @@ mod test { a_fulltext_options.analyzer ); assert!(a_fulltext_options.case_sensitive); + + let mut builder = RegionMetadataBuilder::from_existing(metadata); + builder + .alter(AlterKind::UnsetColumnFulltext { + column_name: "b".to_string(), + }) + .unwrap(); + let metadata = builder.build().unwrap(); + let a_fulltext_options = metadata + .column_by_name("b") + .unwrap() + .column_schema + .fulltext_options() + .unwrap() + .unwrap(); + assert!(!a_fulltext_options.enable); + assert_eq!( + datatypes::schema::FulltextAnalyzer::Chinese, + a_fulltext_options.analyzer + ); + assert!(a_fulltext_options.case_sensitive); } #[test] diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 4d742a1426..b8a309b44e 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -413,12 +413,17 @@ pub enum AlterKind { columns: Vec, }, /// Change region options. - ChangeRegionOptions { options: Vec }, + ChangeRegionOptions { + options: Vec, + }, /// Change fulltext index options. - ChangeColumnFulltext { + SetColumnFulltext { column_name: String, options: FulltextOptions, }, + UnsetColumnFulltext { + column_name: String, + }, } impl AlterKind { @@ -443,7 +448,8 @@ impl AlterKind { } } AlterKind::ChangeRegionOptions { .. } => {} - AlterKind::ChangeColumnFulltext { column_name, .. } => { + AlterKind::SetColumnFulltext { column_name, .. } + | AlterKind::UnsetColumnFulltext { column_name } => { Self::validate_column_fulltext_option(column_name, metadata)?; } } @@ -468,7 +474,10 @@ impl AlterKind { // todo: we need to check if ttl has ever changed. true } - AlterKind::ChangeColumnFulltext { column_name, .. } => { + AlterKind::SetColumnFulltext { column_name, .. } => { + metadata.column_by_name(column_name).is_some() + } + AlterKind::UnsetColumnFulltext { column_name } => { metadata.column_by_name(column_name).is_some() } } @@ -550,7 +559,7 @@ impl TryFrom for AlterKind { .collect::>>()?, } } - alter_request::Kind::ChangeColumnFulltext(x) => AlterKind::ChangeColumnFulltext { + alter_request::Kind::SetColumnFulltext(x) => AlterKind::SetColumnFulltext { column_name: x.column_name.clone(), options: FulltextOptions { enable: x.enable, @@ -560,6 +569,9 @@ impl TryFrom for AlterKind { case_sensitive: x.case_sensitive, }, }, + alter_request::Kind::UnsetColumnFulltext(x) => AlterKind::UnsetColumnFulltext { + column_name: x.column_name, + }, }; Ok(alter_kind) @@ -1217,8 +1229,8 @@ mod tests { } #[test] - fn test_validate_change_column_fulltext_options() { - let kind = AlterKind::ChangeColumnFulltext { + fn test_validate_modify_column_fulltext_options() { + let kind = AlterKind::SetColumnFulltext { column_name: "tag_0".to_string(), options: FulltextOptions { enable: true, @@ -1233,5 +1245,16 @@ mod tests { let mut metadata = new_metadata(); metadata.schema_version = 1; request.validate(&metadata).unwrap(); + + let kind = AlterKind::UnsetColumnFulltext { + column_name: "tag_0".to_string(), + }; + let request = RegionAlterRequest { + schema_version: 1, + kind, + }; + let mut metadata = new_metadata(); + metadata.schema_version = 1; + request.validate(&metadata).unwrap(); } } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index af13fe40d5..fd775bd76f 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -207,10 +207,13 @@ impl TableMeta { // No need to rebuild table meta when renaming tables. AlterKind::RenameTable { .. } => Ok(self.new_meta_builder()), AlterKind::ChangeTableOptions { options } => self.change_table_options(options), - AlterKind::ChangeColumnFulltext { + AlterKind::SetColumnFulltext { column_name, options, - } => self.change_column_fulltext_options(table_name, column_name, options), + } => self.change_column_fulltext_options(table_name, column_name, true, Some(options)), + AlterKind::UnsetColumnFulltext { column_name } => { + self.change_column_fulltext_options(table_name, column_name, false, None) + } } } @@ -255,7 +258,8 @@ impl TableMeta { &self, table_name: &str, column_name: &str, - options: &FulltextOptions, + enable: bool, + options: Option<&FulltextOptions>, ) -> Result { let table_schema = &self.schema; let mut meta_builder = self.new_meta_builder(); @@ -279,21 +283,31 @@ impl TableMeta { .fulltext_options() .context(error::SetFulltextOptionsSnafu { column_name })?; - ensure!( - !(current_fulltext_options.is_some_and(|o| o.enable) && options.enable), - error::InvalidColumnOptionSnafu { - column_name, - msg: "FULLTEXT index options already enabled", - } - ); - let mut columns = Vec::with_capacity(table_schema.column_schemas().len()); for column_schema in table_schema.column_schemas() { if column_schema.name == column_name { let mut new_column_schema = column_schema.clone(); - new_column_schema - .set_fulltext_options(options) - .context(error::SetFulltextOptionsSnafu { column_name })?; + if enable { + ensure!( + options.is_some(), + error::InvalidColumnOptionSnafu { + column_name, + msg: "FULLTEXT index options must be provided", + } + ); + set_column_fulltext_options( + &mut new_column_schema, + column_name, + options.unwrap(), + current_fulltext_options.clone(), + )? + } else { + unset_column_fulltext_options( + &mut new_column_schema, + column_name, + current_fulltext_options.clone(), + )? + } columns.push(new_column_schema); } else { columns.push(column_schema.clone()); @@ -987,6 +1001,63 @@ impl TryFrom for TableInfo { } } +fn set_column_fulltext_options( + column_schema: &mut ColumnSchema, + column_name: &str, + options: &FulltextOptions, + current_options: Option, +) -> Result<()> { + if let Some(current_options) = current_options { + ensure!( + !current_options.enable, + error::InvalidColumnOptionSnafu { + column_name, + msg: "FULLTEXT index already enabled", + } + ); + + ensure!( + current_options.analyzer == options.analyzer + && current_options.case_sensitive == options.case_sensitive, + error::InvalidColumnOptionSnafu { + column_name, + msg: format!("Cannot change analyzer or case_sensitive if FULLTEXT index is set before. Previous analyzer: {}, previous case_sensitive: {}", + current_options.analyzer, current_options.case_sensitive), + } + ); + } + + column_schema + .set_fulltext_options(options) + .context(error::SetFulltextOptionsSnafu { column_name })?; + + Ok(()) +} + +fn unset_column_fulltext_options( + column_schema: &mut ColumnSchema, + column_name: &str, + current_options: Option, +) -> Result<()> { + ensure!( + current_options + .clone() + .is_some_and(|options| options.enable), + error::InvalidColumnOptionSnafu { + column_name, + msg: "FULLTEXT index already disabled".to_string(), + } + ); + + let mut options = current_options.unwrap(); + options.enable = false; + column_schema + .set_fulltext_options(&options) + .context(error::SetFulltextOptionsSnafu { column_name })?; + + Ok(()) +} + #[cfg(test)] mod tests { use common_error::ext::ErrorExt; @@ -1434,7 +1505,7 @@ mod tests { } #[test] - fn test_alter_column_fulltext_options() { + fn test_modify_column_fulltext_options() { let schema = Arc::new(new_test_schema()); let meta = TableMetaBuilder::default() .schema(schema) @@ -1444,7 +1515,7 @@ mod tests { .build() .unwrap(); - let alter_kind = AlterKind::ChangeColumnFulltext { + let alter_kind = AlterKind::SetColumnFulltext { column_name: "col1".to_string(), options: FulltextOptions::default(), }; @@ -1461,7 +1532,7 @@ mod tests { let new_meta = add_columns_to_meta_with_location(&meta); assert_eq!(meta.region_numbers, new_meta.region_numbers); - let alter_kind = AlterKind::ChangeColumnFulltext { + let alter_kind = AlterKind::SetColumnFulltext { column_name: "my_tag_first".to_string(), options: FulltextOptions { enable: true, @@ -1485,5 +1556,20 @@ mod tests { fulltext_options.analyzer ); assert!(fulltext_options.case_sensitive); + + let alter_kind = AlterKind::UnsetColumnFulltext { + column_name: "my_tag_first".to_string(), + }; + let new_meta = new_meta + .builder_with_alter_kind("my_table", &alter_kind, false) + .unwrap() + .build() + .unwrap(); + let column_schema = new_meta + .schema + .column_schema_by_name("my_tag_first") + .unwrap(); + let fulltext_options = column_schema.fulltext_options().unwrap().unwrap(); + assert!(!fulltext_options.enable); } } diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 43adc51994..0e9cad7f05 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -216,10 +216,13 @@ pub enum AlterKind { ChangeTableOptions { options: Vec, }, - ChangeColumnFulltext { + SetColumnFulltext { column_name: String, options: FulltextOptions, }, + UnsetColumnFulltext { + column_name: String, + }, } // #[derive(Debug, Clone, Serialize, Deserialize)] diff --git a/tests/cases/standalone/common/alter/change_col_fulltext_options.result b/tests/cases/standalone/common/alter/change_col_fulltext_options.result index 8e684b5c2a..46bd510462 100644 --- a/tests/cases/standalone/common/alter/change_col_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_col_fulltext_options.result @@ -115,7 +115,7 @@ SHOW CREATE TABLE test; | | ) | +-------+-------------------------------------+ -ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true'); Affected Rows: 0 @@ -125,7 +125,7 @@ SHOW CREATE TABLE test; | Table | Create Table | +-------+---------------------------------------------------------------------------------------+ | test | CREATE TABLE IF NOT EXISTS "test" ( | -| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'), | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true'), | | | "time" TIMESTAMP(3) NOT NULL, | | | TIME INDEX ("time") | | | ) | @@ -138,7 +138,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); -Error: 1004(InvalidArguments), Invalid column option, column name: message, error: FULLTEXT index options already enabled +Error: 1004(InvalidArguments), Invalid column option, column name: message, error: FULLTEXT index already enabled ALTER TABLE test MODIFY COLUMN message UNSET FULLTEXT; @@ -173,6 +173,10 @@ ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case Error: 1004(InvalidArguments), Invalid column option, column name: time, error: FULLTEXT index only supports string type +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'); + +Error: 1004(InvalidArguments), Invalid column option, column name: message, error: Cannot change analyzer or case_sensitive if FULLTEXT index is set before. Previous analyzer: Chinese, previous case_sensitive: true + DROP TABLE test; Affected Rows: 0 diff --git a/tests/cases/standalone/common/alter/change_col_fulltext_options.sql b/tests/cases/standalone/common/alter/change_col_fulltext_options.sql index 8d7c82faa4..88c8b3b180 100644 --- a/tests/cases/standalone/common/alter/change_col_fulltext_options.sql +++ b/tests/cases/standalone/common/alter/change_col_fulltext_options.sql @@ -33,7 +33,7 @@ ALTER TABLE test MODIFY COLUMN message UNSET FULLTEXT; SHOW CREATE TABLE test; -ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true'); SHOW CREATE TABLE test; @@ -49,4 +49,6 @@ ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', c ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'); + DROP TABLE test;