feat: add unset table options support (#5034)

* feat: add unset table options support

* test: add tests

* chore: update greptime-proto

* chore: add comments

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 = "58f8f18215e800b240bae81ac45678d75417d7f5" }
+greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "0b90ddc7eb2e99ce15d1d62c5d41f76a139c5c28" }
 hex = "0.4"
 humantime = "2.1"
 humantime-serde = "1.1"

src/common/grpc-expr/src/alter.rs

@@ -23,13 +23,13 @@ use api::v1::{
 use common_query::AddColumnLocation;
 use datatypes::schema::{ColumnSchema, FulltextOptions, RawSchema};
 use snafu::{ensure, OptionExt, ResultExt};
-use store_api::region_request::ChangeOption;
+use store_api::region_request::{SetRegionOption, UnsetRegionOption};
 use table::metadata::TableId;
 use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest, ModifyColumnTypeRequest};
 
 use crate::error::{
-    InvalidChangeFulltextOptionRequestSnafu, InvalidChangeTableOptionRequestSnafu,
-    InvalidColumnDefSnafu, MissingFieldSnafu, MissingTimestampColumnSnafu, Result,
+    InvalidColumnDefSnafu, InvalidSetFulltextOptionRequestSnafu, InvalidSetTableOptionRequestSnafu,
+    InvalidUnsetTableOptionRequestSnafu, MissingFieldSnafu, MissingTimestampColumnSnafu, Result,
     UnknownLocationTypeSnafu,
 };
 
@@ -95,22 +95,30 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result<Alter
         Kind::RenameTable(RenameTable { new_table_name }) => {
             AlterKind::RenameTable { new_table_name }
         }
-        Kind::ChangeTableOptions(api::v1::ChangeTableOptions {
-            change_table_options,
-        }) => AlterKind::ChangeTableOptions {
-            options: change_table_options
-                .iter()
-                .map(ChangeOption::try_from)
-                .collect::<std::result::Result<Vec<_>, _>>()
-                .context(InvalidChangeTableOptionRequestSnafu)?,
-        },
+        Kind::SetTableOptions(api::v1::SetTableOptions { table_options }) => {
+            AlterKind::SetTableOptions {
+                options: table_options
+                    .iter()
+                    .map(SetRegionOption::try_from)
+                    .collect::<std::result::Result<Vec<_>, _>>()
+                    .context(InvalidSetTableOptionRequestSnafu)?,
+            }
+        }
+        Kind::UnsetTableOptions(api::v1::UnsetTableOptions { keys }) => {
+            AlterKind::UnsetTableOptions {
+                keys: keys
+                    .iter()
+                    .map(|key| UnsetRegionOption::try_from(key.as_str()))
+                    .collect::<std::result::Result<Vec<_>, _>>()
+                    .context(InvalidUnsetTableOptionRequestSnafu)?,
+            }
+        }
         Kind::SetColumnFulltext(c) => AlterKind::SetColumnFulltext {
             column_name: c.column_name,
             options: FulltextOptions {
                 enable: c.enable,
                 analyzer: as_fulltext_option(
-                    Analyzer::try_from(c.analyzer)
-                        .context(InvalidChangeFulltextOptionRequestSnafu)?,
+                    Analyzer::try_from(c.analyzer).context(InvalidSetFulltextOptionRequestSnafu)?,
                 ),
                 case_sensitive: c.case_sensitive,
             },

src/common/grpc-expr/src/error.rs

@@ -120,14 +120,20 @@ pub enum Error {
         location: Location,
     },
 
-    #[snafu(display("Invalid change table option request"))]
-    InvalidChangeTableOptionRequest {
+    #[snafu(display("Invalid set table option request"))]
+    InvalidSetTableOptionRequest {
         #[snafu(source)]
         error: MetadataError,
     },
 
-    #[snafu(display("Invalid change fulltext option request"))]
-    InvalidChangeFulltextOptionRequest {
+    #[snafu(display("Invalid unset table option request"))]
+    InvalidUnsetTableOptionRequest {
+        #[snafu(source)]
+        error: MetadataError,
+    },
+
+    #[snafu(display("Invalid set fulltext option request"))]
+    InvalidSetFulltextOptionRequest {
         #[snafu(implicit)]
         location: Location,
         #[snafu(source)]
@@ -156,8 +162,9 @@ impl ErrorExt for Error {
             Error::UnknownColumnDataType { .. } | Error::InvalidFulltextColumnType { .. } => {
                 StatusCode::InvalidArguments
             }
-            Error::InvalidChangeTableOptionRequest { .. }
-            | Error::InvalidChangeFulltextOptionRequest { .. } => StatusCode::InvalidArguments,
+            Error::InvalidSetTableOptionRequest { .. }
+            | Error::InvalidUnsetTableOptionRequest { .. }
+            | Error::InvalidSetFulltextOptionRequest { .. } => StatusCode::InvalidArguments,
         }
     }
 

src/common/meta/src/ddl/alter_table/region_request.rs

@@ -106,7 +106,8 @@ fn create_proto_alter_kind(
             })))
         }
         Kind::RenameTable(_) => Ok(None),
-        Kind::ChangeTableOptions(v) => Ok(Some(alter_request::Kind::ChangeTableOptions(v.clone()))),
+        Kind::SetTableOptions(v) => Ok(Some(alter_request::Kind::SetTableOptions(v.clone()))),
+        Kind::UnsetTableOptions(v) => Ok(Some(alter_request::Kind::UnsetTableOptions(v.clone()))),
         Kind::SetColumnFulltext(v) => Ok(Some(alter_request::Kind::SetColumnFulltext(v.clone()))),
         Kind::UnsetColumnFulltext(v) => {
             Ok(Some(alter_request::Kind::UnsetColumnFulltext(v.clone())))

src/common/meta/src/ddl/alter_table/update_metadata.rs

@@ -53,7 +53,8 @@ impl AlterTableProcedure {
             }
             AlterKind::DropColumns { .. }
             | AlterKind::ModifyColumnTypes { .. }
-            | AlterKind::ChangeTableOptions { .. }
+            | AlterKind::SetTableOptions { .. }
+            | AlterKind::UnsetTableOptions { .. }
             | AlterKind::SetColumnFulltext { .. }
             | AlterKind::UnsetColumnFulltext { .. } => {}
         }

src/common/meta/src/ddl/tests/alter_table.rs

@@ -19,8 +19,8 @@ use std::sync::Arc;
 use api::v1::alter_expr::Kind;
 use api::v1::region::{region_request, RegionRequest};
 use api::v1::{
-    AddColumn, AddColumns, AlterExpr, ChangeTableOption, ChangeTableOptions, ColumnDataType,
-    ColumnDef as PbColumnDef, DropColumn, DropColumns, SemanticType,
+    AddColumn, AddColumns, AlterExpr, ColumnDataType, ColumnDef as PbColumnDef, DropColumn,
+    DropColumns, SemanticType, SetTableOptions, TableOption,
 };
 use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME};
 use common_error::ext::ErrorExt;
@@ -389,8 +389,8 @@ async fn test_on_update_table_options() {
             catalog_name: DEFAULT_CATALOG_NAME.to_string(),
             schema_name: DEFAULT_SCHEMA_NAME.to_string(),
             table_name: table_name.to_string(),
-            kind: Some(Kind::ChangeTableOptions(ChangeTableOptions {
-                change_table_options: vec![ChangeTableOption {
+            kind: Some(Kind::SetTableOptions(SetTableOptions {
+                table_options: vec![TableOption {
                     key: TTL_KEY.to_string(),
                     value: "1d".to_string(),
                 }],

src/mito2/src/worker/handle_alter.rs

@@ -22,11 +22,11 @@ use common_telemetry::{debug, info};
 use humantime_serde::re::humantime;
 use snafu::ResultExt;
 use store_api::metadata::{
-    InvalidRegionOptionChangeRequestSnafu, MetadataError, RegionMetadata, RegionMetadataBuilder,
+    InvalidSetRegionOptionRequestSnafu, MetadataError, RegionMetadata, RegionMetadataBuilder,
     RegionMetadataRef,
 };
 use store_api::mito_engine_options;
-use store_api::region_request::{AlterKind, ChangeOption, RegionAlterRequest};
+use store_api::region_request::{AlterKind, RegionAlterRequest, SetRegionOption};
 use store_api::storage::RegionId;
 
 use crate::error::{
@@ -58,12 +58,29 @@ impl<S> RegionWorkerLoop<S> {
         let version = region.version();
 
         // fast path for memory state changes like options.
-        if let AlterKind::ChangeRegionOptions { options } = request.kind {
-            match self.handle_alter_region_options(region, version, options) {
-                Ok(_) => sender.send(Ok(0)),
-                Err(e) => sender.send(Err(e).context(InvalidMetadataSnafu)),
+        match request.kind {
+            AlterKind::SetRegionOptions { options } => {
+                match self.handle_alter_region_options(region, version, options) {
+                    Ok(_) => sender.send(Ok(0)),
+                    Err(e) => sender.send(Err(e).context(InvalidMetadataSnafu)),
+                }
+                return;
             }
-            return;
+            AlterKind::UnsetRegionOptions { keys } => {
+                // Converts the keys to SetRegionOption.
+                //
+                // It passes an empty string to achieve the purpose of unset
+                match self.handle_alter_region_options(
+                    region,
+                    version,
+                    keys.iter().map(Into::into).collect(),
+                ) {
+                    Ok(_) => sender.send(Ok(0)),
+                    Err(e) => sender.send(Err(e).context(InvalidMetadataSnafu)),
+                }
+                return;
+            }
+            _ => {}
         }
 
         if version.metadata.schema_version != request.schema_version {
@@ -162,12 +179,12 @@ impl<S> RegionWorkerLoop<S> {
         &mut self,
         region: MitoRegionRef,
         version: VersionRef,
-        options: Vec<ChangeOption>,
+        options: Vec<SetRegionOption>,
     ) -> std::result::Result<(), MetadataError> {
         let mut current_options = version.options.clone();
         for option in options {
             match option {
-                ChangeOption::TTL(new_ttl) => {
+                SetRegionOption::TTL(new_ttl) => {
                     info!(
                         "Update region ttl: {}, previous: {:?} new: {:?}",
                         region.region_id, current_options.ttl, new_ttl
@@ -178,9 +195,9 @@ impl<S> RegionWorkerLoop<S> {
                         current_options.ttl = Some(new_ttl);
                     }
                 }
-                ChangeOption::Twsc(key, value) => {
+                SetRegionOption::Twsc(key, value) => {
                     let Twcs(options) = &mut current_options.compaction;
-                    change_twcs_options(
+                    set_twcs_options(
                         options,
                         &TwcsOptions::default(),
                         &key,
@@ -213,7 +230,7 @@ fn metadata_after_alteration(
     Ok(Arc::new(new_meta))
 }
 
-fn change_twcs_options(
+fn set_twcs_options(
     options: &mut TwcsOptions,
     default_option: &TwcsOptions,
     key: &str,
@@ -245,30 +262,30 @@ fn change_twcs_options(
             options.max_inactive_window_files = files;
         }
         mito_engine_options::TWCS_MAX_OUTPUT_FILE_SIZE => {
-            let size =
-                if value.is_empty() {
-                    default_option.max_output_file_size
-                } else {
-                    Some(ReadableSize::from_str(value).map_err(|_| {
-                        InvalidRegionOptionChangeRequestSnafu { key, value }.build()
-                    })?)
-                };
+            let size = if value.is_empty() {
+                default_option.max_output_file_size
+            } else {
+                Some(
+                    ReadableSize::from_str(value)
+                        .map_err(|_| InvalidSetRegionOptionRequestSnafu { key, value }.build())?,
+                )
+            };
             log_option_update(region_id, key, options.max_output_file_size, size);
             options.max_output_file_size = size;
         }
         mito_engine_options::TWCS_TIME_WINDOW => {
-            let window =
-                if value.is_empty() {
-                    default_option.time_window
-                } else {
-                    Some(humantime::parse_duration(value).map_err(|_| {
-                        InvalidRegionOptionChangeRequestSnafu { key, value }.build()
-                    })?)
-                };
+            let window = if value.is_empty() {
+                default_option.time_window
+            } else {
+                Some(
+                    humantime::parse_duration(value)
+                        .map_err(|_| InvalidSetRegionOptionRequestSnafu { key, value }.build())?,
+                )
+            };
             log_option_update(region_id, key, options.time_window, window);
             options.time_window = window;
         }
-        _ => return InvalidRegionOptionChangeRequestSnafu { key, value }.fail(),
+        _ => return InvalidSetRegionOptionRequestSnafu { key, value }.fail(),
     }
     Ok(())
 }
@@ -283,7 +300,7 @@ fn parse_usize_with_default(
     } else {
         value
             .parse::<usize>()
-            .map_err(|_| InvalidRegionOptionChangeRequestSnafu { key, value }.build())
+            .map_err(|_| InvalidSetRegionOptionRequestSnafu { key, value }.build())
     }
 }
 

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, ChangeTableOptions, ColumnDataType,
-    ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropColumn,
-    DropColumns, ExpireAfter, ModifyColumnType, ModifyColumnTypes, RenameTable, SemanticType,
-    SetColumnFulltext, TableName, UnsetColumnFulltext,
+    AddColumn, AddColumns, AlterExpr, Analyzer, ColumnDataType, ColumnDataTypeExtension,
+    CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropColumn, DropColumns, ExpireAfter,
+    ModifyColumnType, ModifyColumnTypes, RenameTable, SemanticType, SetColumnFulltext,
+    SetTableOptions, TableName, UnsetColumnFulltext, UnsetTableOptions,
 };
 use common_error::ext::BoxedError;
 use common_grpc_expr::util::ColumnExpr;
@@ -526,11 +526,14 @@ pub(crate) fn to_alter_expr(
         AlterTableOperation::RenameTable { new_table_name } => Kind::RenameTable(RenameTable {
             new_table_name: new_table_name.to_string(),
         }),
-        AlterTableOperation::ChangeTableOptions { options } => {
-            Kind::ChangeTableOptions(ChangeTableOptions {
-                change_table_options: options.into_iter().map(Into::into).collect(),
+        AlterTableOperation::SetTableOptions { options } => {
+            Kind::SetTableOptions(SetTableOptions {
+                table_options: options.into_iter().map(Into::into).collect(),
             })
         }
+        AlterTableOperation::UnsetTableOptions { keys } => {
+            Kind::UnsetTableOptions(UnsetTableOptions { keys })
+        }
         AlterTableOperation::SetColumnFulltext {
             column_name,
             options,