From 6bc72b77c942e1f9b3468441b6fefd2a76a7a1f0 Mon Sep 17 00:00:00 2001 From: lyang24 Date: Sun, 1 Dec 2024 15:11:48 -0800 Subject: [PATCH] feat: support alter inverted index. --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/common/grpc-expr/src/alter.rs | 44 +++-- .../src/ddl/alter_table/region_request.rs | 6 +- .../src/ddl/alter_table/update_metadata.rs | 4 +- src/datatypes/src/schema/column_schema.rs | 13 ++ src/mito2/src/engine/alter_test.rs | 138 +++++++++++++- src/operator/src/expr_factory.rs | 56 ++++-- src/sql/src/parsers/alter_parser.rs | 171 +++++++++++++++--- src/sql/src/statements/alter.rs | 76 ++++++-- src/store-api/src/metadata.rs | 69 +++++-- src/store-api/src/region_request.rs | 116 +++++++++--- src/table/src/metadata.rs | 122 +++++++++++-- src/table/src/requests.rs | 20 +- .../alter/change_col_inverted_index.result | 106 +++++++++++ .../alter/change_col_inverted_index.sql | 32 ++++ 16 files changed, 822 insertions(+), 155 deletions(-) create mode 100644 tests/cases/standalone/common/alter/change_col_inverted_index.result create mode 100644 tests/cases/standalone/common/alter/change_col_inverted_index.sql diff --git a/Cargo.lock b/Cargo.lock index 534b8c465ae6..aebe0cd234e5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4550,7 +4550,7 @@ dependencies = [ [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=a875e976441188028353f7274a46a7e6e065c5d4#a875e976441188028353f7274a46a7e6e065c5d4" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=5af5b4e883919f2f7e22ffdea13b656c3ca217b8#5af5b4e883919f2f7e22ffdea13b656c3ca217b8" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index 990bc71a907b..f016a22b6043 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -124,7 +124,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 = "a875e976441188028353f7274a46a7e6e065c5d4" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "5af5b4e883919f2f7e22ffdea13b656c3ca217b8" } 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 4c4ad0905bb3..03a6dc52f37f 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -25,7 +25,10 @@ use datatypes::schema::{ColumnSchema, FulltextOptions, RawSchema}; use snafu::{ensure, OptionExt, ResultExt}; use store_api::region_request::{SetRegionOption, UnsetRegionOption}; use table::metadata::TableId; -use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest, ModifyColumnTypeRequest}; +use table::requests::{ + AddColumnRequest, AlterKind, AlterTableRequest, ModifyColumnTypeRequest, SetIndexOptions, + UnsetIndexOptions, +}; use crate::error::{ InvalidColumnDefSnafu, InvalidSetFulltextOptionRequestSnafu, InvalidSetTableOptionRequestSnafu, @@ -113,18 +116,37 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterTableExpr) -> Result< .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(InvalidSetFulltextOptionRequestSnafu)?, - ), - case_sensitive: c.case_sensitive, + Kind::SetIndex(o) => match o.options.unwrap() { + api::v1::set_index::Options::Fulltext(f) => AlterKind::SetIndex { + options: SetIndexOptions::Fulltext { + column_name: f.column_name.clone(), + options: FulltextOptions { + enable: f.enable, + analyzer: as_fulltext_option( + Analyzer::try_from(f.analyzer) + .context(InvalidSetFulltextOptionRequestSnafu)?, + ), + case_sensitive: f.case_sensitive, + }, + }, + }, + api::v1::set_index::Options::Inverted(i) => AlterKind::SetIndex { + options: SetIndexOptions::Inverted { + column_name: i.column_name, + }, }, }, - Kind::UnsetColumnFulltext(c) => AlterKind::UnsetColumnFulltext { - column_name: c.column_name, + Kind::UnsetIndex(o) => match o.options.unwrap() { + api::v1::unset_index::Options::Fulltext(f) => AlterKind::UnsetIndex { + options: UnsetIndexOptions::Fulltext { + column_name: f.column_name, + }, + }, + api::v1::unset_index::Options::Inverted(i) => AlterKind::UnsetIndex { + options: UnsetIndexOptions::Inverted { + column_name: i.column_name, + }, + }, }, }; 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 fb700335aaf8..284cc20b5123 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -108,10 +108,8 @@ fn create_proto_alter_kind( Kind::RenameTable(_) => Ok(None), 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()))) - } + Kind::SetIndex(v) => Ok(Some(alter_request::Kind::SetIndex(v.clone()))), + Kind::UnsetIndex(v) => Ok(Some(alter_request::Kind::UnsetIndex(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 de72b7977aa2..4b054be0029c 100644 --- a/src/common/meta/src/ddl/alter_table/update_metadata.rs +++ b/src/common/meta/src/ddl/alter_table/update_metadata.rs @@ -55,8 +55,8 @@ impl AlterTableProcedure { | AlterKind::ModifyColumnTypes { .. } | AlterKind::SetTableOptions { .. } | AlterKind::UnsetTableOptions { .. } - | AlterKind::SetColumnFulltext { .. } - | AlterKind::UnsetColumnFulltext { .. } => {} + | AlterKind::SetIndex { .. } + | AlterKind::UnsetIndex { .. } => {} } Ok(new_info) diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index c1e2df846918..d9fa6fc1fd64 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -149,6 +149,19 @@ impl ColumnSchema { self } + pub fn update_inverted_index(&mut self, value: bool) { + match value { + true => { + self.metadata + .insert(INVERTED_INDEX_KEY.to_string(), value.to_string()); + } + + false => { + self.metadata.remove(INVERTED_INDEX_KEY); + } + } + } + pub fn is_inverted_indexed(&self) -> bool { self.metadata .get(INVERTED_INDEX_KEY) diff --git a/src/mito2/src/engine/alter_test.rs b/src/mito2/src/engine/alter_test.rs index b774dd8a05cf..f66966a3f1da 100644 --- a/src/mito2/src/engine/alter_test.rs +++ b/src/mito2/src/engine/alter_test.rs @@ -26,8 +26,8 @@ use datatypes::schema::{ColumnSchema, FulltextAnalyzer, FulltextOptions}; use store_api::metadata::ColumnMetadata; use store_api::region_engine::{RegionEngine, RegionRole}; use store_api::region_request::{ - AddColumn, AddColumnLocation, AlterKind, RegionAlterRequest, RegionOpenRequest, RegionRequest, - SetRegionOption, + AddColumn, AddColumnLocation, AlterKind, ApiSetIndexOptions, RegionAlterRequest, + RegionOpenRequest, RegionRequest, SetRegionOption, }; use store_api::storage::{RegionId, ScanRequest}; @@ -69,15 +69,28 @@ fn add_tag1() -> RegionAlterRequest { } } +fn alter_column_inverted_index() -> RegionAlterRequest { + RegionAlterRequest { + schema_version: 0, + kind: AlterKind::SetIndex { + options: ApiSetIndexOptions::Inverted { + column_name: "tag_0".to_string(), + }, + }, + } +} + fn alter_column_fulltext_options() -> RegionAlterRequest { RegionAlterRequest { schema_version: 0, - kind: AlterKind::SetColumnFulltext { - column_name: "tag_0".to_string(), - options: FulltextOptions { - enable: true, - analyzer: FulltextAnalyzer::English, - case_sensitive: false, + kind: AlterKind::SetIndex { + options: ApiSetIndexOptions::Fulltext { + column_name: "tag_0".to_string(), + options: FulltextOptions { + enable: true, + analyzer: FulltextAnalyzer::English, + case_sensitive: false, + }, }, }, } @@ -574,6 +587,115 @@ async fn test_alter_column_fulltext_options() { check_region_version(&engine, region_id, 1, 3, 1, 3); } +#[tokio::test] +async fn test_alter_column_set_inverted_index() { + common_telemetry::init_default_ut_logging(); + + let mut env = TestEnv::new(); + let listener = Arc::new(AlterFlushListener::default()); + let engine = env + .create_engine_with(MitoConfig::default(), None, Some(listener.clone())) + .await; + + let region_id = RegionId::new(1, 1); + let request = CreateRequestBuilder::new().build(); + + env.get_schema_metadata_manager() + .register_region_table_info( + region_id.table_id(), + "test_table", + "test_catalog", + "test_schema", + None, + ) + .await; + + let column_schemas = rows_schema(&request); + let region_dir = request.region_dir.clone(); + engine + .handle_request(region_id, RegionRequest::Create(request)) + .await + .unwrap(); + + let rows = Rows { + schema: column_schemas, + rows: build_rows(0, 3), + }; + put_rows(&engine, region_id, rows).await; + + // Spawns a task to flush the engine. + let engine_cloned = engine.clone(); + let flush_job = tokio::spawn(async move { + flush_region(&engine_cloned, region_id, None).await; + }); + // Waits for flush begin. + listener.wait_flush_begin().await; + + // Consumes the notify permit in the listener. + listener.wait_request_begin().await; + + // Submits an alter request to the region. The region should add the request + // to the pending ddl request list. + let request = alter_column_inverted_index(); + let engine_cloned = engine.clone(); + let alter_job = tokio::spawn(async move { + engine_cloned + .handle_request(region_id, RegionRequest::Alter(request)) + .await + .unwrap(); + }); + // Waits until the worker handles the alter request. + listener.wait_request_begin().await; + + // Spawns two task to flush the engine. The flush scheduler should put them to the + // pending task list. + let engine_cloned = engine.clone(); + let pending_flush_job = tokio::spawn(async move { + flush_region(&engine_cloned, region_id, None).await; + }); + // Waits until the worker handles the flush request. + listener.wait_request_begin().await; + + // Wake up flush. + listener.wake_flush(); + // Wait for the flush job. + flush_job.await.unwrap(); + // Wait for pending flush job. + pending_flush_job.await.unwrap(); + // Wait for the write job. + alter_job.await.unwrap(); + + let check_inverted_index_set = |engine: &MitoEngine| { + assert!(engine + .get_region(region_id) + .unwrap() + .metadata() + .column_by_name("tag_0") + .unwrap() + .column_schema + .is_inverted_indexed()) + }; + check_inverted_index_set(&engine); + check_region_version(&engine, region_id, 1, 3, 1, 3); + + // Reopen region. + let engine = env.reopen_engine(engine, MitoConfig::default()).await; + engine + .handle_request( + region_id, + RegionRequest::Open(RegionOpenRequest { + engine: String::new(), + region_dir, + options: HashMap::default(), + skip_wal_replay: false, + }), + ) + .await + .unwrap(); + check_inverted_index_set(&engine); + check_region_version(&engine, region_id, 1, 3, 1, 3); +} + #[tokio::test] async fn test_alter_region_ttl_options() { common_telemetry::init_default_ut_logging(); diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 59fe87a66ec3..3ef3ba6b8f9b 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -19,11 +19,11 @@ use api::v1::alter_database_expr::Kind as AlterDatabaseKind; use api::v1::alter_table_expr::Kind as AlterTableKind; use api::v1::column_def::options_from_column_schema; use api::v1::{ - AddColumn, AddColumns, AlterDatabaseExpr, AlterTableExpr, Analyzer, ColumnDataType, - ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropColumn, - DropColumns, ExpireAfter, ModifyColumnType, ModifyColumnTypes, RenameTable, SemanticType, - SetColumnFulltext, SetDatabaseOptions, SetTableOptions, TableName, UnsetColumnFulltext, - UnsetDatabaseOptions, UnsetTableOptions, + set_index, unset_index, AddColumn, AddColumns, AlterDatabaseExpr, AlterTableExpr, Analyzer, + ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, + DropColumn, DropColumns, ExpireAfter, ModifyColumnType, ModifyColumnTypes, RenameTable, + SemanticType, SetDatabaseOptions, SetFulltext, SetIndex, SetInverted, SetTableOptions, + TableName, UnsetDatabaseOptions, UnsetFulltext, UnsetIndex, UnsetInverted, UnsetTableOptions, }; use common_error::ext::BoxedError; use common_grpc_expr::util::ColumnExpr; @@ -540,23 +540,39 @@ pub(crate) fn to_alter_table_expr( AlterTableOperation::UnsetTableOptions { keys } => { AlterTableKind::UnsetTableOptions(UnsetTableOptions { keys }) } - AlterTableOperation::SetColumnFulltext { - column_name, - options, - } => AlterTableKind::SetColumnFulltext(SetColumnFulltext { - column_name: column_name.value, - enable: options.enable, - analyzer: match options.analyzer { - FulltextAnalyzer::English => Analyzer::English.into(), - FulltextAnalyzer::Chinese => Analyzer::Chinese.into(), + AlterTableOperation::SetIndex { options } => AlterTableKind::SetIndex(match options { + sql::statements::alter::SetIndexOperation::Fulltext { + column_name, + options, + } => SetIndex { + options: Some(set_index::Options::Fulltext(SetFulltext { + column_name: column_name.value, + enable: options.enable, + analyzer: match options.analyzer { + FulltextAnalyzer::English => Analyzer::English.into(), + FulltextAnalyzer::Chinese => Analyzer::Chinese.into(), + }, + case_sensitive: options.case_sensitive, + })), + }, + sql::statements::alter::SetIndexOperation::Inverted { column_name } => SetIndex { + options: Some(set_index::Options::Inverted(SetInverted { + column_name: column_name.value, + })), + }, + }), + AlterTableOperation::UnsetIndex { options } => AlterTableKind::UnsetIndex(match options { + sql::statements::alter::UnsetIndexOperation::Fulltext { column_name } => UnsetIndex { + options: Some(unset_index::Options::Fulltext(UnsetFulltext { + column_name: column_name.value, + })), + }, + sql::statements::alter::UnsetIndexOperation::Inverted { column_name } => UnsetIndex { + options: Some(unset_index::Options::Inverted(UnsetInverted { + column_name: column_name.value, + })), }, - case_sensitive: options.case_sensitive, }), - AlterTableOperation::UnsetColumnFulltext { column_name } => { - AlterTableKind::UnsetColumnFulltext(UnsetColumnFulltext { - column_name: column_name.value, - }) - } }; Ok(AlterTableExpr { diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index e64a734ebf99..96c43e49eb6a 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -20,13 +20,15 @@ use snafu::{ensure, ResultExt}; use sqlparser::ast::Ident; use sqlparser::keywords::Keyword; use sqlparser::parser::{Parser, ParserError}; -use sqlparser::tokenizer::Token; +use sqlparser::tokenizer::{Token, TokenWithLocation}; +use super::create_parser::INVERTED; use crate::error::{self, InvalidColumnOptionSnafu, Result, SetFulltextOptionSnafu}; use crate::parser::ParserContext; use crate::parsers::utils::validate_column_fulltext_create_option; use crate::statements::alter::{ AlterDatabase, AlterDatabaseOperation, AlterTable, AlterTableOperation, KeyValueOption, + SetIndexOperation, UnsetIndexOperation, }; use crate::statements::statement::Statement; use crate::util::parse_option_string; @@ -226,15 +228,13 @@ impl ParserContext<'_> { 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::UnsetColumnFulltext { column_name }) + // consume the current token. + self.parser.next_token(); + self.parse_alter_column_unset_index(column_name) } else if w.keyword == Keyword::SET { - self.parse_alter_column_fulltext(column_name) + // consume the current token. + self.parser.next_token(); + self.parse_alter_column_set_index(column_name) } else { let data_type = self.parser.parse_data_type().context(error::SyntaxSnafu)?; Ok(AlterTableOperation::ModifyColumnType { @@ -250,13 +250,62 @@ impl ParserContext<'_> { } } - fn parse_alter_column_fulltext(&mut self, column_name: Ident) -> Result { - let _ = self.parser.next_token(); + fn parse_alter_column_unset_index( + &mut self, + column_name: Ident, + ) -> Result { + match self.parser.next_token() { + TokenWithLocation { + token: Token::Word(w), + .. + } if w.keyword == Keyword::FULLTEXT => Ok(AlterTableOperation::UnsetIndex { + options: UnsetIndexOperation::Fulltext { column_name }, + }), + + TokenWithLocation { + token: Token::Word(w), + .. + } if w.value.eq_ignore_ascii_case(INVERTED) => { + self.parser + .expect_keyword(Keyword::INDEX) + .context(error::SyntaxSnafu)?; + Ok(AlterTableOperation::UnsetIndex { + options: UnsetIndexOperation::Inverted { column_name }, + }) + } + _ => self.expected( + format!("{:?} OR INVERTED INDEX", Keyword::FULLTEXT).as_str(), + self.parser.peek_token(), + ), + } + } - self.parser - .expect_keyword(Keyword::FULLTEXT) - .context(error::SyntaxSnafu)?; + fn parse_alter_column_set_index(&mut self, column_name: Ident) -> Result { + match self.parser.next_token() { + TokenWithLocation { + token: Token::Word(w), + .. + } if w.keyword == Keyword::FULLTEXT => self.parse_alter_column_fulltext(column_name), + + TokenWithLocation { + token: Token::Word(w), + .. + } if w.value.eq_ignore_ascii_case(INVERTED) => { + self.parser + .expect_keyword(Keyword::INDEX) + .context(error::SyntaxSnafu)?; + Ok(AlterTableOperation::SetIndex { + options: SetIndexOperation::Inverted { column_name }, + }) + } + _ => self.expected( + format!("{:?} OR INVERTED INDEX", Keyword::FULLTEXT).as_str(), + self.parser.peek_token(), + ), + } + } + fn parse_alter_column_fulltext(&mut self, column_name: Ident) -> Result { let mut options = self .parser .parse_options(Keyword::WITH) @@ -280,9 +329,11 @@ impl ParserContext<'_> { "true".to_string(), ); - Ok(AlterTableOperation::SetColumnFulltext { - column_name, - options: options.try_into().context(SetFulltextOptionSnafu)?, + Ok(AlterTableOperation::SetIndex { + options: SetIndexOperation::Fulltext { + column_name, + options: options.try_into().context(SetFulltextOptionSnafu)?, + }, }) } } @@ -764,14 +815,13 @@ mod tests { assert_eq!("test_table", alter_table.table_name().0[0].value); let alter_operation = alter_table.alter_operation(); - assert_matches!( - alter_operation, - AlterTableOperation::SetColumnFulltext { .. } - ); match alter_operation { - AlterTableOperation::SetColumnFulltext { - column_name, - options, + AlterTableOperation::SetIndex { + options: + SetIndexOperation::Fulltext { + column_name, + options, + }, } => { assert_eq!("a", column_name.value); assert_eq!( @@ -784,7 +834,7 @@ mod tests { ); } _ => unreachable!(), - } + }; } _ => unreachable!(), } @@ -803,10 +853,12 @@ mod tests { let alter_operation = alter_table.alter_operation(); assert_eq!( alter_operation, - &AlterTableOperation::UnsetColumnFulltext { - column_name: Ident { - value: "a".to_string(), - quote_style: None + &AlterTableOperation::UnsetIndex { + options: UnsetIndexOperation::Fulltext { + column_name: Ident { + value: "a".to_string(), + quote_style: None + } } } ); @@ -827,4 +879,65 @@ mod tests { "Invalid column option, column name: a, error: invalid FULLTEXT option: abcd" ); } + + #[test] + fn test_parse_alter_column_inverted() { + let sql = "ALTER TABLE test_table MODIFY COLUMN a SET INVERTED INDEX"; + 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::AlterTable { .. }); + match statement { + Statement::AlterTable(alter_table) => { + assert_eq!("test_table", alter_table.table_name().0[0].value); + + let alter_operation = alter_table.alter_operation(); + match alter_operation { + AlterTableOperation::SetIndex { + options: SetIndexOperation::Inverted { column_name }, + } => assert_eq!("a", column_name.value), + _ => unreachable!(), + }; + } + _ => unreachable!(), + } + + let sql = "ALTER TABLE test_table MODIFY COLUMN a UNSET INVERTED INDEX"; + 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::AlterTable { .. }); + match statement { + Statement::AlterTable(alter_table) => { + assert_eq!("test_table", alter_table.table_name().0[0].value); + + let alter_operation = alter_table.alter_operation(); + assert_eq!( + alter_operation, + &AlterTableOperation::UnsetIndex { + options: UnsetIndexOperation::Inverted { + column_name: Ident { + value: "a".to_string(), + quote_style: None + } + } + } + ); + } + _ => unreachable!(), + } + + let invalid_sql = "ALTER TABLE test_table MODIFY COLUMN a SET INVERTED"; + ParserContext::create_with_dialect( + invalid_sql, + &GreptimeDbDialect {}, + ParseOptions::default(), + ) + .unwrap_err(); + } } diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index df148ae5b63d..033a69f21413 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -78,14 +78,35 @@ pub enum AlterTableOperation { /// `DROP COLUMN ` DropColumn { name: Ident }, /// `RENAME ` - RenameTable { new_table_name: String }, + RenameTable { + new_table_name: String, + }, + SetIndex { + options: SetIndexOperation, + }, + UnsetIndex { + options: UnsetIndexOperation, + }, +} + +#[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut, Serialize)] +pub enum SetIndexOperation { /// `MODIFY COLUMN SET FULLTEXT [WITH ]` - SetColumnFulltext { + Fulltext { column_name: Ident, options: FulltextOptions, }, + /// `MODIFY COLUMN SET INVERTED INDEX` + Inverted { column_name: Ident }, +} + +#[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut, Serialize)] +pub enum UnsetIndexOperation { /// `MODIFY COLUMN UNSET FULLTEXT` - UnsetColumnFulltext { column_name: Ident }, + Fulltext { column_name: Ident }, + + /// `MODIFY COLUMN UNSET INVERTED INDEX` + Inverted { column_name: Ident }, } impl Display for AlterTableOperation { @@ -130,15 +151,25 @@ impl Display for AlterTableOperation { let keys = keys.iter().map(|k| format!("'{k}'")).join(","); write!(f, "UNSET {keys}") } - AlterTableOperation::SetColumnFulltext { - column_name, - options, - } => { - 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") - } + AlterTableOperation::SetIndex { options } => match options { + SetIndexOperation::Fulltext { + column_name, + options, + } => { + write!(f, "MODIFY COLUMN {column_name} SET FULLTEXT WITH(analyzer={0}, case_sensitive={1})", options.analyzer, options.case_sensitive) + } + SetIndexOperation::Inverted { column_name } => { + write!(f, "MODIFY COLUMN {column_name} SET INVERTED INDEX") + } + }, + AlterTableOperation::UnsetIndex { options } => match options { + UnsetIndexOperation::Fulltext { column_name } => { + write!(f, "MODIFY COLUMN {column_name} UNSET FULLTEXT") + } + UnsetIndexOperation::Inverted { column_name } => { + write!(f, "MODIFY COLUMN {column_name} UNSET INVERTED INDEX") + } + }, } } } @@ -400,5 +431,26 @@ ALTER TABLE monitor MODIFY COLUMN a UNSET FULLTEXT"#, unreachable!(); } } + + let sql = "ALTER TABLE monitor MODIFY COLUMN a SET INVERTED INDEX"; + let stmts = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(1, stmts.len()); + assert_matches!(&stmts[0], Statement::AlterTable { .. }); + + match &stmts[0] { + Statement::AlterTable(set) => { + let new_sql = format!("\n{}", set); + assert_eq!( + r#" +ALTER TABLE monitor MODIFY COLUMN a SET INVERTED INDEX"#, + &new_sql + ); + } + _ => { + unreachable!(); + } + } } } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 57351e19ca99..d3f60834c391 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -33,7 +33,10 @@ use serde::de::Error; use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; -use crate::region_request::{AddColumn, AddColumnLocation, AlterKind, ModifyColumnType}; +use crate::region_request::{ + AddColumn, AddColumnLocation, AlterKind, ApiSetIndexOptions, ApiUnsetIndexOptions, + ModifyColumnType, +}; use crate::storage::consts::is_internal_column; use crate::storage::{ColumnId, RegionId}; @@ -553,13 +556,23 @@ 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::SetColumnFulltext { - 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::SetIndex { options } => match options { + ApiSetIndexOptions::Fulltext { + column_name, + options, + } => self.change_column_fulltext_options(column_name, true, Some(options))?, + ApiSetIndexOptions::Inverted { column_name } => { + self.change_column_inverted_index_options(column_name, true)? + } + }, + AlterKind::UnsetIndex { options } => match options { + ApiUnsetIndexOptions::Fulltext { column_name } => { + self.change_column_fulltext_options(column_name, false, None)? + } + ApiUnsetIndexOptions::Inverted { column_name } => { + self.change_column_inverted_index_options(column_name, false)? + } + }, AlterKind::SetRegionOptions { options: _ } => { // nothing to be done with RegionMetadata } @@ -666,6 +679,26 @@ impl RegionMetadataBuilder { } } + fn change_column_inverted_index_options( + &mut self, + column_name: String, + value: bool, + ) -> Result<()> { + for column_meta in self.column_metadatas.iter_mut() { + if column_meta.column_schema.name == column_name { + ensure!( + column_meta.column_schema.data_type.is_string(), + InvalidColumnOptionSnafu { + column_name, + msg: "INVERTED index only supports string type".to_string(), + } + ); + column_meta.column_schema.update_inverted_index(value) + } + } + Ok(()) + } + fn change_column_fulltext_options( &mut self, column_name: String, @@ -1363,12 +1396,14 @@ mod test { let mut builder = RegionMetadataBuilder::from_existing(metadata); builder - .alter(AlterKind::SetColumnFulltext { - column_name: "b".to_string(), - options: FulltextOptions { - enable: true, - analyzer: datatypes::schema::FulltextAnalyzer::Chinese, - case_sensitive: true, + .alter(AlterKind::SetIndex { + options: ApiSetIndexOptions::Fulltext { + column_name: "b".to_string(), + options: FulltextOptions { + enable: true, + analyzer: datatypes::schema::FulltextAnalyzer::Chinese, + case_sensitive: true, + }, }, }) .unwrap(); @@ -1389,8 +1424,10 @@ mod test { let mut builder = RegionMetadataBuilder::from_existing(metadata); builder - .alter(AlterKind::UnsetColumnFulltext { - column_name: "b".to_string(), + .alter(AlterKind::UnsetIndex { + options: ApiUnsetIndexOptions::Fulltext { + column_name: "b".to_string(), + }, }) .unwrap(); let metadata = builder.build().unwrap(); diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index ee9ebf483d98..17ecb9924b7d 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -23,7 +23,7 @@ use api::v1::region::{ CompactRequest, CreateRequest, CreateRequests, DeleteRequests, DropRequest, DropRequests, FlushRequest, InsertRequests, OpenRequest, TruncateRequest, }; -use api::v1::{self, Analyzer, Option as PbOption, Rows, SemanticType}; +use api::v1::{self, set_index, Analyzer, Option as PbOption, Rows, SemanticType}; pub use common_base::AffectedRows; use common_time::TimeToLive; use datatypes::data_type::ConcreteDataType; @@ -416,13 +416,45 @@ pub enum AlterKind { SetRegionOptions { options: Vec }, /// Unset region options. UnsetRegionOptions { keys: Vec }, - /// Set fulltext index options. - SetColumnFulltext { + /// Set index options. + SetIndex { options: ApiSetIndexOptions }, + /// Unset index options. + UnsetIndex { options: ApiUnsetIndexOptions }, +} + +#[derive(Debug, PartialEq, Eq, Clone)] +pub enum ApiSetIndexOptions { + Fulltext { column_name: String, options: FulltextOptions, }, - /// Unset fulltext index options. - UnsetColumnFulltext { column_name: String }, + Inverted { + column_name: String, + }, +} + +impl ApiSetIndexOptions { + pub fn column_name(&self) -> &String { + match self { + ApiSetIndexOptions::Fulltext { column_name, .. } => column_name, + ApiSetIndexOptions::Inverted { column_name } => column_name, + } + } +} + +#[derive(Debug, PartialEq, Eq, Clone)] +pub enum ApiUnsetIndexOptions { + Fulltext { column_name: String }, + Inverted { column_name: String }, +} + +impl ApiUnsetIndexOptions { + pub fn column_name(&self) -> &String { + match self { + ApiUnsetIndexOptions::Fulltext { column_name } => column_name, + ApiUnsetIndexOptions::Inverted { column_name } => column_name, + } + } } impl AlterKind { @@ -448,9 +480,11 @@ impl AlterKind { } AlterKind::SetRegionOptions { .. } => {} AlterKind::UnsetRegionOptions { .. } => {} - AlterKind::SetColumnFulltext { column_name, .. } - | AlterKind::UnsetColumnFulltext { column_name } => { - Self::validate_column_fulltext_option(column_name, metadata)?; + AlterKind::SetIndex { options } => { + Self::validate_column_fulltext_option(options.column_name(), metadata)?; + } + AlterKind::UnsetIndex { options } => { + Self::validate_column_fulltext_option(options.column_name(), metadata)?; } } Ok(()) @@ -475,11 +509,11 @@ impl AlterKind { true } AlterKind::UnsetRegionOptions { .. } => true, - AlterKind::SetColumnFulltext { column_name, .. } => { - metadata.column_by_name(column_name).is_some() + AlterKind::SetIndex { options, .. } => { + metadata.column_by_name(options.column_name()).is_some() } - AlterKind::UnsetColumnFulltext { column_name } => { - metadata.column_by_name(column_name).is_some() + AlterKind::UnsetIndex { options } => { + metadata.column_by_name(options.column_name()).is_some() } } } @@ -565,18 +599,36 @@ impl TryFrom for AlterKind { .map(|key| UnsetRegionOption::try_from(key.as_str())) .collect::>>()?, }, - alter_request::Kind::SetColumnFulltext(x) => AlterKind::SetColumnFulltext { - column_name: x.column_name.clone(), - options: FulltextOptions { - enable: x.enable, - analyzer: as_fulltext_option( - Analyzer::try_from(x.analyzer).context(DecodeProtoSnafu)?, - ), - case_sensitive: x.case_sensitive, + alter_request::Kind::SetIndex(o) => match o.options.unwrap() { + set_index::Options::Fulltext(x) => AlterKind::SetIndex { + options: ApiSetIndexOptions::Fulltext { + column_name: x.column_name.clone(), + options: FulltextOptions { + enable: x.enable, + analyzer: as_fulltext_option( + Analyzer::try_from(x.analyzer).context(DecodeProtoSnafu)?, + ), + case_sensitive: x.case_sensitive, + }, + }, + }, + set_index::Options::Inverted(i) => AlterKind::SetIndex { + options: ApiSetIndexOptions::Inverted { + column_name: i.column_name, + }, }, }, - alter_request::Kind::UnsetColumnFulltext(x) => AlterKind::UnsetColumnFulltext { - column_name: x.column_name, + alter_request::Kind::UnsetIndex(o) => match o.options.unwrap() { + v1::unset_index::Options::Fulltext(f) => AlterKind::UnsetIndex { + options: ApiUnsetIndexOptions::Fulltext { + column_name: f.column_name, + }, + }, + v1::unset_index::Options::Inverted(i) => AlterKind::UnsetIndex { + options: ApiUnsetIndexOptions::Inverted { + column_name: i.column_name, + }, + }, }, }; @@ -1306,12 +1358,14 @@ mod tests { #[test] fn test_validate_modify_column_fulltext_options() { - let kind = AlterKind::SetColumnFulltext { - column_name: "tag_0".to_string(), - options: FulltextOptions { - enable: true, - analyzer: FulltextAnalyzer::Chinese, - case_sensitive: false, + let kind = AlterKind::SetIndex { + options: ApiSetIndexOptions::Fulltext { + column_name: "tag_0".to_string(), + options: FulltextOptions { + enable: true, + analyzer: FulltextAnalyzer::Chinese, + case_sensitive: false, + }, }, }; let request = RegionAlterRequest { @@ -1322,8 +1376,10 @@ mod tests { metadata.schema_version = 1; request.validate(&metadata).unwrap(); - let kind = AlterKind::UnsetColumnFulltext { - column_name: "tag_0".to_string(), + let kind = AlterKind::UnsetIndex { + options: ApiUnsetIndexOptions::Fulltext { + column_name: "tag_0".to_string(), + }, }; let request = RegionAlterRequest { schema_version: 1, diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 6dfc47314a36..5dc01d7f5147 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -32,7 +32,10 @@ use store_api::region_request::{SetRegionOption, UnsetRegionOption}; use store_api::storage::{ColumnDescriptor, ColumnDescriptorBuilder, ColumnId, RegionId}; use crate::error::{self, Result}; -use crate::requests::{AddColumnRequest, AlterKind, ModifyColumnTypeRequest, TableOptions}; +use crate::requests::{ + AddColumnRequest, AlterKind, ModifyColumnTypeRequest, SetIndexOptions, TableOptions, + UnsetIndexOptions, +}; pub type TableId = u32; pub type TableVersion = u64; @@ -208,13 +211,28 @@ impl TableMeta { AlterKind::RenameTable { .. } => Ok(self.new_meta_builder()), AlterKind::SetTableOptions { options } => self.set_table_options(options), AlterKind::UnsetTableOptions { keys } => self.unset_table_options(keys), - AlterKind::SetColumnFulltext { - 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) - } + AlterKind::SetIndex { options } => match options { + SetIndexOptions::Fulltext { + column_name, + options, + } => self.change_column_fulltext_options( + table_name, + column_name, + true, + Some(options), + ), + SetIndexOptions::Inverted { column_name } => { + self.change_column_modify_inverted_index(table_name, column_name, true) + } + }, + AlterKind::UnsetIndex { options } => match options { + UnsetIndexOptions::Fulltext { column_name } => { + self.change_column_fulltext_options(table_name, column_name, false, None) + } + UnsetIndexOptions::Inverted { column_name } => { + self.change_column_modify_inverted_index(table_name, column_name, false) + } + }, } } @@ -255,6 +273,66 @@ impl TableMeta { self.set_table_options(&requests) } + /// Creates a [TableMetaBuilder] with modified column inverted index. + fn change_column_modify_inverted_index( + &self, + table_name: &str, + column_name: &str, + value: bool, + ) -> Result { + let table_schema = &self.schema; + let mut meta_builder = self.new_meta_builder(); + + let column = &table_schema + .column_schema_by_name(column_name) + .with_context(|| error::ColumnNotExistsSnafu { + column_name, + table_name, + })?; + + ensure!( + column.data_type.is_string(), + error::InvalidColumnOptionSnafu { + column_name, + msg: "INVERTED index only supports string type", + } + ); + + 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.update_inverted_index(value); + columns.push(new_column_schema); + } else { + columns.push(column_schema.clone()); + } + } + + // TODO(CookiePieWw): This part for all alter table operations is similar. We can refactor it. + let mut builder = SchemaBuilder::try_from_columns(columns) + .with_context(|_| error::SchemaBuildSnafu { + msg: format!("Failed to convert column schemas into schema for table {table_name}"), + })? + .version(table_schema.version() + 1); + + for (k, v) in table_schema.metadata().iter() { + builder = builder.add_metadata(k, v); + } + + let new_schema = builder.build().with_context(|_| error::SchemaBuildSnafu { + msg: format!( + "Table {table_name} cannot change fulltext options for column {column_name}", + ), + })?; + + let _ = meta_builder + .schema(Arc::new(new_schema)) + .primary_key_indices(self.primary_key_indices.clone()); + + Ok(meta_builder) + } + /// Creates a [TableMetaBuilder] with modified column fulltext options. fn change_column_fulltext_options( &self, @@ -1526,9 +1604,11 @@ mod tests { .build() .unwrap(); - let alter_kind = AlterKind::SetColumnFulltext { - column_name: "col1".to_string(), - options: FulltextOptions::default(), + let alter_kind = AlterKind::SetIndex { + options: SetIndexOptions::Fulltext { + column_name: "col1".to_string(), + options: FulltextOptions::default(), + }, }; let err = meta .builder_with_alter_kind("my_table", &alter_kind, false) @@ -1543,12 +1623,14 @@ 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::SetColumnFulltext { - column_name: "my_tag_first".to_string(), - options: FulltextOptions { - enable: true, - analyzer: datatypes::schema::FulltextAnalyzer::Chinese, - case_sensitive: true, + let alter_kind = AlterKind::SetIndex { + options: SetIndexOptions::Fulltext { + column_name: "my_tag_first".to_string(), + options: FulltextOptions { + enable: true, + analyzer: datatypes::schema::FulltextAnalyzer::Chinese, + case_sensitive: true, + }, }, }; let new_meta = new_meta @@ -1568,8 +1650,10 @@ mod tests { ); assert!(fulltext_options.case_sensitive); - let alter_kind = AlterKind::UnsetColumnFulltext { - column_name: "my_tag_first".to_string(), + let alter_kind = AlterKind::UnsetIndex { + options: UnsetIndexOptions::Fulltext { + column_name: "my_tag_first".to_string(), + }, }; let new_meta = new_meta .builder_with_alter_kind("my_table", &alter_kind, false) diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 74554631c62d..9377b1cef3ca 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -214,15 +214,31 @@ pub enum AlterKind { UnsetTableOptions { keys: Vec, }, - SetColumnFulltext { + SetIndex { + options: SetIndexOptions, + }, + UnsetIndex { + options: UnsetIndexOptions, + }, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum SetIndexOptions { + Fulltext { column_name: String, options: FulltextOptions, }, - UnsetColumnFulltext { + Inverted { column_name: String, }, } +#[derive(Debug, Clone, Serialize, Deserialize)] +pub enum UnsetIndexOptions { + Fulltext { column_name: String }, + Inverted { column_name: String }, +} + #[derive(Debug)] pub struct InsertRequest { pub catalog_name: String, diff --git a/tests/cases/standalone/common/alter/change_col_inverted_index.result b/tests/cases/standalone/common/alter/change_col_inverted_index.result new file mode 100644 index 000000000000..168e250e1e88 --- /dev/null +++ b/tests/cases/standalone/common/alter/change_col_inverted_index.result @@ -0,0 +1,106 @@ +CREATE TABLE fox ( + ts TIMESTAMP TIME INDEX, + fox STRING, +); + +Affected Rows: 0 + +INSERT INTO fox VALUES + (1, 'The quick brown fox jumps over the lazy dog'), + (2, 'The fox jumps over the lazy dog'), + (3, 'The quick brown jumps over the lazy dog'), + (4, 'The quick brown fox over the lazy dog'), + (5, 'The quick brown fox jumps the lazy dog'), + (6, 'The quick brown fox jumps over dog'), + (7, 'The quick brown fox jumps over the dog'); + +Affected Rows: 7 + +ALTER TABLE fox MODIFY COLUMN fox SET INVERTED INDEX; + +Affected Rows: 0 + +SELECT fox FROM fox WHERE MATCHES(fox, '"fox jumps"') ORDER BY ts; + ++---------------------------------------------+ +| fox | ++---------------------------------------------+ +| The quick brown fox jumps over the lazy dog | +| The fox jumps over the lazy dog | +| The quick brown fox jumps the lazy dog | +| The quick brown fox jumps over dog | +| The quick brown fox jumps over the dog | ++---------------------------------------------+ + +SHOW CREATE TABLE fox; + ++-------+------------------------------------+ +| Table | Create Table | ++-------+------------------------------------+ +| fox | CREATE TABLE IF NOT EXISTS "fox" ( | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "fox" STRING NULL, | +| | TIME INDEX ("ts"), | +| | INVERTED INDEX ("fox") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++-------+------------------------------------+ + +-- SQLNESS ARG restart=true +SHOW CREATE TABLE fox; + ++-------+------------------------------------+ +| Table | Create Table | ++-------+------------------------------------+ +| fox | CREATE TABLE IF NOT EXISTS "fox" ( | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "fox" STRING NULL, | +| | TIME INDEX ("ts"), | +| | INVERTED INDEX ("fox") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++-------+------------------------------------+ + +ALTER TABLE fox MODIFY COLUMN fox UNSET INVERTED INDEX; + +Affected Rows: 0 + +SHOW CREATE TABLE fox; + ++-------+------------------------------------+ +| Table | Create Table | ++-------+------------------------------------+ +| fox | CREATE TABLE IF NOT EXISTS "fox" ( | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "fox" STRING NULL, | +| | TIME INDEX ("ts") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++-------+------------------------------------+ + +-- SQLNESS ARG restart=true +SHOW CREATE TABLE fox; + ++-------+------------------------------------+ +| Table | Create Table | ++-------+------------------------------------+ +| fox | CREATE TABLE IF NOT EXISTS "fox" ( | +| | "ts" TIMESTAMP(3) NOT NULL, | +| | "fox" STRING NULL, | +| | TIME INDEX ("ts") | +| | ) | +| | | +| | ENGINE=mito | +| | | ++-------+------------------------------------+ + +DROP TABLE fox; + +Affected Rows: 0 + diff --git a/tests/cases/standalone/common/alter/change_col_inverted_index.sql b/tests/cases/standalone/common/alter/change_col_inverted_index.sql new file mode 100644 index 000000000000..30e26d7c2e68 --- /dev/null +++ b/tests/cases/standalone/common/alter/change_col_inverted_index.sql @@ -0,0 +1,32 @@ +CREATE TABLE fox ( + ts TIMESTAMP TIME INDEX, + fox STRING, +); + +INSERT INTO fox VALUES + (1, 'The quick brown fox jumps over the lazy dog'), + (2, 'The fox jumps over the lazy dog'), + (3, 'The quick brown jumps over the lazy dog'), + (4, 'The quick brown fox over the lazy dog'), + (5, 'The quick brown fox jumps the lazy dog'), + (6, 'The quick brown fox jumps over dog'), + (7, 'The quick brown fox jumps over the dog'); + + +ALTER TABLE fox MODIFY COLUMN fox SET INVERTED INDEX; + +SELECT fox FROM fox WHERE MATCHES(fox, '"fox jumps"') ORDER BY ts; + +SHOW CREATE TABLE fox; + +-- SQLNESS ARG restart=true +SHOW CREATE TABLE fox; + +ALTER TABLE fox MODIFY COLUMN fox UNSET INVERTED INDEX; + +SHOW CREATE TABLE fox; + +-- SQLNESS ARG restart=true +SHOW CREATE TABLE fox; + +DROP TABLE fox;