From 72fa166f333f5cd734439ea92fd5cd8eba5bf1c7 Mon Sep 17 00:00:00 2001 From: CookiePieWw Date: Wed, 13 Nov 2024 12:51:42 +0800 Subject: [PATCH 1/3] refactor: use UNSERT instead of enable --- src/sql/src/parsers/alter_parser.rs | 141 +++++++++++++----- .../alter/change_col_fulltext_options.result | 4 +- .../alter/change_col_fulltext_options.sql | 4 +- 3 files changed, 108 insertions(+), 41 deletions(-) diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 0bd9c56cec79..f368fa9f4376 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -15,8 +15,9 @@ use std::collections::HashMap; use common_query::AddColumnLocation; -use datatypes::schema::COLUMN_FULLTEXT_CHANGE_OPT_KEY_ENABLE; +use datatypes::schema::{FulltextOptions, COLUMN_FULLTEXT_CHANGE_OPT_KEY_ENABLE}; use snafu::{ensure, ResultExt}; +use sqlparser::ast::Ident; use sqlparser::keywords::Keyword; use sqlparser::parser::{Parser, ParserError}; use sqlparser::tokenizer::Token; @@ -28,10 +29,6 @@ use crate::statements::alter::{AlterTable, AlterTableOperation, ChangeTableOptio use crate::statements::statement::Statement; use crate::util::parse_option_string; -fn validate_column_fulltext_alter_option(key: &str) -> bool { - key == COLUMN_FULLTEXT_CHANGE_OPT_KEY_ENABLE || validate_column_fulltext_create_option(key) -} - impl ParserContext<'_> { pub(crate) fn parse_alter(&mut self) -> Result { let alter_table = self.parse_alter_table()?; @@ -153,40 +150,73 @@ impl ParserContext<'_> { .context(error::SyntaxSnafu)?, ); - if self.parser.parse_keyword(Keyword::SET) { - self.parser - .expect_keyword(Keyword::FULLTEXT) - .context(error::SyntaxSnafu)?; - - let options = self - .parser - .parse_options(Keyword::WITH) - .context(error::SyntaxSnafu)? - .into_iter() - .map(parse_option_string) - .collect::>>()?; - - for key in options.keys() { - ensure!( - validate_column_fulltext_alter_option(key), - InvalidColumnOptionSnafu { - name: column_name.to_string(), - msg: format!("invalid FULLTEXT option: {key}"), - } - ); + match self.parser.peek_token().token { + Token::Word(w) => { + if w.value.eq_ignore_ascii_case("UNSET") { + let _ = self.parser.next_token(); + + self.parser + .expect_keyword(Keyword::FULLTEXT) + .context(error::SyntaxSnafu)?; + + Ok(AlterTableOperation::ChangeColumnFulltext { + column_name, + options: FulltextOptions { + enable: false, + ..Default::default() + }, + }) + } else if w.keyword == Keyword::SET { + self.parse_alter_column_fulltext(column_name) + } else { + let data_type = self.parser.parse_data_type().context(error::SyntaxSnafu)?; + Ok(AlterTableOperation::ChangeColumnType { + column_name, + target_type: data_type, + }) + } } + _ => self.expected( + "SET or UNSET or data type after MODIFY COLUMN", + self.parser.peek_token(), + )?, + } + } - Ok(AlterTableOperation::ChangeColumnFulltext { - column_name, - options: options.try_into().context(SetFulltextOptionSnafu)?, - }) - } else { - let target_type = self.parser.parse_data_type().context(error::SyntaxSnafu)?; - Ok(AlterTableOperation::ChangeColumnType { - column_name, - target_type, - }) + fn parse_alter_column_fulltext(&mut self, column_name: Ident) -> Result { + let _ = self.parser.next_token(); + + self.parser + .expect_keyword(Keyword::FULLTEXT) + .context(error::SyntaxSnafu)?; + + let mut options = self + .parser + .parse_options(Keyword::WITH) + .context(error::SyntaxSnafu)? + .into_iter() + .map(parse_option_string) + .collect::>>()?; + + for key in options.keys() { + ensure!( + validate_column_fulltext_create_option(key), + InvalidColumnOptionSnafu { + name: column_name.to_string(), + msg: format!("invalid FULLTEXT option: {key}"), + } + ); } + + options.insert( + COLUMN_FULLTEXT_CHANGE_OPT_KEY_ENABLE.to_string(), + "true".to_string(), + ); + + Ok(AlterTableOperation::ChangeColumnFulltext { + column_name, + options: options.try_into().context(SetFulltextOptionSnafu)?, + }) } } @@ -557,7 +587,7 @@ mod tests { #[test] fn test_parse_alter_column_fulltext() { - let sql = "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT WITH(enable='true',analyzer='English',case_sensitive='false')"; + let sql = "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT WITH(analyzer='English',case_sensitive='false')"; let mut result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) .unwrap(); @@ -595,6 +625,43 @@ mod tests { _ => unreachable!(), } + let sql = "ALTER TABLE test_table MODIFY COLUMN a UNSET FULLTEXT"; + let mut result = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(1, result.len()); + let statement = result.remove(0); + assert_matches!(statement, Statement::Alter { .. }); + match statement { + Statement::Alter(alter_table) => { + assert_eq!("test_table", alter_table.table_name().0[0].value); + + let alter_operation = alter_table.alter_operation(); + assert_matches!( + 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 + ); + } + _ => unreachable!(), + } + } + _ => unreachable!(), + } + let invalid_sql = "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT WITH('abcd'='true')"; let result = ParserContext::create_with_dialect( invalid_sql, 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 f9af6e9288e2..8e684b5c2adf 100644 --- a/tests/cases/standalone/common/alter/change_col_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_col_fulltext_options.result @@ -94,7 +94,7 @@ SHOW CREATE TABLE test; | | ) | +-------+---------------------------------------------------------------------------------------+ -ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(enable = 'false'); +ALTER TABLE test MODIFY COLUMN message UNSET FULLTEXT; Affected Rows: 0 @@ -140,7 +140,7 @@ ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', c Error: 1004(InvalidArguments), Invalid column option, column name: message, error: FULLTEXT index options already enabled -ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(enable = 'false'); +ALTER TABLE test MODIFY COLUMN message UNSET FULLTEXT; 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 82b25235b779..8d7c82faa445 100644 --- a/tests/cases/standalone/common/alter/change_col_fulltext_options.sql +++ b/tests/cases/standalone/common/alter/change_col_fulltext_options.sql @@ -29,7 +29,7 @@ SELECT * FROM test WHERE MATCHES(message, 'hello'); -- SQLNESS ARG restart=true SHOW CREATE TABLE test; -ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(enable = 'false'); +ALTER TABLE test MODIFY COLUMN message UNSET FULLTEXT; SHOW CREATE TABLE test; @@ -39,7 +39,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); -ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(enable = 'false'); +ALTER TABLE test MODIFY COLUMN message UNSET FULLTEXT; SHOW CREATE TABLE test; From 6a31926c1bded37e9d2de738e337cf2c10618750 Mon Sep 17 00:00:00 2001 From: CookiePieWw Date: Wed, 13 Nov 2024 13:14:59 +0800 Subject: [PATCH 2/3] fix: test --- src/sql/src/statements/alter.rs | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 29efb3f568ea..d93c5c17bbce 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -126,10 +126,16 @@ impl Display for AlterTableOperation { AlterTableOperation::ChangeColumnFulltext { column_name, options, - } => write!( - f, - r#"MODIFY COLUMN {column_name} SET FULLTEXT WITH({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(()) + } + }, } } } @@ -243,7 +249,7 @@ ALTER TABLE monitor RENAME monitor_new"#, } } - let sql = "ALTER TABLE monitor MODIFY COLUMN a SET FULLTEXT WITH(enable='true',analyzer='English',case_sensitive='false')"; + let sql = "ALTER TABLE monitor MODIFY COLUMN a SET FULLTEXT WITH(analyzer='English',case_sensitive='false')"; let stmts = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) .unwrap(); @@ -255,7 +261,7 @@ ALTER TABLE monitor RENAME monitor_new"#, let new_sql = format!("\n{}", set); assert_eq!( r#" -ALTER TABLE monitor MODIFY COLUMN a SET FULLTEXT WITH(enable=true, analyzer=English, case_sensitive=false)"#, +ALTER TABLE monitor MODIFY COLUMN a SET FULLTEXT WITH(analyzer=English, case_sensitive=false)"#, &new_sql ); } From a84b99105b89085f71aa5073b37b60871db7e6f6 Mon Sep 17 00:00:00 2001 From: CookiePieWw Date: Wed, 13 Nov 2024 14:53:41 +0800 Subject: [PATCH 3/3] chore: comment --- src/sql/src/statements/alter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index d93c5c17bbce..180b1ee65c42 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -76,7 +76,7 @@ pub enum AlterTableOperation { DropColumn { name: Ident }, /// `RENAME ` RenameTable { new_table_name: String }, - /// `MODIFY COLUMN SET FULLTEXT [WITH ]` + /// `MODIFY COLUMN [SET | UNSET] FULLTEXT [WITH ]` ChangeColumnFulltext { column_name: Ident, options: FulltextOptions,