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 <[email protected]> * Apply suggestions from code review Co-authored-by: Weny Xu <[email protected]> --------- Co-authored-by: jeremyhi <[email protected]> Co-authored-by: Weny Xu <[email protected]>
GreptimeTeam · Nov 20, 2024 · 55ced9a · 55ced9a
1 parent 3633f25
commit 55ced9a
Show file tree

Hide file tree

Showing 15 changed files with 319 additions and 99 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git 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
@@ -104,7 +104,7 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result<Alter
                 .collect::<std::result::Result<Vec<_>, _>>()
                 .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<Alter
                 case_sensitive: c.case_sensitive,
             },
         },
+        Kind::UnsetColumnFulltext(c) => 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -76,11 +76,13 @@ pub enum AlterTableOperation {
     DropColumn { name: Ident },
     /// `RENAME <new_table_name>`
     RenameTable { new_table_name: String },
-    /// `MODIFY COLUMN <column_name> [SET | UNSET] FULLTEXT [WITH <options>]`
-    ChangeColumnFulltext {
+    /// `MODIFY COLUMN <column_name> SET FULLTEXT [WITH <options>]`
+    SetColumnFulltext {
         column_name: Ident,
         options: FulltextOptions,
     },
+    /// `MODIFY COLUMN <column_name> 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!();
+            }
+        }
     }
 }