From 14d997e2d16b1c630b26584790b9734fbabfcd7f Mon Sep 17 00:00:00 2001 From: Weny Xu Date: Thu, 21 Nov 2024 11:52:56 +0800 Subject: [PATCH] feat: add unset table options support (#5034) * feat: add unset table options support * test: add tests * chore: update greptime-proto * chore: add comments --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/common/grpc-expr/src/alter.rs | 36 +++-- src/common/grpc-expr/src/error.rs | 19 ++- .../src/ddl/alter_table/region_request.rs | 3 +- .../src/ddl/alter_table/update_metadata.rs | 3 +- src/common/meta/src/ddl/tests/alter_table.rs | 8 +- src/mito2/src/worker/handle_alter.rs | 77 ++++++---- src/operator/src/expr_factory.rs | 17 ++- src/sql/src/parsers/alter_parser.rs | 70 +++++++-- src/sql/src/statements/alter.rs | 39 +++-- src/store-api/src/metadata.rs | 16 +- src/store-api/src/region_request.rs | 138 ++++++++++++++---- src/table/src/metadata.rs | 16 +- src/table/src/requests.rs | 33 +---- .../common/alter/alter_table_options.result | 2 +- .../common/alter/alter_table_options.sql | 2 +- 17 files changed, 329 insertions(+), 154 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3e5ae977cb0c..11ff3881a9eb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4597,7 +4597,7 @@ dependencies = [ [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=58f8f18215e800b240bae81ac45678d75417d7f5#58f8f18215e800b240bae81ac45678d75417d7f5" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=0b90ddc7eb2e99ce15d1d62c5d41f76a139c5c28#0b90ddc7eb2e99ce15d1d62c5d41f76a139c5c28" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index c8a665f82502..e2fec19aa6c9 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 = "58f8f18215e800b240bae81ac45678d75417d7f5" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "0b90ddc7eb2e99ce15d1d62c5d41f76a139c5c28" } 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 7b4c04067950..94cac67d6f91 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/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 { 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::, _>>() - .context(InvalidChangeTableOptionRequestSnafu)?, - }, + Kind::SetTableOptions(api::v1::SetTableOptions { table_options }) => { + AlterKind::SetTableOptions { + options: table_options + .iter() + .map(SetRegionOption::try_from) + .collect::, _>>() + .context(InvalidSetTableOptionRequestSnafu)?, + } + } + Kind::UnsetTableOptions(api::v1::UnsetTableOptions { keys }) => { + AlterKind::UnsetTableOptions { + keys: keys + .iter() + .map(|key| UnsetRegionOption::try_from(key.as_str())) + .collect::, _>>() + .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, }, diff --git a/src/common/grpc-expr/src/error.rs b/src/common/grpc-expr/src/error.rs index c9fb1f949e6a..374dde80d109 100644 --- a/src/common/grpc-expr/src/error.rs +++ b/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, } } 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 fd523bbfe208..4583a78faaeb 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/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()))) 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 c37fb55da4f1..de72b7977aa2 100644 --- a/src/common/meta/src/ddl/alter_table/update_metadata.rs +++ b/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 { .. } => {} } diff --git a/src/common/meta/src/ddl/tests/alter_table.rs b/src/common/meta/src/ddl/tests/alter_table.rs index 7ede16b125fe..7874c3e79835 100644 --- a/src/common/meta/src/ddl/tests/alter_table.rs +++ b/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(), }], diff --git a/src/mito2/src/worker/handle_alter.rs b/src/mito2/src/worker/handle_alter.rs index 8ab7e55bda23..3908cee98be0 100644 --- a/src/mito2/src/worker/handle_alter.rs +++ b/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 RegionWorkerLoop { 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 RegionWorkerLoop { &mut self, region: MitoRegionRef, version: VersionRef, - options: Vec, + options: Vec, ) -> 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 RegionWorkerLoop { 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::() - .map_err(|_| InvalidRegionOptionChangeRequestSnafu { key, value }.build()) + .map_err(|_| InvalidSetRegionOptionRequestSnafu { key, value }.build()) } } diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 7ac69fa3099b..451bd5d5e8e5 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, 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, diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index cc88f2225783..4fa2a3f865bf 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -25,7 +25,7 @@ use sqlparser::tokenizer::Token; use crate::error::{self, InvalidColumnOptionSnafu, Result, SetFulltextOptionSnafu}; use crate::parser::ParserContext; use crate::parsers::utils::validate_column_fulltext_create_option; -use crate::statements::alter::{AlterTable, AlterTableOperation, ChangeTableOption}; +use crate::statements::alter::{AlterTable, AlterTableOperation, TableOption}; use crate::statements::statement::Statement; use crate::util::parse_option_string; @@ -50,6 +50,8 @@ impl ParserContext<'_> { Token::Word(w) => { if w.value.eq_ignore_ascii_case("MODIFY") { self.parse_alter_table_modify()? + } else if w.value.eq_ignore_ascii_case("UNSET") { + self.parse_alter_table_unset()? } else { match w.keyword { Keyword::ADD => self.parse_alter_table_add()?, @@ -87,9 +89,9 @@ impl ParserContext<'_> { .parse_comma_separated(parse_string_options) .context(error::SyntaxSnafu)? .into_iter() - .map(|(key, value)| ChangeTableOption { key, value }) + .map(|(key, value)| TableOption { key, value }) .collect(); - AlterTableOperation::ChangeTableOptions { options } + AlterTableOperation::SetTableOptions { options } } _ => self.expected( "ADD or DROP or MODIFY or RENAME or SET after ALTER TABLE", @@ -103,6 +105,18 @@ impl ParserContext<'_> { Ok(AlterTable::new(table_name, alter_operation)) } + fn parse_alter_table_unset(&mut self) -> Result { + let _ = self.parser.next_token(); + let keys = self + .parser + .parse_comma_separated(parse_string_option_names) + .context(error::SyntaxSnafu)? + .into_iter() + .collect(); + + Ok(AlterTableOperation::UnsetTableOptions { keys }) + } + fn parse_alter_table_add(&mut self) -> Result { let _ = self.parser.next_token(); if let Some(constraint) = self @@ -214,6 +228,7 @@ impl ParserContext<'_> { } } +/// Parses a string literal and an optional string literal value. fn parse_string_options(parser: &mut Parser) -> std::result::Result<(String, String), ParserError> { let name = parser.parse_literal_string()?; parser.expect_token(&Token::Eq)?; @@ -230,6 +245,11 @@ fn parse_string_options(parser: &mut Parser) -> std::result::Result<(String, Str Ok((name, value)) } +/// Parses a comma separated list of string literals. +fn parse_string_option_names(parser: &mut Parser) -> std::result::Result { + parser.parse_literal_string() +} + #[cfg(test)] mod tests { use std::assert_matches::assert_matches; @@ -537,7 +557,7 @@ mod tests { } } - fn check_parse_alter_table(sql: &str, expected: &[(&str, &str)]) { + fn check_parse_alter_table_set_options(sql: &str, expected: &[(&str, &str)]) { let result = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) .unwrap(); @@ -546,7 +566,7 @@ mod tests { unreachable!() }; assert_eq!("test_table", alter.table_name.0[0].value); - let AlterTableOperation::ChangeTableOptions { options } = &alter.alter_operation else { + let AlterTableOperation::SetTableOptions { options } = &alter.alter_operation else { unreachable!() }; @@ -558,18 +578,36 @@ mod tests { assert_eq!(expected, &res); } + fn check_parse_alter_table_unset_options(sql: &str, expected: &[&str]) { + let result = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(1, result.len()); + let Statement::Alter(alter) = &result[0] else { + unreachable!() + }; + assert_eq!("test_table", alter.table_name.0[0].value); + let AlterTableOperation::UnsetTableOptions { keys } = &alter.alter_operation else { + unreachable!() + }; + + assert_eq!(sql, alter.to_string()); + let res = keys.iter().map(|o| o.to_string()).collect::>(); + assert_eq!(expected, &res); + } + #[test] - fn test_parse_alter_column() { - check_parse_alter_table("ALTER TABLE test_table SET 'a'='A'", &[("a", "A")]); - check_parse_alter_table( + fn test_parse_alter_table_set_options() { + check_parse_alter_table_set_options("ALTER TABLE test_table SET 'a'='A'", &[("a", "A")]); + check_parse_alter_table_set_options( "ALTER TABLE test_table SET 'a'='A','b'='B'", &[("a", "A"), ("b", "B")], ); - check_parse_alter_table( + check_parse_alter_table_set_options( "ALTER TABLE test_table SET 'a'='A','b'='B','c'='C'", &[("a", "A"), ("b", "B"), ("c", "C")], ); - check_parse_alter_table("ALTER TABLE test_table SET 'a'=NULL", &[("a", "")]); + check_parse_alter_table_set_options("ALTER TABLE test_table SET 'a'=NULL", &[("a", "")]); ParserContext::create_with_dialect( "ALTER TABLE test_table SET a INTEGER", @@ -579,6 +617,18 @@ mod tests { .unwrap_err(); } + #[test] + fn test_parse_alter_table_unset_options() { + check_parse_alter_table_unset_options("ALTER TABLE test_table UNSET 'a'", &["a"]); + check_parse_alter_table_unset_options("ALTER TABLE test_table UNSET 'a','b'", &["a", "b"]); + ParserContext::create_with_dialect( + "ALTER TABLE test_table UNSET a INTEGER", + &GreptimeDbDialect {}, + ParseOptions::default(), + ) + .unwrap_err(); + } + #[test] fn test_parse_alter_column_fulltext() { let sql = "ALTER TABLE test_table MODIFY COLUMN a SET FULLTEXT WITH(analyzer='English',case_sensitive='false')"; diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 30e6fda0e7bb..543c6e827294 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -71,18 +71,29 @@ pub enum AlterTableOperation { target_type: DataType, }, /// `SET =
` - ChangeTableOptions { options: Vec }, + SetTableOptions { + options: Vec, + }, + UnsetTableOptions { + keys: Vec, + }, /// `DROP COLUMN ` - DropColumn { name: Ident }, + DropColumn { + name: Ident, + }, /// `RENAME ` - RenameTable { new_table_name: String }, + RenameTable { + new_table_name: String, + }, /// `MODIFY COLUMN SET FULLTEXT [WITH ]` SetColumnFulltext { column_name: Ident, options: FulltextOptions, }, /// `MODIFY COLUMN UNSET FULLTEXT` - UnsetColumnFulltext { column_name: Ident }, + UnsetColumnFulltext { + column_name: Ident, + }, } impl Display for AlterTableOperation { @@ -109,10 +120,10 @@ impl Display for AlterTableOperation { } => { write!(f, r#"MODIFY COLUMN {column_name} {target_type}"#) } - AlterTableOperation::ChangeTableOptions { options } => { + AlterTableOperation::SetTableOptions { options } => { let kvs = options .iter() - .map(|ChangeTableOption { key, value }| { + .map(|TableOption { key, value }| { if !value.is_empty() { format!("'{key}'='{value}'") } else { @@ -121,9 +132,11 @@ impl Display for AlterTableOperation { }) .join(","); - write!(f, "SET {kvs}")?; - - Ok(()) + write!(f, "SET {kvs}") + } + AlterTableOperation::UnsetTableOptions { keys } => { + let keys = keys.iter().map(|k| format!("'{k}'")).join(","); + write!(f, "UNSET {keys}") } AlterTableOperation::SetColumnFulltext { column_name, @@ -139,14 +152,14 @@ impl Display for AlterTableOperation { } #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut)] -pub struct ChangeTableOption { +pub struct TableOption { pub key: String, pub value: String, } -impl From for v1::ChangeTableOption { - fn from(c: ChangeTableOption) -> Self { - v1::ChangeTableOption { +impl From for v1::TableOption { + fn from(c: TableOption) -> Self { + v1::TableOption { key: c.key, value: c.value, } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 136cf610330f..57351e19ca99 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -560,7 +560,10 @@ impl RegionMetadataBuilder { AlterKind::UnsetColumnFulltext { column_name } => { self.change_column_fulltext_options(column_name, false, None)? } - AlterKind::ChangeRegionOptions { options: _ } => { + AlterKind::SetRegionOptions { options: _ } => { + // nothing to be done with RegionMetadata + } + AlterKind::UnsetRegionOptions { keys: _ } => { // nothing to be done with RegionMetadata } } @@ -829,14 +832,21 @@ pub enum MetadataError { location: Location, }, - #[snafu(display("Invalid region option change request, key: {}, value: {}", key, value))] - InvalidRegionOptionChangeRequest { + #[snafu(display("Invalid set region option request, key: {}, value: {}", key, value))] + InvalidSetRegionOptionRequest { key: String, value: String, #[snafu(implicit)] location: Location, }, + #[snafu(display("Invalid set region option request, key: {}", key))] + InvalidUnsetRegionOptionRequest { + key: String, + #[snafu(implicit)] + location: Location, + }, + #[snafu(display("Failed to decode protobuf"))] DecodeProto { #[snafu(source)] diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index b8a309b44e22..dcc6a9ef9b49 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -13,7 +13,7 @@ // limitations under the License. use std::collections::HashMap; -use std::fmt; +use std::fmt::{self, Display}; use std::time::Duration; use api::helper::ColumnDataTypeWrapper; @@ -24,7 +24,7 @@ use api::v1::region::{ CompactRequest, CreateRequest, CreateRequests, DeleteRequests, DropRequest, DropRequests, FlushRequest, InsertRequests, OpenRequest, TruncateRequest, }; -use api::v1::{self, Analyzer, ChangeTableOption, Rows, SemanticType}; +use api::v1::{self, Analyzer, Rows, SemanticType, TableOption}; pub use common_base::AffectedRows; use datatypes::data_type::ConcreteDataType; use datatypes::schema::FulltextOptions; @@ -34,8 +34,8 @@ use strum::IntoStaticStr; use crate::logstore::entry; use crate::metadata::{ - ColumnMetadata, DecodeProtoSnafu, InvalidRawRegionRequestSnafu, - InvalidRegionOptionChangeRequestSnafu, InvalidRegionRequestSnafu, MetadataError, + ColumnMetadata, DecodeProtoSnafu, InvalidRawRegionRequestSnafu, InvalidRegionRequestSnafu, + InvalidSetRegionOptionRequestSnafu, InvalidUnsetRegionOptionRequestSnafu, MetadataError, RegionMetadata, Result, }; use crate::metric_engine_consts::PHYSICAL_TABLE_METADATA_KEY; @@ -412,18 +412,17 @@ pub enum AlterKind { /// Columns to change. columns: Vec, }, - /// Change region options. - ChangeRegionOptions { - options: Vec, - }, - /// Change fulltext index options. + /// Set region options. + SetRegionOptions { options: Vec }, + /// Unset region options. + UnsetRegionOptions { keys: Vec }, + /// Set fulltext index options. SetColumnFulltext { column_name: String, options: FulltextOptions, }, - UnsetColumnFulltext { - column_name: String, - }, + /// Unset fulltext index options. + UnsetColumnFulltext { column_name: String }, } impl AlterKind { @@ -447,7 +446,8 @@ impl AlterKind { col_to_change.validate(metadata)?; } } - AlterKind::ChangeRegionOptions { .. } => {} + AlterKind::SetRegionOptions { .. } => {} + AlterKind::UnsetRegionOptions { .. } => {} AlterKind::SetColumnFulltext { column_name, .. } | AlterKind::UnsetColumnFulltext { column_name } => { Self::validate_column_fulltext_option(column_name, metadata)?; @@ -469,11 +469,12 @@ impl AlterKind { AlterKind::ModifyColumnTypes { columns } => columns .iter() .any(|col_to_change| col_to_change.need_alter(metadata)), - AlterKind::ChangeRegionOptions { .. } => { + AlterKind::SetRegionOptions { .. } => { // we need to update region options for `ChangeTableOptions`. // todo: we need to check if ttl has ever changed. true } + AlterKind::UnsetRegionOptions { .. } => true, AlterKind::SetColumnFulltext { column_name, .. } => { metadata.column_by_name(column_name).is_some() } @@ -550,15 +551,20 @@ impl TryFrom for AlterKind { let names = x.drop_columns.into_iter().map(|x| x.name).collect(); AlterKind::DropColumns { names } } - alter_request::Kind::ChangeTableOptions(change_options) => { - AlterKind::ChangeRegionOptions { - options: change_options - .change_table_options - .iter() - .map(TryFrom::try_from) - .collect::>>()?, - } - } + alter_request::Kind::SetTableOptions(options) => AlterKind::SetRegionOptions { + options: options + .table_options + .iter() + .map(TryFrom::try_from) + .collect::>>()?, + }, + alter_request::Kind::UnsetTableOptions(options) => AlterKind::UnsetRegionOptions { + keys: options + .keys + .iter() + .map(|key| UnsetRegionOption::try_from(key.as_str())) + .collect::>>()?, + }, alter_request::Kind::SetColumnFulltext(x) => AlterKind::SetColumnFulltext { column_name: x.column_name.clone(), options: FulltextOptions { @@ -739,17 +745,17 @@ impl From for ModifyColumnType { } #[derive(Debug, Eq, PartialEq, Clone, Serialize, Deserialize)] -pub enum ChangeOption { +pub enum SetRegionOption { TTL(Duration), // Modifying TwscOptions with values as (option name, new value). Twsc(String, String), } -impl TryFrom<&ChangeTableOption> for ChangeOption { +impl TryFrom<&TableOption> for SetRegionOption { type Error = MetadataError; - fn try_from(value: &ChangeTableOption) -> std::result::Result { - let ChangeTableOption { key, value } = value; + fn try_from(value: &TableOption) -> std::result::Result { + let TableOption { key, value } = value; match key.as_str() { TTL_KEY => { @@ -757,7 +763,7 @@ impl TryFrom<&ChangeTableOption> for ChangeOption { Duration::from_secs(0) } else { humantime::parse_duration(value) - .map_err(|_| InvalidRegionOptionChangeRequestSnafu { key, value }.build())? + .map_err(|_| InvalidSetRegionOptionRequestSnafu { key, value }.build())? }; Ok(Self::TTL(ttl)) } @@ -767,11 +773,85 @@ impl TryFrom<&ChangeTableOption> for ChangeOption { | TWCS_MAX_INACTIVE_WINDOW_RUNS | TWCS_MAX_OUTPUT_FILE_SIZE | TWCS_TIME_WINDOW => Ok(Self::Twsc(key.to_string(), value.to_string())), - _ => InvalidRegionOptionChangeRequestSnafu { key, value }.fail(), + _ => InvalidSetRegionOptionRequestSnafu { key, value }.fail(), + } + } +} + +impl From<&UnsetRegionOption> for SetRegionOption { + fn from(unset_option: &UnsetRegionOption) -> Self { + match unset_option { + UnsetRegionOption::TwcsMaxActiveWindowFiles => { + SetRegionOption::Twsc(unset_option.to_string(), String::new()) + } + UnsetRegionOption::TwcsMaxInactiveWindowFiles => { + SetRegionOption::Twsc(unset_option.to_string(), String::new()) + } + UnsetRegionOption::TwcsMaxActiveWindowRuns => { + SetRegionOption::Twsc(unset_option.to_string(), String::new()) + } + UnsetRegionOption::TwcsMaxInactiveWindowRuns => { + SetRegionOption::Twsc(unset_option.to_string(), String::new()) + } + UnsetRegionOption::TwcsMaxOutputFileSize => { + SetRegionOption::Twsc(unset_option.to_string(), String::new()) + } + UnsetRegionOption::TwcsTimeWindow => { + SetRegionOption::Twsc(unset_option.to_string(), String::new()) + } + UnsetRegionOption::Ttl => SetRegionOption::TTL(Duration::default()), } } } +impl TryFrom<&str> for UnsetRegionOption { + type Error = MetadataError; + + fn try_from(key: &str) -> Result { + match key.to_ascii_lowercase().as_str() { + TTL_KEY => Ok(Self::Ttl), + TWCS_MAX_ACTIVE_WINDOW_FILES => Ok(Self::TwcsMaxActiveWindowFiles), + TWCS_MAX_INACTIVE_WINDOW_FILES => Ok(Self::TwcsMaxInactiveWindowFiles), + TWCS_MAX_ACTIVE_WINDOW_RUNS => Ok(Self::TwcsMaxActiveWindowRuns), + TWCS_MAX_INACTIVE_WINDOW_RUNS => Ok(Self::TwcsMaxInactiveWindowRuns), + TWCS_MAX_OUTPUT_FILE_SIZE => Ok(Self::TwcsMaxOutputFileSize), + TWCS_TIME_WINDOW => Ok(Self::TwcsTimeWindow), + _ => InvalidUnsetRegionOptionRequestSnafu { key }.fail(), + } + } +} + +#[derive(Debug, Eq, PartialEq, Clone, Serialize, Deserialize)] +pub enum UnsetRegionOption { + TwcsMaxActiveWindowFiles, + TwcsMaxInactiveWindowFiles, + TwcsMaxActiveWindowRuns, + TwcsMaxInactiveWindowRuns, + TwcsMaxOutputFileSize, + TwcsTimeWindow, + Ttl, +} + +impl UnsetRegionOption { + pub fn as_str(&self) -> &str { + match self { + Self::Ttl => TTL_KEY, + Self::TwcsMaxActiveWindowFiles => TWCS_MAX_ACTIVE_WINDOW_FILES, + Self::TwcsMaxInactiveWindowFiles => TWCS_MAX_INACTIVE_WINDOW_FILES, + Self::TwcsMaxActiveWindowRuns => TWCS_MAX_ACTIVE_WINDOW_RUNS, + Self::TwcsMaxInactiveWindowRuns => TWCS_MAX_INACTIVE_WINDOW_RUNS, + Self::TwcsMaxOutputFileSize => TWCS_MAX_OUTPUT_FILE_SIZE, + Self::TwcsTimeWindow => TWCS_TIME_WINDOW, + } + } +} + +impl Display for UnsetRegionOption { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.as_str()) + } +} + #[derive(Debug, Clone, Default)] pub struct RegionFlushRequest { pub row_group_size: Option, diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index fd775bd76f45..7f4ddb409acd 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -28,7 +28,7 @@ use serde::{Deserialize, Serialize}; use snafu::{ensure, OptionExt, ResultExt}; use store_api::metric_engine_consts::PHYSICAL_TABLE_METADATA_KEY; use store_api::mito_engine_options::{COMPACTION_TYPE, COMPACTION_TYPE_TWCS}; -use store_api::region_request::ChangeOption; +use store_api::region_request::{SetRegionOption, UnsetRegionOption}; use store_api::storage::{ColumnDescriptor, ColumnDescriptorBuilder, ColumnId, RegionId}; use crate::error::{self, Result}; @@ -206,7 +206,8 @@ 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::SetTableOptions { options } => self.set_table_options(options), + AlterKind::UnsetTableOptions { keys } => self.unset_table_options(keys), AlterKind::SetColumnFulltext { column_name, options, @@ -218,19 +219,19 @@ impl TableMeta { } /// Creates a [TableMetaBuilder] with modified table options. - fn change_table_options(&self, requests: &[ChangeOption]) -> Result { + fn set_table_options(&self, requests: &[SetRegionOption]) -> Result { let mut new_options = self.options.clone(); for request in requests { match request { - ChangeOption::TTL(new_ttl) => { + SetRegionOption::TTL(new_ttl) => { if new_ttl.is_zero() { new_options.ttl = None; } else { new_options.ttl = Some(*new_ttl); } } - ChangeOption::Twsc(key, value) => { + SetRegionOption::Twsc(key, value) => { if !value.is_empty() { new_options .extra_options @@ -253,6 +254,11 @@ impl TableMeta { Ok(builder) } + fn unset_table_options(&self, requests: &[UnsetRegionOption]) -> Result { + let requests = requests.iter().map(Into::into).collect::>(); + self.set_table_options(&requests) + } + /// Creates a [TableMetaBuilder] with modified column fulltext options. fn change_column_fulltext_options( &self, diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 0e9cad7f05f3..4c273e03b785 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -30,7 +30,7 @@ use greptime_proto::v1::region::compact_request; use serde::{Deserialize, Serialize}; use store_api::metric_engine_consts::{LOGICAL_TABLE_METADATA_KEY, PHYSICAL_TABLE_METADATA_KEY}; use store_api::mito_engine_options::is_mito_engine_option_key; -use store_api::region_request::ChangeOption; +use store_api::region_request::{SetRegionOption, UnsetRegionOption}; use crate::error::{ParseTableOptionSnafu, Result}; use crate::metadata::{TableId, TableVersion}; @@ -213,8 +213,11 @@ pub enum AlterKind { RenameTable { new_table_name: String, }, - ChangeTableOptions { - options: Vec, + SetTableOptions { + options: Vec, + }, + UnsetTableOptions { + keys: Vec, }, SetColumnFulltext { column_name: String, @@ -225,30 +228,6 @@ pub enum AlterKind { }, } -// #[derive(Debug, Clone, Serialize, Deserialize)] -// pub enum ChangeTableOptionRequest { -// TTL(Duration), -// } - -// impl TryFrom<&ChangeTableOption> for ChangeTableOptionRequest { -// type Error = Error; -// -// fn try_from(value: &ChangeTableOption) -> std::result::Result { -// let ChangeTableOption { key, value } = value; -// if key == TTL_KEY { -// let ttl = if value.is_empty() { -// Duration::from_secs(0) -// } else { -// humantime::parse_duration(value) -// .map_err(|_| error::InvalidTableOptionValueSnafu { key, value }.build())? -// }; -// Ok(Self::TTL(ttl)) -// } else { -// UnsupportedTableOptionChangeSnafu { key }.fail() -// } -// } -// } - #[derive(Debug)] pub struct InsertRequest { pub catalog_name: String, diff --git a/tests/cases/standalone/common/alter/alter_table_options.result b/tests/cases/standalone/common/alter/alter_table_options.result index 53e765b0d23a..0cd05c96212e 100644 --- a/tests/cases/standalone/common/alter/alter_table_options.result +++ b/tests/cases/standalone/common/alter/alter_table_options.result @@ -186,7 +186,7 @@ SHOW CREATE TABLE ato; | | ) | +-------+----------------------------------------------------+ -ALTER TABLE ato SET 'compaction.twcs.time_window'=''; +ALTER TABLE ato UNSET 'compaction.twcs.time_window'; Affected Rows: 0 diff --git a/tests/cases/standalone/common/alter/alter_table_options.sql b/tests/cases/standalone/common/alter/alter_table_options.sql index d2ad4546c97d..a6cdb1de6f98 100644 --- a/tests/cases/standalone/common/alter/alter_table_options.sql +++ b/tests/cases/standalone/common/alter/alter_table_options.sql @@ -42,7 +42,7 @@ ALTER TABLE ato SET 'compaction.twcs.max_inactive_window_runs'='6'; SHOW CREATE TABLE ato; -ALTER TABLE ato SET 'compaction.twcs.time_window'=''; +ALTER TABLE ato UNSET 'compaction.twcs.time_window'; SHOW CREATE TABLE ato;