From 693130c8c58d5619012fe166620b92675815c48d Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 28 Aug 2024 23:24:41 +0800 Subject: [PATCH 01/20] feat: Alter column option --- Cargo.lock | 2 +- Cargo.toml | 4 +- src/common/grpc-expr/src/alter.rs | 1 + .../src/ddl/alter_table/region_request.rs | 1 + .../src/ddl/alter_table/update_metadata.rs | 2 + src/operator/src/expr_factory.rs | 13 ++++-- src/sql/src/parsers/alter_parser.rs | 31 +++++++++++++- src/sql/src/statements/alter.rs | 16 +++++++- src/sql/src/statements/option_map.rs | 1 + src/table/src/error.rs | 12 +++++- src/table/src/metadata.rs | 41 ++++++++++++++++++- src/table/src/requests.rs | 4 ++ 12 files changed, 118 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index bdd59547a9e0..4c1ad0ad4029 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4252,7 +4252,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=c437b55725b7f5224fe9d46db21072b4a682ee4b#c437b55725b7f5224fe9d46db21072b4a682ee4b" +source = "git+https://github.com/irenjj/greptime-proto.git?rev=ca5920ee9263650c37556edad4bc877a674af444#ca5920ee9263650c37556edad4bc877a674af444" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index 4b5fb9369e38..d371c76781fc 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,7 +119,7 @@ etcd-client = { version = "0.13" } fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "c437b55725b7f5224fe9d46db21072b4a682ee4b" } +greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "ca5920ee9263650c37556edad4bc877a674af444" } humantime = "2.1" humantime-serde = "1.1" itertools = "0.10" @@ -151,7 +151,7 @@ reqwest = { version = "0.12", default-features = false, features = [ "stream", "multipart", ] } -# SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 +# SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 rskafka = { git = "https://github.com/WenyXu/rskafka.git", rev = "940c6030012c5b746fad819fb72e3325b26e39de", features = [ "transport-tls", ] } diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index ac49069412cd..dc1b0c316b26 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -92,6 +92,7 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result { AlterKind::RenameTable { new_table_name } } + Kind::AddFulltext(_) => { todo!() }, }; let request = AlterTableRequest { diff --git a/src/common/meta/src/ddl/alter_table/region_request.rs b/src/common/meta/src/ddl/alter_table/region_request.rs index b4223b8ea05d..b5e7e7a632de 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -106,6 +106,7 @@ fn create_proto_alter_kind( }))) } Kind::RenameTable(_) => Ok(None), + Kind::AddFulltext(_) => todo!(), } } 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 6ef6e2e7fc88..8fe513a6ab2a 100644 --- a/src/common/meta/src/ddl/alter_table/update_metadata.rs +++ b/src/common/meta/src/ddl/alter_table/update_metadata.rs @@ -52,6 +52,8 @@ impl AlterTableProcedure { new_info.name = new_table_name.to_string(); } AlterKind::DropColumns { .. } | AlterKind::ChangeColumnTypes { .. } => {} + AlterKind::AddFulltexts { column_name, options } => { + }, } Ok(new_info) diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index e9be2e712fab..e25bd2fc9482 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -18,9 +18,9 @@ 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, ChangeColumnType, ChangeColumnTypes, ColumnDataType, - ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropColumn, - DropColumns, ExpireAfter, RenameTable, SemanticType, TableName, + AddColumn, AddColumns, AddFulltext, AlterExpr, ChangeColumnType, ChangeColumnTypes, + ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, + DropColumn, DropColumns, ExpireAfter, RenameTable, SemanticType, TableName, }; use common_error::ext::BoxedError; use common_grpc_expr::util::ColumnExpr; @@ -483,6 +483,13 @@ pub(crate) fn to_alter_expr( AlterTableOperation::RenameTable { new_table_name } => Kind::RenameTable(RenameTable { new_table_name: new_table_name.to_string(), }), + AlterTableOperation::AlterColumnFulltext { + column_name, + options, + } => Kind::AddFulltext(AddFulltext { + column_name: column_name.value.to_string(), + options: options.hash_options.clone(), + }), }; Ok(AlterExpr { diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 0473ef52cee5..8ce9e08169b6 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -12,8 +12,11 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::collections::HashMap; + use common_query::AddColumnLocation; use snafu::ResultExt; +use sqlparser::ast::{Expr, SqlOption}; use sqlparser::keywords::Keyword; use sqlparser::parser::ParserError; use sqlparser::tokenizer::Token; @@ -94,9 +97,35 @@ impl<'a> ParserContext<'a> { } }; AlterTableOperation::RenameTable { new_table_name } + } else if self.parser.parse_keyword(Keyword::SET) { + let _ = self.parser.parse_keyword(Keyword::COLUMN); + let column_name = Self::canonicalize_identifier(self.parser.parse_identifier(false)?); + + let _ = self.parser.parse_keyword(Keyword::FULLTEXT); + + let options= self + .parser + .parse_options(Keyword::WITH)? + .into_iter() + .map(|option: SqlOption| -> Result<(String, String)> { + let (key, value) = (option.name, option.value); + let v = match value { + Expr::Value(sqlparser::ast::Value::SingleQuotedString(v)) + | Expr::Value(sqlparser::ast::Value::DoubleQuotedString(v)) => v, + Expr::Identifier(v) => v.value, + Expr::Value(sqlparser::ast::Value::Number(v, _)) => v.to_string(), + _ => return error::InvalidTableOptionValueSnafu { key, value }.fail(), + }; + let k = key.value.to_lowercase(); + Ok((k, v)) + }) + .collect::>>() + .unwrap(); + + AlterTableOperation::AlterColumnFulltext { column_name, options: options.into() } } else { return Err(ParserError::ParserError(format!( - "expect keyword ADD or DROP or MODIFY or RENAME after ALTER TABLE, found {}", + "expect keyword ADD or DROP or MODIFY or RENAME or SET after ALTER TABLE, found {}", self.parser.peek_token() ))); }; diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 5d679549a187..28eb9360a6ca 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -12,11 +12,14 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::fmt::{Debug, Display}; +use std::{collections::HashMap, fmt::{Debug, Display}, ops::ControlFlow}; use common_query::AddColumnLocation; use sqlparser::ast::{ColumnDef, DataType, Ident, ObjectName, TableConstraint}; use sqlparser_derive::{Visit, VisitMut}; +use sqlparser::ast::{Visit, VisitMut, Visitor, VisitorMut}; + +use super::OptionMap; #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut)] pub struct AlterTable { @@ -71,6 +74,11 @@ pub enum AlterTableOperation { DropColumn { name: Ident }, /// `RENAME ` RenameTable { new_table_name: String }, + /// `SET COLUMN FULLTEXT WITH` + AlterColumnFulltext { + column_name: Ident, + options: OptionMap, + }, } impl Display for AlterTableOperation { @@ -97,6 +105,12 @@ impl Display for AlterTableOperation { } => { write!(f, r#"MODIFY COLUMN {column_name} {target_type}"#) } + AlterTableOperation::AlterColumnFulltext { + column_name, + options, + } => { + write!(f, r#"SET COLUMN {column_name} FULLTEXT WITH {:?}"#, options.hash_options) + } } } } diff --git a/src/sql/src/statements/option_map.rs b/src/sql/src/statements/option_map.rs index 86f186d9b15d..3a7b61831b4e 100644 --- a/src/sql/src/statements/option_map.rs +++ b/src/sql/src/statements/option_map.rs @@ -25,6 +25,7 @@ const REDACTED_OPTIONS: [&str; 2] = ["access_key_id", "secret_access_key"]; pub struct OptionMap { options: BTreeMap, secrets: BTreeMap, + pub hash_options: HashMap, // TODO(renjj): move into a new struct } impl OptionMap { diff --git a/src/table/src/error.rs b/src/table/src/error.rs index 9b633bb5bb98..7c08e913eb2a 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -137,6 +137,14 @@ pub enum Error { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Failed to retrieve fulltext options from column metadata"))] + FulltextOptions { + #[snafu(implicit)] + location: Location, + source: datatypes::error::Error, + column_name: String, + }, } impl ErrorExt for Error { @@ -155,7 +163,9 @@ impl ErrorExt for Error { Error::TableOperation { source } => source.status_code(), Error::ColumnNotExists { .. } => StatusCode::TableColumnNotFound, Error::Unsupported { .. } => StatusCode::Unsupported, - Error::ParseTableOption { .. } => StatusCode::InvalidArguments, + Error::ParseTableOption { .. } | Error::FulltextOptions { .. } => { + StatusCode::InvalidArguments + } Error::MissingTimeIndexColumn { .. } => StatusCode::IllegalState, } } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index d01eb82a68c4..e701ae40753c 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -20,7 +20,9 @@ use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME}; use common_query::AddColumnLocation; use datafusion_expr::TableProviderFilterPushDown; pub use datatypes::error::{Error as ConvertError, Result as ConvertResult}; -use datatypes::schema::{ColumnSchema, RawSchema, Schema, SchemaBuilder, SchemaRef}; +use datatypes::schema::{ + ColumnSchema, FulltextOptions, RawSchema, Schema, SchemaBuilder, SchemaRef, +}; use derive_builder::Builder; use serde::{Deserialize, Serialize}; use snafu::{ensure, OptionExt, ResultExt}; @@ -208,6 +210,10 @@ impl TableMeta { .next_column_id(self.next_column_id); Ok(meta_builder) } + AlterKind::AddFulltexts { + column_name, + options, + } => self.change_column_fulltext(table_name, column_name, options.clone()), } } @@ -588,6 +594,39 @@ impl TableMeta { Ok(meta_builder) } + fn change_column_fulltext( + &self, + table_name: &str, + column_name: &String, + options: HashMap, + ) -> Result { + let table_schema = &self.schema; + let mut meta_builder = self.new_meta_builder(); + + let index = table_schema + .column_index_by_name(column_name) + .with_context(|| error::ColumnNotExistsSnafu { + column_name, + table_name, + })?; + let mut column = &table_schema.column_schemas()[index]; + + let fulltext_options = column + .fulltext_options() + .context(error::FulltextOptionsSnafu { column_name })?; + + let mut fulltext_options = match fulltext_options { + Some(fulltext_options) => fulltext_options, + None => FulltextOptions::default(), + }; + + let new_column = column.clone() + .with_fulltext_options(fulltext_options) + .context(error::FulltextOptionsSnafu { column_name })?; + + Ok(meta_builder) + } + /// Split requests into different groups using column location info. fn split_requests_by_column_location<'a>( &self, diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index a00b25eacb13..5b6c2df083fd 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -211,6 +211,10 @@ pub enum AlterKind { RenameTable { new_table_name: String, }, + AddFulltexts { + column_name: String, + options: HashMap, + } } #[derive(Debug)] From ebc5ced86421dae53f10fdfe3a6b306c6d0ba312 Mon Sep 17 00:00:00 2001 From: irenjj Date: Thu, 5 Sep 2024 23:41:03 +0800 Subject: [PATCH 02/20] update --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/common/grpc-expr/src/alter.rs | 12 ++- .../src/ddl/alter_table/region_request.rs | 2 +- .../src/ddl/alter_table/update_metadata.rs | 3 +- src/datatypes/src/schema/column_schema.rs | 8 ++ src/operator/src/expr_factory.rs | 4 +- src/sql/src/parsers/alter_parser.rs | 9 +- src/sql/src/statements/alter.rs | 15 +++- src/sql/src/statements/option_map.rs | 2 +- src/store-api/src/metadata.rs | 59 ++++++++++++- src/store-api/src/region_request.rs | 58 ++++++++++++- src/table/src/error.rs | 7 +- src/table/src/metadata.rs | 86 ++++++++++++++++--- src/table/src/requests.rs | 4 +- 15 files changed, 234 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4c1ad0ad4029..78c07a04ba5b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4252,7 +4252,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/irenjj/greptime-proto.git?rev=ca5920ee9263650c37556edad4bc877a674af444#ca5920ee9263650c37556edad4bc877a674af444" +source = "git+https://github.com/irenjj/greptime-proto.git?rev=d942e6bc84beaf48a59b00ce9064eafb16539336#d942e6bc84beaf48a59b00ce9064eafb16539336" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index d371c76781fc..8fe134ddc722 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,7 +119,7 @@ etcd-client = { version = "0.13" } fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "ca5920ee9263650c37556edad4bc877a674af444" } +greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "d942e6bc84beaf48a59b00ce9064eafb16539336" } humantime = "2.1" humantime-serde = "1.1" itertools = "0.10" diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index dc1b0c316b26..fa559117ad92 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -16,8 +16,8 @@ use api::helper::ColumnDataTypeWrapper; use api::v1::add_column_location::LocationType; use api::v1::alter_expr::Kind; use api::v1::{ - column_def, AddColumnLocation as Location, AlterExpr, ChangeColumnTypes, CreateTableExpr, - DropColumns, RenameTable, SemanticType, + column_def, AddColumnLocation as Location, ChangeFulltext, AlterExpr, ChangeColumnTypes, + CreateTableExpr, DropColumns, RenameTable, SemanticType, }; use common_query::AddColumnLocation; use datatypes::schema::{ColumnSchema, RawSchema}; @@ -92,7 +92,13 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result { AlterKind::RenameTable { new_table_name } } - Kind::AddFulltext(_) => { todo!() }, + Kind::ChangeFulltext(ChangeFulltext { + column_name, + options, + }) => AlterKind::ChangeFulltext { + column_name, + options, + }, }; let request = AlterTableRequest { diff --git a/src/common/meta/src/ddl/alter_table/region_request.rs b/src/common/meta/src/ddl/alter_table/region_request.rs index b5e7e7a632de..feba22523e4c 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,7 @@ fn create_proto_alter_kind( }))) } Kind::RenameTable(_) => Ok(None), - Kind::AddFulltext(_) => todo!(), + Kind::ChangeFulltext(x) => Ok(Some(alter_request::Kind::ChangeFulltex(x.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 8fe513a6ab2a..1c94e9099301 100644 --- a/src/common/meta/src/ddl/alter_table/update_metadata.rs +++ b/src/common/meta/src/ddl/alter_table/update_metadata.rs @@ -52,8 +52,7 @@ impl AlterTableProcedure { new_info.name = new_table_name.to_string(); } AlterKind::DropColumns { .. } | AlterKind::ChangeColumnTypes { .. } => {} - AlterKind::AddFulltexts { column_name, options } => { - }, + AlterKind::ChangeFulltext { .. } => {} } Ok(new_info) diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index c3cd8b345314..07443535e546 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -262,6 +262,14 @@ impl ColumnSchema { ); Ok(self) } + + pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> { + self.metadata.insert( + FULLTEXT_KEY.to_string(), + serde_json::to_string(&options).context(error::SerializeSnafu)?, + ); + Ok(()) + } } impl TryFrom<&Field> for ColumnSchema { diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index e25bd2fc9482..85f1c9105e8e 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -18,7 +18,7 @@ use api::helper::ColumnDataTypeWrapper; use api::v1::alter_expr::Kind; use api::v1::column_def::options_from_column_schema; use api::v1::{ - AddColumn, AddColumns, AddFulltext, AlterExpr, ChangeColumnType, ChangeColumnTypes, + AddColumn, AddColumns, ChangeFulltext, AlterExpr, ChangeColumnType, ChangeColumnTypes, ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropColumn, DropColumns, ExpireAfter, RenameTable, SemanticType, TableName, }; @@ -486,7 +486,7 @@ pub(crate) fn to_alter_expr( AlterTableOperation::AlterColumnFulltext { column_name, options, - } => Kind::AddFulltext(AddFulltext { + } => Kind::ChangeFulltext(ChangeFulltext { column_name: column_name.value.to_string(), options: options.hash_options.clone(), }), diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 8ce9e08169b6..35d7041d5508 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -103,7 +103,7 @@ impl<'a> ParserContext<'a> { let _ = self.parser.parse_keyword(Keyword::FULLTEXT); - let options= self + let options = self .parser .parse_options(Keyword::WITH)? .into_iter() @@ -122,7 +122,12 @@ impl<'a> ParserContext<'a> { .collect::>>() .unwrap(); - AlterTableOperation::AlterColumnFulltext { column_name, options: options.into() } + println!("options:{:?}", options); + + AlterTableOperation::AlterColumnFulltext { + column_name, + options: options.into(), + } } else { return Err(ParserError::ParserError(format!( "expect keyword ADD or DROP or MODIFY or RENAME or SET after ALTER TABLE, found {}", diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 28eb9360a6ca..54566a25af0e 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -12,12 +12,15 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{collections::HashMap, fmt::{Debug, Display}, ops::ControlFlow}; +use std::collections::HashMap; +use std::fmt::{Debug, Display}; +use std::ops::ControlFlow; use common_query::AddColumnLocation; -use sqlparser::ast::{ColumnDef, DataType, Ident, ObjectName, TableConstraint}; +use sqlparser::ast::{ + ColumnDef, DataType, Ident, ObjectName, TableConstraint, Visit, VisitMut, Visitor, VisitorMut, +}; use sqlparser_derive::{Visit, VisitMut}; -use sqlparser::ast::{Visit, VisitMut, Visitor, VisitorMut}; use super::OptionMap; @@ -109,7 +112,11 @@ impl Display for AlterTableOperation { column_name, options, } => { - write!(f, r#"SET COLUMN {column_name} FULLTEXT WITH {:?}"#, options.hash_options) + write!( + f, + r#"SET COLUMN {column_name} FULLTEXT WITH {:?}"#, + options.hash_options + ) } } } diff --git a/src/sql/src/statements/option_map.rs b/src/sql/src/statements/option_map.rs index 3a7b61831b4e..5955ae16d3bf 100644 --- a/src/sql/src/statements/option_map.rs +++ b/src/sql/src/statements/option_map.rs @@ -25,7 +25,7 @@ const REDACTED_OPTIONS: [&str; 2] = ["access_key_id", "secret_access_key"]; pub struct OptionMap { options: BTreeMap, secrets: BTreeMap, - pub hash_options: HashMap, // TODO(renjj): move into a new struct + pub hash_options: HashMap, // TODO(renjj): move into a new struct } impl OptionMap { diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index a94879675e5b..8dd78ff36924 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -28,7 +28,7 @@ use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; use common_macro::stack_trace_debug; use datatypes::arrow::datatypes::FieldRef; -use datatypes::schema::{ColumnSchema, Schema, SchemaRef}; +use datatypes::schema::{ColumnSchema, FulltextAnalyzer, FulltextOptions, Schema, SchemaRef}; use serde::de::Error; use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; @@ -39,6 +39,9 @@ use crate::storage::{ColumnId, RegionId}; pub type Result = std::result::Result; +const COLUMN_FULLTEXT_OPT_KEY_ANALYZER: &str = "analyzer"; +const COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE: &str = "case_sensitive"; + /// Metadata of a column. #[derive(Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct ColumnMetadata { @@ -523,6 +526,10 @@ impl RegionMetadataBuilder { AlterKind::AddColumns { columns } => self.add_columns(columns)?, AlterKind::DropColumns { names } => self.drop_columns(&names), AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns), + AlterKind::ChangeColumnFulltext { + column_name, + options, + } => self.change_column_fulltext(column_name, options), } Ok(self) } @@ -622,6 +629,49 @@ impl RegionMetadataBuilder { } } } + + /// Changes column fulltext option. + fn change_column_fulltext(&mut self, column_name: String, options: HashMap) { + println!("fulltext_options: {:?}", options); + + for column_meta in self.column_metadatas.iter_mut() { + let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() { + f + } else { + FulltextOptions::default() + }; + + if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { + match analyzer.to_ascii_lowercase().as_str() { + "english" => { + fulltext.enable = true; + fulltext.analyzer = FulltextAnalyzer::English; + } + "chinese" => { + fulltext.enable = true; + fulltext.analyzer = FulltextAnalyzer::Chinese; + } + _ => {} + } + } + if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { + match case_sensitive.to_ascii_lowercase().as_str() { + "true" => { + fulltext.enable = true; + fulltext.case_sensitive = true; + } + "false" => { + fulltext.enable = true; + fulltext.case_sensitive = false; + } + _ => {} + } + } + println!("column_meta.column_schema1: {:?}", column_meta.column_schema); + column_meta.column_schema.set_fulltext_options(fulltext).expect("set fulltext"); + println!("column_meta.column_schema2: {:?}", column_meta.column_schema); + } + } } /// Fields skipped in serialization. @@ -738,6 +788,13 @@ pub enum MetadataError { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Invalid fulltext options"))] + InvalidFulltextOptions { + #[snafu(implicit)] + location: Location, + column_name: String, + }, } impl ErrorExt for MetadataError { diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 1452fcbe6125..1e4aaf654006 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -30,12 +30,15 @@ use strum::IntoStaticStr; use crate::logstore::entry; use crate::metadata::{ - ColumnMetadata, InvalidRawRegionRequestSnafu, InvalidRegionRequestSnafu, MetadataError, - RegionMetadata, Result, + ColumnMetadata, InvalidFulltextOptionsSnafu, InvalidRawRegionRequestSnafu, + InvalidRegionRequestSnafu, MetadataError, RegionMetadata, Result, }; use crate::path_utils::region_dir; use crate::storage::{ColumnId, RegionId, ScanRequest}; +const COLUMN_FULLTEXT_OPT_KEY_ANALYZER: &str = "analyzer"; +const COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE: &str = "case_sensitive"; + #[derive(Debug, IntoStaticStr)] pub enum RegionRequest { Put(RegionPutRequest), @@ -389,6 +392,13 @@ pub enum AlterKind { /// Columns to change. columns: Vec, }, + /// Change columns fulltext from the region, only fields are allowed to change. + ChangeColumnFulltext { + /// Name of column to change. + column_name: String, + /// Fulltext options. + options: HashMap, + }, } impl AlterKind { @@ -412,6 +422,10 @@ impl AlterKind { col_to_change.validate(metadata)?; } } + AlterKind::ChangeColumnFulltext { + column_name, + options, + } => Self::validate_column_fulltext_to_alter(metadata, column_name, options)?, } Ok(()) } @@ -429,6 +443,9 @@ impl AlterKind { AlterKind::ChangeColumnTypes { columns } => columns .iter() .any(|col_to_change| col_to_change.need_alter(metadata)), + AlterKind::ChangeColumnFulltext { column_name, .. } => { + metadata.column_by_name(column_name).is_some() + } } } @@ -446,6 +463,39 @@ impl AlterKind { ); Ok(()) } + + pub fn validate_column_fulltext_to_alter( + metadata: &RegionMetadata, + column_name: &str, + options: &HashMap, + ) -> Result<()> { + let _ = + metadata + .column_by_name(column_name) + .with_context(|| InvalidRegionRequestSnafu { + region_id: metadata.region_id, + err: format!("column {} not found", column_name), + })?; + + for (k, v) in options { + if k.to_ascii_lowercase().eq(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { + if v.to_ascii_lowercase().ne("english") && v.to_ascii_lowercase().ne("chinese") { + return InvalidFulltextOptionsSnafu { column_name }.fail(); + } + } else if k + .to_ascii_lowercase() + .eq(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) + { + if v.to_ascii_lowercase().ne("true") && v.to_ascii_lowercase().ne("false") { + return InvalidFulltextOptionsSnafu { column_name }.fail(); + } + } else { + return InvalidFulltextOptionsSnafu { column_name }.fail(); + } + } + + Ok(()) + } } impl TryFrom for AlterKind { @@ -473,6 +523,10 @@ impl TryFrom for AlterKind { let names = x.drop_columns.into_iter().map(|x| x.name).collect(); AlterKind::DropColumns { names } } + alter_request::Kind::ChangeFulltex(x) => AlterKind::ChangeColumnFulltext { + column_name: x.column_name, + options: x.options, + }, }; Ok(alter_kind) diff --git a/src/table/src/error.rs b/src/table/src/error.rs index 7c08e913eb2a..6e7debdcf912 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -138,11 +138,10 @@ pub enum Error { location: Location, }, - #[snafu(display("Failed to retrieve fulltext options from column metadata"))] - FulltextOptions { + #[snafu(display("Invalid fulltext options"))] + InvalidFulltextOptions { #[snafu(implicit)] location: Location, - source: datatypes::error::Error, column_name: String, }, } @@ -163,7 +162,7 @@ impl ErrorExt for Error { Error::TableOperation { source } => source.status_code(), Error::ColumnNotExists { .. } => StatusCode::TableColumnNotFound, Error::Unsupported { .. } => StatusCode::Unsupported, - Error::ParseTableOption { .. } | Error::FulltextOptions { .. } => { + Error::ParseTableOption { .. } | Error::InvalidFulltextOptions { .. } => { StatusCode::InvalidArguments } Error::MissingTimeIndexColumn { .. } => StatusCode::IllegalState, diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index e701ae40753c..87b1c58b3c83 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -21,7 +21,7 @@ use common_query::AddColumnLocation; use datafusion_expr::TableProviderFilterPushDown; pub use datatypes::error::{Error as ConvertError, Result as ConvertResult}; use datatypes::schema::{ - ColumnSchema, FulltextOptions, RawSchema, Schema, SchemaBuilder, SchemaRef, + ColumnSchema, FulltextAnalyzer, FulltextOptions, RawSchema, Schema, SchemaBuilder, SchemaRef, }; use derive_builder::Builder; use serde::{Deserialize, Serialize}; @@ -34,6 +34,9 @@ use crate::requests::{AddColumnRequest, AlterKind, ChangeColumnTypeRequest, Tabl pub type TableId = u32; pub type TableVersion = u64; +const COLUMN_FULLTEXT_OPT_KEY_ANALYZER: &str = "analyzer"; +const COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE: &str = "case_sensitive"; + /// Indicates whether and how a filter expression can be handled by a /// Table for table scans. #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)] @@ -210,7 +213,7 @@ impl TableMeta { .next_column_id(self.next_column_id); Ok(meta_builder) } - AlterKind::AddFulltexts { + AlterKind::ChangeFulltext { column_name, options, } => self.change_column_fulltext(table_name, column_name, options.clone()), @@ -600,6 +603,7 @@ impl TableMeta { column_name: &String, options: HashMap, ) -> Result { + println!("{:?}", options); let table_schema = &self.schema; let mut meta_builder = self.new_meta_builder(); @@ -609,20 +613,76 @@ impl TableMeta { column_name, table_name, })?; - let mut column = &table_schema.column_schemas()[index]; - - let fulltext_options = column - .fulltext_options() - .context(error::FulltextOptionsSnafu { column_name })?; + let column = &table_schema.column_schemas()[index]; - let mut fulltext_options = match fulltext_options { - Some(fulltext_options) => fulltext_options, - None => FulltextOptions::default(), + let mut fulltext = if let Ok(Some(f)) = column.fulltext_options() { + f + } else { + FulltextOptions::default() }; - let new_column = column.clone() - .with_fulltext_options(fulltext_options) - .context(error::FulltextOptionsSnafu { column_name })?; + if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { + match analyzer.to_ascii_lowercase().as_str() { + "english" => { + fulltext.enable = true; + fulltext.analyzer = FulltextAnalyzer::English; + } + "chinese" => { + fulltext.enable = true; + fulltext.analyzer = FulltextAnalyzer::Chinese; + } + _ => { + return error::InvalidFulltextOptionsSnafu { column_name }.fail()?; + } + } + } + if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { + match case_sensitive.to_ascii_lowercase().as_str() { + "true" => { + fulltext.enable = true; + fulltext.case_sensitive = true; + } + "false" => { + fulltext.enable = true; + fulltext.case_sensitive = false; + } + _ => { + return error::InvalidFulltextOptionsSnafu { column_name }.fail()?; + } + } + } + + let columns: Vec<_> = table_schema + .column_schemas() + .iter() + .cloned() + .map(|mut column| { + if column.name == *column_name { + column = column + .clone() + .with_fulltext_options(fulltext.clone()) + .unwrap(); + } + column + }) + .collect(); + + 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 datatype with columns {column_name}"), + })?; + + let _ = meta_builder + .schema(Arc::new(new_schema)) + .primary_key_indices(self.primary_key_indices.clone()); Ok(meta_builder) } diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 5b6c2df083fd..24d756943354 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -211,10 +211,10 @@ pub enum AlterKind { RenameTable { new_table_name: String, }, - AddFulltexts { + ChangeFulltext { column_name: String, options: HashMap, - } + }, } #[derive(Debug)] From 1cfc57b77c3d60998c5abe3571fc2d3b2c4b7fbf Mon Sep 17 00:00:00 2001 From: irenjj Date: Sat, 7 Sep 2024 14:41:57 +0800 Subject: [PATCH 03/20] fix hash_options empty --- src/sql/src/parsers/alter_parser.rs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 35d7041d5508..e49051068dbf 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -1,5 +1,3 @@ -// Copyright 2023 Greptime Team -// // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -12,7 +10,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::HashMap; +use std::collections::{BTreeMap, HashMap}; use common_query::AddColumnLocation; use snafu::ResultExt; @@ -25,10 +23,12 @@ use crate::error::{self, Result}; use crate::parser::ParserContext; use crate::statements::alter::{AlterTable, AlterTableOperation}; use crate::statements::statement::Statement; +use crate::statements::OptionMap; impl<'a> ParserContext<'a> { pub(crate) fn parse_alter(&mut self) -> Result { let alter_table = self.parse_alter_table().context(error::SyntaxSnafu)?; + println!("{:?}", alter_table); Ok(Statement::Alter(alter_table)) } @@ -103,7 +103,7 @@ impl<'a> ParserContext<'a> { let _ = self.parser.parse_keyword(Keyword::FULLTEXT); - let options = self + let fulltext_options = self .parser .parse_options(Keyword::WITH)? .into_iter() @@ -122,11 +122,12 @@ impl<'a> ParserContext<'a> { .collect::>>() .unwrap(); - println!("options:{:?}", options); + let mut options = ::default(); + options.hash_options = fulltext_options; AlterTableOperation::AlterColumnFulltext { column_name, - options: options.into(), + options, } } else { return Err(ParserError::ParserError(format!( From 8f94e7f4e7d23dda861da59afe0fce55c48b793d Mon Sep 17 00:00:00 2001 From: irenjj Date: Sat, 7 Sep 2024 16:48:20 +0800 Subject: [PATCH 04/20] refactor & add test --- src/operator/src/expr_factory.rs | 2 +- src/sql/src/parsers/alter_parser.rs | 11 +- src/sql/src/statements/alter.rs | 8 +- src/sql/src/statements/option_map.rs | 1 - src/store-api/src/metadata.rs | 10 +- src/table/src/metadata.rs | 1 - .../explain/analyze_append_table_count.result | 14 +- .../alter/change_fulltext_options.result | 198 ++++++++++++++++++ .../common/alter/change_fulltext_options.sql | 42 ++++ 9 files changed, 256 insertions(+), 31 deletions(-) create mode 100644 tests/cases/standalone/common/alter/change_fulltext_options.result create mode 100644 tests/cases/standalone/common/alter/change_fulltext_options.sql diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 85f1c9105e8e..8d5c818de38b 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -488,7 +488,7 @@ pub(crate) fn to_alter_expr( options, } => Kind::ChangeFulltext(ChangeFulltext { column_name: column_name.value.to_string(), - options: options.hash_options.clone(), + options: options.clone().into_map(), }), }; diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index e49051068dbf..f6444265e782 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -10,7 +10,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::{BTreeMap, HashMap}; +use std::collections::HashMap; use common_query::AddColumnLocation; use snafu::ResultExt; @@ -23,12 +23,10 @@ use crate::error::{self, Result}; use crate::parser::ParserContext; use crate::statements::alter::{AlterTable, AlterTableOperation}; use crate::statements::statement::Statement; -use crate::statements::OptionMap; impl<'a> ParserContext<'a> { pub(crate) fn parse_alter(&mut self) -> Result { let alter_table = self.parse_alter_table().context(error::SyntaxSnafu)?; - println!("{:?}", alter_table); Ok(Statement::Alter(alter_table)) } @@ -103,7 +101,7 @@ impl<'a> ParserContext<'a> { let _ = self.parser.parse_keyword(Keyword::FULLTEXT); - let fulltext_options = self + let options = self .parser .parse_options(Keyword::WITH)? .into_iter() @@ -122,12 +120,9 @@ impl<'a> ParserContext<'a> { .collect::>>() .unwrap(); - let mut options = ::default(); - options.hash_options = fulltext_options; - AlterTableOperation::AlterColumnFulltext { column_name, - options, + options: options.into(), } } else { return Err(ParserError::ParserError(format!( diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 54566a25af0e..2cf7d91ec2cd 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -12,14 +12,10 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::HashMap; use std::fmt::{Debug, Display}; -use std::ops::ControlFlow; use common_query::AddColumnLocation; -use sqlparser::ast::{ - ColumnDef, DataType, Ident, ObjectName, TableConstraint, Visit, VisitMut, Visitor, VisitorMut, -}; +use sqlparser::ast::{ColumnDef, DataType, Ident, ObjectName, TableConstraint}; use sqlparser_derive::{Visit, VisitMut}; use super::OptionMap; @@ -115,7 +111,7 @@ impl Display for AlterTableOperation { write!( f, r#"SET COLUMN {column_name} FULLTEXT WITH {:?}"#, - options.hash_options + options.to_str_map() ) } } diff --git a/src/sql/src/statements/option_map.rs b/src/sql/src/statements/option_map.rs index 5955ae16d3bf..86f186d9b15d 100644 --- a/src/sql/src/statements/option_map.rs +++ b/src/sql/src/statements/option_map.rs @@ -25,7 +25,6 @@ const REDACTED_OPTIONS: [&str; 2] = ["access_key_id", "secret_access_key"]; pub struct OptionMap { options: BTreeMap, secrets: BTreeMap, - pub hash_options: HashMap, // TODO(renjj): move into a new struct } impl OptionMap { diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 8dd78ff36924..642cc5926272 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -527,9 +527,9 @@ impl RegionMetadataBuilder { AlterKind::DropColumns { names } => self.drop_columns(&names), AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns), AlterKind::ChangeColumnFulltext { - column_name, + column_name: _, options, - } => self.change_column_fulltext(column_name, options), + } => self.change_column_fulltext(options), } Ok(self) } @@ -631,9 +631,7 @@ impl RegionMetadataBuilder { } /// Changes column fulltext option. - fn change_column_fulltext(&mut self, column_name: String, options: HashMap) { - println!("fulltext_options: {:?}", options); - + fn change_column_fulltext(&mut self, options: HashMap) { for column_meta in self.column_metadatas.iter_mut() { let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() { f @@ -667,9 +665,7 @@ impl RegionMetadataBuilder { _ => {} } } - println!("column_meta.column_schema1: {:?}", column_meta.column_schema); column_meta.column_schema.set_fulltext_options(fulltext).expect("set fulltext"); - println!("column_meta.column_schema2: {:?}", column_meta.column_schema); } } } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 87b1c58b3c83..50aa8791c393 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -603,7 +603,6 @@ impl TableMeta { column_name: &String, options: HashMap, ) -> Result { - println!("{:?}", options); let table_schema = &self.schema; let mut meta_builder = self.new_meta_builder(); diff --git a/tests/cases/distributed/explain/analyze_append_table_count.result b/tests/cases/distributed/explain/analyze_append_table_count.result index 7a2a1603f205..52734ae2ebbb 100644 --- a/tests/cases/distributed/explain/analyze_append_table_count.result +++ b/tests/cases/distributed/explain/analyze_append_table_count.result @@ -21,14 +21,14 @@ Affected Rows: 0 -- SQLNESS REPLACE (peers.*) REDACTED EXPLAIN ANALYZE SELECT count(*) FROM test_table; -+-------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ -| stage | node | plan | -+-------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ ++-------+------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ +| stage | node | plan | ++-------+------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | 0 | 0 | MergeScanExec: REDACTED -| | | | +| | | | | 1 | 0 | ProjectionExec: expr=[0 as COUNT(test_table.timestamp)] REDACTED | | | common_recordbatch::adapter::MetricCollector REDACTED -| | | | -| | | Total rows: 1 | -+-------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ +| | | | +| | | Total rows: 1 | ++-------+------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result new file mode 100644 index 000000000000..44e7319f115f --- /dev/null +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -0,0 +1,198 @@ +CREATE TABLE `test` ( + `message` STRING FULLTEXT, + `time` TIMESTAMP TIME INDEX, +) WITH ( + append_mode = 'true' +); + +Affected Rows: 0 + +SHOW CREATE TABLE test; + ++-------+----------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+----------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+----------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'true'); + +Affected Rows: 0 + +SHOW CREATE TABLE test; + ++-------+---------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+---------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+---------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'false'); + +Affected Rows: 0 + +SHOW CREATE TABLE test; + ++-------+----------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+----------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+----------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'true'); + +Affected Rows: 0 + +SHOW CREATE TABLE test; + ++-------+---------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+---------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+---------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'false'); + +Affected Rows: 0 + +SHOW CREATE TABLE test; + ++-------+----------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+----------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+----------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'yes'); + +Error: 1004(InvalidArguments), Invalid fulltext options + +SHOW CREATE TABLE test; + ++-------+----------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+----------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+----------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinglish', case_sensitive = 'false'); + +Error: 1004(InvalidArguments), Invalid fulltext options + +SHOW CREATE TABLE test; + ++-------+----------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+----------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+----------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'false'); + +Affected Rows: 0 + +SHOW CREATE TABLE test; + ++-------+-----------------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+-----------------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'), | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+-----------------------------------------------------------------------------------------------+ + +ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'no'); + +Error: 1004(InvalidArguments), Invalid fulltext options + +SHOW CREATE TABLE test; + ++-------+-----------------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+-----------------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'), | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+-----------------------------------------------------------------------------------------------+ + +DROP TABLE test; + +Affected Rows: 0 + diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.sql b/tests/cases/standalone/common/alter/change_fulltext_options.sql new file mode 100644 index 000000000000..51a8fb88233a --- /dev/null +++ b/tests/cases/standalone/common/alter/change_fulltext_options.sql @@ -0,0 +1,42 @@ +CREATE TABLE `test` ( + `message` STRING FULLTEXT, + `time` TIMESTAMP TIME INDEX, +) WITH ( + append_mode = 'true' +); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'true'); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'false'); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'true'); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'false'); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'yes'); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinglish', case_sensitive = 'false'); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'false'); + +SHOW CREATE TABLE test; + +ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'no'); + +SHOW CREATE TABLE test; + +DROP TABLE test; From 8b7f57fae46753127a8bccc83e093af5599caab7 Mon Sep 17 00:00:00 2001 From: irenjj Date: Sat, 7 Sep 2024 16:52:19 +0800 Subject: [PATCH 05/20] fix --- .../explain/analyze_append_table_count.result | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/tests/cases/distributed/explain/analyze_append_table_count.result b/tests/cases/distributed/explain/analyze_append_table_count.result index 52734ae2ebbb..7a2a1603f205 100644 --- a/tests/cases/distributed/explain/analyze_append_table_count.result +++ b/tests/cases/distributed/explain/analyze_append_table_count.result @@ -21,14 +21,14 @@ Affected Rows: 0 -- SQLNESS REPLACE (peers.*) REDACTED EXPLAIN ANALYZE SELECT count(*) FROM test_table; -+-------+------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ -| stage | node | plan | -+-------+------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ ++-------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ +| stage | node | plan | ++-------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ | 0 | 0 | MergeScanExec: REDACTED -| | | | +| | | | | 1 | 0 | ProjectionExec: expr=[0 as COUNT(test_table.timestamp)] REDACTED | | | common_recordbatch::adapter::MetricCollector REDACTED -| | | | -| | | Total rows: 1 | -+-------+------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ +| | | | +| | | Total rows: 1 | ++-------+------+------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+ From 093c5c64ec495bad67d5bc47d7fcbd9d0b16bddc Mon Sep 17 00:00:00 2001 From: irenjj Date: Sat, 7 Sep 2024 20:07:09 +0800 Subject: [PATCH 06/20] change alter syntax --- src/sql/src/parsers/alter_parser.rs | 66 +++++++++---------- .../alter/change_fulltext_options.result | 16 ++--- .../common/alter/change_fulltext_options.sql | 16 ++--- 3 files changed, 49 insertions(+), 49 deletions(-) diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index f6444265e782..25d8fd14eeb5 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -77,11 +77,40 @@ impl<'a> ParserContext<'a> { } else if self.consume_token("MODIFY") { let _ = self.parser.parse_keyword(Keyword::COLUMN); let column_name = Self::canonicalize_identifier(self.parser.parse_identifier(false)?); - let target_type = self.parser.parse_data_type()?; - AlterTableOperation::ChangeColumnType { - column_name, - target_type, + if self.parser.parse_keyword(Keyword::SET) { + let _ = self.parser.parse_keyword(Keyword::FULLTEXT); + + let options = self + .parser + .parse_options(Keyword::WITH)? + .into_iter() + .map(|option: SqlOption| -> Result<(String, String)> { + let (key, value) = (option.name, option.value); + let v = match value { + Expr::Value(sqlparser::ast::Value::SingleQuotedString(v)) + | Expr::Value(sqlparser::ast::Value::DoubleQuotedString(v)) => v, + Expr::Identifier(v) => v.value, + Expr::Value(sqlparser::ast::Value::Number(v, _)) => v.to_string(), + _ => return error::InvalidTableOptionValueSnafu { key, value }.fail(), + }; + let k = key.value.to_lowercase(); + Ok((k, v)) + }) + .collect::>>() + .unwrap(); + + AlterTableOperation::AlterColumnFulltext { + column_name, + options: options.into(), + } + } else { + let target_type = self.parser.parse_data_type()?; + + AlterTableOperation::ChangeColumnType { + column_name, + target_type, + } } } else if self.parser.parse_keyword(Keyword::RENAME) { let new_table_name_obj_raw = self.parse_object_name()?; @@ -95,35 +124,6 @@ impl<'a> ParserContext<'a> { } }; AlterTableOperation::RenameTable { new_table_name } - } else if self.parser.parse_keyword(Keyword::SET) { - let _ = self.parser.parse_keyword(Keyword::COLUMN); - let column_name = Self::canonicalize_identifier(self.parser.parse_identifier(false)?); - - let _ = self.parser.parse_keyword(Keyword::FULLTEXT); - - let options = self - .parser - .parse_options(Keyword::WITH)? - .into_iter() - .map(|option: SqlOption| -> Result<(String, String)> { - let (key, value) = (option.name, option.value); - let v = match value { - Expr::Value(sqlparser::ast::Value::SingleQuotedString(v)) - | Expr::Value(sqlparser::ast::Value::DoubleQuotedString(v)) => v, - Expr::Identifier(v) => v.value, - Expr::Value(sqlparser::ast::Value::Number(v, _)) => v.to_string(), - _ => return error::InvalidTableOptionValueSnafu { key, value }.fail(), - }; - let k = key.value.to_lowercase(); - Ok((k, v)) - }) - .collect::>>() - .unwrap(); - - AlterTableOperation::AlterColumnFulltext { - column_name, - options: options.into(), - } } else { return Err(ParserError::ParserError(format!( "expect keyword ADD or DROP or MODIFY or RENAME or SET after ALTER TABLE, found {}", diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result index 44e7319f115f..b3cc77e95d8e 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -24,7 +24,7 @@ SHOW CREATE TABLE test; | | ) | +-------+----------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'true'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true'); Affected Rows: 0 @@ -45,7 +45,7 @@ SHOW CREATE TABLE test; | | ) | +-------+---------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); Affected Rows: 0 @@ -66,7 +66,7 @@ SHOW CREATE TABLE test; | | ) | +-------+----------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'true'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'); Affected Rows: 0 @@ -87,7 +87,7 @@ SHOW CREATE TABLE test; | | ) | +-------+---------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'); Affected Rows: 0 @@ -108,7 +108,7 @@ SHOW CREATE TABLE test; | | ) | +-------+----------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'yes'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'yes'); Error: 1004(InvalidArguments), Invalid fulltext options @@ -129,7 +129,7 @@ SHOW CREATE TABLE test; | | ) | +-------+----------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinglish', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinglish', case_sensitive = 'false'); Error: 1004(InvalidArguments), Invalid fulltext options @@ -150,7 +150,7 @@ SHOW CREATE TABLE test; | | ) | +-------+----------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); Affected Rows: 0 @@ -171,7 +171,7 @@ SHOW CREATE TABLE test; | | ) | +-------+-----------------------------------------------------------------------------------------------+ -ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'no'); +ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'no'); Error: 1004(InvalidArguments), Invalid fulltext options diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.sql b/tests/cases/standalone/common/alter/change_fulltext_options.sql index 51a8fb88233a..d29549cff614 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.sql +++ b/tests/cases/standalone/common/alter/change_fulltext_options.sql @@ -7,35 +7,35 @@ CREATE TABLE `test` ( SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'true'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true'); SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinese', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'true'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'true'); SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'); SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN message WITH(analyzer = 'English', case_sensitive = 'yes'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'yes'); SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN message WITH(analyzer = 'Chinglish', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinglish', case_sensitive = 'false'); SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'false'); +ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); SHOW CREATE TABLE test; -ALTER TABLE test SET COLUMN time WITH(analyzer = 'Chinese', case_sensitive = 'no'); +ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'no'); SHOW CREATE TABLE test; From 7ac9a2001097276e413e853251a2e96115605f62 Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 11 Sep 2024 20:45:21 +0800 Subject: [PATCH 07/20] Update src/store-api/src/metadata.rs Co-authored-by: Weny Xu --- src/store-api/src/metadata.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 642cc5926272..fe00fcfcb027 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -785,7 +785,7 @@ pub enum MetadataError { location: Location, }, - #[snafu(display("Invalid fulltext options"))] + #[snafu(display("Invalid fulltext options, column: {}", column_name))] InvalidFulltextOptions { #[snafu(implicit)] location: Location, From 1543268c7f23117f5657dfeb55840c182bf58f20 Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 11 Sep 2024 20:45:37 +0800 Subject: [PATCH 08/20] Update src/sql/src/statements/alter.rs Co-authored-by: Weny Xu --- src/sql/src/statements/alter.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 2cf7d91ec2cd..1166d58512ac 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -18,7 +18,7 @@ use common_query::AddColumnLocation; use sqlparser::ast::{ColumnDef, DataType, Ident, ObjectName, TableConstraint}; use sqlparser_derive::{Visit, VisitMut}; -use super::OptionMap; +use crate::statements::OptionMap; #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut)] pub struct AlterTable { From 3601a401728c920a201f206f98e8f792392f45d1 Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 11 Sep 2024 21:23:23 +0800 Subject: [PATCH 09/20] simplify --- Cargo.toml | 2 +- src/datatypes/src/schema.rs | 45 ++++++++++++++ src/datatypes/src/schema/column_schema.rs | 19 +++--- src/sql/src/parsers/alter_parser.rs | 22 +++---- src/sql/src/statements.rs | 2 +- src/store-api/src/metadata.rs | 52 ++++++---------- src/store-api/src/region_request.rs | 5 +- src/table/src/error.rs | 2 +- src/table/src/metadata.rs | 46 +++----------- .../alter/change_fulltext_options.result | 62 +++++++++---------- 10 files changed, 126 insertions(+), 131 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 8fe134ddc722..3d9ccec79266 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -151,7 +151,7 @@ reqwest = { version = "0.12", default-features = false, features = [ "stream", "multipart", ] } -# SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 +# SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 rskafka = { git = "https://github.com/WenyXu/rskafka.git", rev = "940c6030012c5b746fad819fb72e3325b26e39de", features = [ "transport-tls", ] } diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index 3bb35a595f4a..21bf921ca688 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -32,6 +32,9 @@ pub use crate::schema::column_schema::{ pub use crate::schema::constraint::ColumnDefaultConstraint; pub use crate::schema::raw::RawSchema; +const COLUMN_FULLTEXT_OPT_KEY_ANALYZER: &str = "analyzer"; +const COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE: &str = "case_sensitive"; + /// Key used to store version number of the schema in metadata. pub const VERSION_KEY: &str = "greptime:version"; @@ -300,6 +303,48 @@ fn validate_timestamp_index(column_schemas: &[ColumnSchema], timestamp_index: us Ok(()) } +pub fn validate_fulltext_options( + fulltext: &mut FulltextOptions, + options: &HashMap, +) -> bool { + let mut is_fulltext_key_exist = false; + if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { + match analyzer.to_ascii_lowercase().as_str() { + "english" => { + fulltext.enable = true; + fulltext.analyzer = FulltextAnalyzer::English; + } + "chinese" => { + fulltext.enable = true; + fulltext.analyzer = FulltextAnalyzer::Chinese; + } + _ => { + return false; + } + } + is_fulltext_key_exist = true; + } + + if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { + match case_sensitive.to_ascii_lowercase().as_str() { + "true" => { + fulltext.enable = true; + fulltext.case_sensitive = true; + } + "false" => { + fulltext.enable = true; + fulltext.case_sensitive = false; + } + _ => { + return false; + } + } + is_fulltext_key_exist = true; + } + + is_fulltext_key_exist +} + pub type SchemaRef = Arc; impl TryFrom> for Schema { diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index 07443535e546..ea2ac20b64cf 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -255,19 +255,18 @@ impl ColumnSchema { } } - pub fn with_fulltext_options(mut self, options: FulltextOptions) -> Result { - self.metadata.insert( - FULLTEXT_KEY.to_string(), - serde_json::to_string(&options).context(error::SerializeSnafu)?, - ); + pub fn with_fulltext_options(mut self, options: &FulltextOptions) -> Result { + self.set_fulltext_options(options)?; Ok(self) } - pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> { - self.metadata.insert( - FULLTEXT_KEY.to_string(), - serde_json::to_string(&options).context(error::SerializeSnafu)?, - ); + pub fn set_fulltext_options(&mut self, options: &FulltextOptions) -> Result<()> { + if self.data_type == ConcreteDataType::string_datatype() { + self.metadata.insert( + FULLTEXT_KEY.to_string(), + serde_json::to_string(&options).context(error::SerializeSnafu)?, + ); + } Ok(()) } } diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 25d8fd14eeb5..524ad40cb428 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -14,7 +14,6 @@ use std::collections::HashMap; use common_query::AddColumnLocation; use snafu::ResultExt; -use sqlparser::ast::{Expr, SqlOption}; use sqlparser::keywords::Keyword; use sqlparser::parser::ParserError; use sqlparser::tokenizer::Token; @@ -23,6 +22,7 @@ use crate::error::{self, Result}; use crate::parser::ParserContext; use crate::statements::alter::{AlterTable, AlterTableOperation}; use crate::statements::statement::Statement; +use crate::util::parse_option_string; impl<'a> ParserContext<'a> { pub(crate) fn parse_alter(&mut self) -> Result { @@ -75,7 +75,12 @@ impl<'a> ParserContext<'a> { ))); } } else if self.consume_token("MODIFY") { - let _ = self.parser.parse_keyword(Keyword::COLUMN); + if !self.parser.parse_keyword(Keyword::COLUMN) { + return Err(ParserError::ParserError(format!( + "expect keyword COLUMN after ALTER TABLE MODIFY, found {}", + self.parser.peek_token() + ))); + } let column_name = Self::canonicalize_identifier(self.parser.parse_identifier(false)?); if self.parser.parse_keyword(Keyword::SET) { @@ -85,18 +90,7 @@ impl<'a> ParserContext<'a> { .parser .parse_options(Keyword::WITH)? .into_iter() - .map(|option: SqlOption| -> Result<(String, String)> { - let (key, value) = (option.name, option.value); - let v = match value { - Expr::Value(sqlparser::ast::Value::SingleQuotedString(v)) - | Expr::Value(sqlparser::ast::Value::DoubleQuotedString(v)) => v, - Expr::Identifier(v) => v.value, - Expr::Value(sqlparser::ast::Value::Number(v, _)) => v.to_string(), - _ => return error::InvalidTableOptionValueSnafu { key, value }.fail(), - }; - let k = key.value.to_lowercase(); - Ok((k, v)) - }) + .map(parse_option_string) .collect::>>() .unwrap(); diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs index 7568fcb24f2a..7e2fcd36f161 100644 --- a/src/sql/src/statements.rs +++ b/src/sql/src/statements.rs @@ -463,7 +463,7 @@ pub fn column_to_schema( if let Some(options) = column.extensions.build_fulltext_options()? { column_schema = column_schema - .with_fulltext_options(options) + .with_fulltext_options(&options) .context(SetFulltextOptionSnafu)?; } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index fe00fcfcb027..872d4d2e985f 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -28,7 +28,9 @@ use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; use common_macro::stack_trace_debug; use datatypes::arrow::datatypes::FieldRef; -use datatypes::schema::{ColumnSchema, FulltextAnalyzer, FulltextOptions, Schema, SchemaRef}; +use datatypes::schema::{ + validate_fulltext_options, ColumnSchema, FulltextOptions, Schema, SchemaRef, +}; use serde::de::Error; use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; @@ -39,9 +41,6 @@ use crate::storage::{ColumnId, RegionId}; pub type Result = std::result::Result; -const COLUMN_FULLTEXT_OPT_KEY_ANALYZER: &str = "analyzer"; -const COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE: &str = "case_sensitive"; - /// Metadata of a column. #[derive(Clone, Serialize, Deserialize, PartialEq, Eq)] pub struct ColumnMetadata { @@ -527,9 +526,9 @@ impl RegionMetadataBuilder { AlterKind::DropColumns { names } => self.drop_columns(&names), AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns), AlterKind::ChangeColumnFulltext { - column_name: _, + column_name, options, - } => self.change_column_fulltext(options), + } => self.change_column_fulltext(column_name, options)?, } Ok(self) } @@ -631,7 +630,11 @@ impl RegionMetadataBuilder { } /// Changes column fulltext option. - fn change_column_fulltext(&mut self, options: HashMap) { + fn change_column_fulltext( + &mut self, + column_name: String, + options: HashMap, + ) -> Result<()> { for column_meta in self.column_metadatas.iter_mut() { let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() { f @@ -639,34 +642,17 @@ impl RegionMetadataBuilder { FulltextOptions::default() }; - if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { - match analyzer.to_ascii_lowercase().as_str() { - "english" => { - fulltext.enable = true; - fulltext.analyzer = FulltextAnalyzer::English; - } - "chinese" => { - fulltext.enable = true; - fulltext.analyzer = FulltextAnalyzer::Chinese; - } - _ => {} - } - } - if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { - match case_sensitive.to_ascii_lowercase().as_str() { - "true" => { - fulltext.enable = true; - fulltext.case_sensitive = true; - } - "false" => { - fulltext.enable = true; - fulltext.case_sensitive = false; - } - _ => {} - } + if !validate_fulltext_options(&mut fulltext, &options) { + return InvalidFulltextOptionsSnafu { column_name }.fail(); } - column_meta.column_schema.set_fulltext_options(fulltext).expect("set fulltext"); + + column_meta + .column_schema + .set_fulltext_options(&fulltext) + .expect("set fulltext"); } + + Ok(()) } } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 1e4aaf654006..0d9227be9d0f 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -490,7 +490,10 @@ impl AlterKind { return InvalidFulltextOptionsSnafu { column_name }.fail(); } } else { - return InvalidFulltextOptionsSnafu { column_name }.fail(); + return InvalidFulltextOptionsSnafu { + column_name, + } + .fail(); } } diff --git a/src/table/src/error.rs b/src/table/src/error.rs index 6e7debdcf912..8488eba9e8cc 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -138,7 +138,7 @@ pub enum Error { location: Location, }, - #[snafu(display("Invalid fulltext options"))] + #[snafu(display("Invalid fulltext options, column: {}", column_name))] InvalidFulltextOptions { #[snafu(implicit)] location: Location, diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 50aa8791c393..736e213f4d5b 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -1,5 +1,3 @@ -// Copyright 2023 Greptime Team -// // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. // You may obtain a copy of the License at @@ -21,7 +19,8 @@ use common_query::AddColumnLocation; use datafusion_expr::TableProviderFilterPushDown; pub use datatypes::error::{Error as ConvertError, Result as ConvertResult}; use datatypes::schema::{ - ColumnSchema, FulltextAnalyzer, FulltextOptions, RawSchema, Schema, SchemaBuilder, SchemaRef, + validate_fulltext_options, ColumnSchema, FulltextOptions, RawSchema, Schema, SchemaBuilder, + SchemaRef, }; use derive_builder::Builder; use serde::{Deserialize, Serialize}; @@ -34,9 +33,6 @@ use crate::requests::{AddColumnRequest, AlterKind, ChangeColumnTypeRequest, Tabl pub type TableId = u32; pub type TableVersion = u64; -const COLUMN_FULLTEXT_OPT_KEY_ANALYZER: &str = "analyzer"; -const COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE: &str = "case_sensitive"; - /// Indicates whether and how a filter expression can be handled by a /// Table for table scans. #[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq)] @@ -606,13 +602,12 @@ impl TableMeta { let table_schema = &self.schema; let mut meta_builder = self.new_meta_builder(); - let index = table_schema - .column_index_by_name(column_name) + let column = &table_schema + .column_schema_by_name(column_name) .with_context(|| error::ColumnNotExistsSnafu { column_name, table_name, })?; - let column = &table_schema.column_schemas()[index]; let mut fulltext = if let Ok(Some(f)) = column.fulltext_options() { f @@ -620,35 +615,8 @@ impl TableMeta { FulltextOptions::default() }; - if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { - match analyzer.to_ascii_lowercase().as_str() { - "english" => { - fulltext.enable = true; - fulltext.analyzer = FulltextAnalyzer::English; - } - "chinese" => { - fulltext.enable = true; - fulltext.analyzer = FulltextAnalyzer::Chinese; - } - _ => { - return error::InvalidFulltextOptionsSnafu { column_name }.fail()?; - } - } - } - if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { - match case_sensitive.to_ascii_lowercase().as_str() { - "true" => { - fulltext.enable = true; - fulltext.case_sensitive = true; - } - "false" => { - fulltext.enable = true; - fulltext.case_sensitive = false; - } - _ => { - return error::InvalidFulltextOptionsSnafu { column_name }.fail()?; - } - } + if !validate_fulltext_options(&mut fulltext, &options) { + return error::InvalidFulltextOptionsSnafu { column_name }.fail(); } let columns: Vec<_> = table_schema @@ -659,7 +627,7 @@ impl TableMeta { if column.name == *column_name { column = column .clone() - .with_fulltext_options(fulltext.clone()) + .with_fulltext_options(&fulltext) .unwrap(); } column diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result index b3cc77e95d8e..cfc8db45c66f 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -110,7 +110,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'yes'); -Error: 1004(InvalidArguments), Invalid fulltext options +Error: 1004(InvalidArguments), Invalid fulltext options, column: message SHOW CREATE TABLE test; @@ -131,7 +131,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinglish', case_sensitive = 'false'); -Error: 1004(InvalidArguments), Invalid fulltext options +Error: 1004(InvalidArguments), Invalid fulltext options, column: message SHOW CREATE TABLE test; @@ -156,41 +156,41 @@ Affected Rows: 0 SHOW CREATE TABLE test; -+-------+-----------------------------------------------------------------------------------------------+ -| Table | Create Table | -+-------+-----------------------------------------------------------------------------------------------+ -| test | CREATE TABLE IF NOT EXISTS "test" ( | -| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | -| | "time" TIMESTAMP(3) NOT NULL FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'), | -| | TIME INDEX ("time") | -| | ) | -| | | -| | ENGINE=mito | -| | WITH( | -| | append_mode = 'true' | -| | ) | -+-------+-----------------------------------------------------------------------------------------------+ ++-------+----------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+----------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+----------------------------------------------------------------------------------------+ ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'no'); -Error: 1004(InvalidArguments), Invalid fulltext options +Error: 1004(InvalidArguments), Invalid fulltext options, column: time SHOW CREATE TABLE test; -+-------+-----------------------------------------------------------------------------------------------+ -| Table | Create Table | -+-------+-----------------------------------------------------------------------------------------------+ -| test | CREATE TABLE IF NOT EXISTS "test" ( | -| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | -| | "time" TIMESTAMP(3) NOT NULL FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'), | -| | TIME INDEX ("time") | -| | ) | -| | | -| | ENGINE=mito | -| | WITH( | -| | append_mode = 'true' | -| | ) | -+-------+-----------------------------------------------------------------------------------------------+ ++-------+----------------------------------------------------------------------------------------+ +| Table | Create Table | ++-------+----------------------------------------------------------------------------------------+ +| test | CREATE TABLE IF NOT EXISTS "test" ( | +| | "message" STRING NULL FULLTEXT WITH(analyzer = 'English', case_sensitive = 'false'), | +| | "time" TIMESTAMP(3) NOT NULL, | +| | TIME INDEX ("time") | +| | ) | +| | | +| | ENGINE=mito | +| | WITH( | +| | append_mode = 'true' | +| | ) | ++-------+----------------------------------------------------------------------------------------+ DROP TABLE test; From 0356c660a2ae6e3de5e2653ef2c315c19acb6f1f Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 11 Sep 2024 21:31:07 +0800 Subject: [PATCH 10/20] typo --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/common/meta/src/ddl/alter_table/region_request.rs | 2 +- src/store-api/src/region_request.rs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 78c07a04ba5b..1e280d90bd3a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4252,7 +4252,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/irenjj/greptime-proto.git?rev=d942e6bc84beaf48a59b00ce9064eafb16539336#d942e6bc84beaf48a59b00ce9064eafb16539336" +source = "git+https://github.com/irenjj/greptime-proto.git?rev=6df0c7ba6d0c9acfcf155483d19889ffed3ef99d#6df0c7ba6d0c9acfcf155483d19889ffed3ef99d" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index 3d9ccec79266..bd25cbfa0887 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,7 +119,7 @@ etcd-client = { version = "0.13" } fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "d942e6bc84beaf48a59b00ce9064eafb16539336" } +greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "6df0c7ba6d0c9acfcf155483d19889ffed3ef99d" } humantime = "2.1" humantime-serde = "1.1" itertools = "0.10" 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 feba22523e4c..a8e9f24a1b06 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,7 @@ fn create_proto_alter_kind( }))) } Kind::RenameTable(_) => Ok(None), - Kind::ChangeFulltext(x) => Ok(Some(alter_request::Kind::ChangeFulltex(x.clone()))), + Kind::ChangeFulltext(x) => Ok(Some(alter_request::Kind::ChangeFulltext(x.clone()))), } } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 0d9227be9d0f..d57184cfd9c2 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -526,7 +526,7 @@ impl TryFrom for AlterKind { let names = x.drop_columns.into_iter().map(|x| x.name).collect(); AlterKind::DropColumns { names } } - alter_request::Kind::ChangeFulltex(x) => AlterKind::ChangeColumnFulltext { + alter_request::Kind::ChangeFulltext(x) => AlterKind::ChangeColumnFulltext { column_name: x.column_name, options: x.options, }, From 20a7b824ee6007384cc37b71a89caa7dd64f8a01 Mon Sep 17 00:00:00 2001 From: irenjj Date: Thu, 12 Sep 2024 00:43:29 +0800 Subject: [PATCH 11/20] add test --- src/datatypes/src/schema.rs | 12 +++++ src/datatypes/src/schema/column_schema.rs | 4 +- src/sql/src/parsers/alter_parser.rs | 34 +++++++++++++ src/sql/src/statements.rs | 2 +- src/store-api/src/metadata.rs | 2 +- src/table/src/metadata.rs | 51 ++++++++++++++++++- .../common/alter/change_fulltext_options.sql | 1 + 7 files changed, 101 insertions(+), 5 deletions(-) diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index 21bf921ca688..3617adb10df8 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -529,4 +529,16 @@ mod tests { .build() .is_err()); } + + #[test] + fn test_validate_fulltext_options() { + let mut fulltext = FulltextOptions::default(); + let options = HashMap::from([(String::from("analyzer"), String::from("English"))]); + assert!(validate_fulltext_options(&mut fulltext, &options)); + + let mut fulltext = FulltextOptions::default(); + let options = HashMap::from([(String::from("analyzer"), String::from("Chinglish"))]); + assert!(!validate_fulltext_options(&mut fulltext, &options)); + + } } diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index ea2ac20b64cf..80b8e8a1e757 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -255,12 +255,12 @@ impl ColumnSchema { } } - pub fn with_fulltext_options(mut self, options: &FulltextOptions) -> Result { + pub fn with_fulltext_options(mut self, options: FulltextOptions) -> Result { self.set_fulltext_options(options)?; Ok(self) } - pub fn set_fulltext_options(&mut self, options: &FulltextOptions) -> Result<()> { + pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> { if self.data_type == ConcreteDataType::string_datatype() { self.metadata.insert( FULLTEXT_KEY.to_string(), diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 524ad40cb428..f8427383ad88 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -430,4 +430,38 @@ mod tests { _ => unreachable!(), } } + + #[test] + fn test_parse_alter_change_fulltext() { + let sql = "ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true');"; + let mut result = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(1, result.len()); + + let statement = result.remove(0); + assert_matches!(statement, Statement::Alter { .. }); + match statement { + Statement::Alter(alter_table) => { + assert_eq!("test", alter_table.table_name().0[0].value); + + let alter_operation = alter_table.alter_operation(); + assert_matches!( + alter_operation, + AlterTableOperation::AlterColumnFulltext { .. } + ); + match alter_operation { + AlterTableOperation::AlterColumnFulltext { + column_name, + options, + } => { + assert_eq!(column_name.value, r#"message"#); + assert_eq!(options.get("analyzer").unwrap(), "Chinese"); + } + _ => unreachable!(), + } + } + _ => unreachable!(), + } + } } diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs index 7e2fcd36f161..7568fcb24f2a 100644 --- a/src/sql/src/statements.rs +++ b/src/sql/src/statements.rs @@ -463,7 +463,7 @@ pub fn column_to_schema( if let Some(options) = column.extensions.build_fulltext_options()? { column_schema = column_schema - .with_fulltext_options(&options) + .with_fulltext_options(options) .context(SetFulltextOptionSnafu)?; } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 872d4d2e985f..1df0fde8fd6b 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -648,7 +648,7 @@ impl RegionMetadataBuilder { column_meta .column_schema - .set_fulltext_options(&fulltext) + .set_fulltext_options(fulltext) .expect("set fulltext"); } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 736e213f4d5b..9aac3231b546 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -627,7 +627,7 @@ impl TableMeta { if column.name == *column_name { column = column .clone() - .with_fulltext_options(&fulltext) + .with_fulltext_options(fulltext.clone()) .unwrap(); } column @@ -948,6 +948,24 @@ mod tests { .unwrap() } + fn new_test_schema2() -> Schema { + let column_schemas = vec![ + ColumnSchema::new("col1", ConcreteDataType::string_datatype(), true), + ColumnSchema::new( + "ts", + ConcreteDataType::timestamp_millisecond_datatype(), + false, + ) + .with_time_index(true), + ColumnSchema::new("col2", ConcreteDataType::int32_datatype(), true), + ]; + SchemaBuilder::try_from(column_schemas) + .unwrap() + .version(123) + .build() + .unwrap() + } + #[test] fn test_raw_convert() { let schema = Arc::new(new_test_schema()); @@ -1366,4 +1384,35 @@ mod tests { assert_eq!(&[0, 1], &new_meta.primary_key_indices[..]); assert_eq!(&[2, 3, 4], &new_meta.value_indices[..]); } + + #[test] + fn test_change_column_fulltext() { + let schema = Arc::new(new_test_schema2()); + let meta = TableMetaBuilder::default() + .schema(schema) + .primary_key_indices(vec![0]) + .engine("engine") + .next_column_id(3) + .build() + .unwrap(); + + let alter_kind = AlterKind::ChangeFulltext { + column_name: "col1".to_string(), + options: HashMap::from([(String::from("analyzer"), String::from("English"))]), + }; + assert!(meta + .builder_with_alter_kind("my_table", &alter_kind, false) + .is_ok()); + + let alter_kind = AlterKind::ChangeFulltext { + column_name: "col1".to_string(), + options: HashMap::from([(String::from("case_sensitive"), String::from("no"))]), + }; + let err = meta + .builder_with_alter_kind("my_table", &alter_kind, false) + .err() + .unwrap(); + assert_eq!(StatusCode::InvalidArguments, err.status_code()); + + } } diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.sql b/tests/cases/standalone/common/alter/change_fulltext_options.sql index d29549cff614..cfc39447725f 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.sql +++ b/tests/cases/standalone/common/alter/change_fulltext_options.sql @@ -9,6 +9,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'true'); +-- SQLNESS ARG restart=true SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); From 8a61fdd7a2581c53a22f1c3633bbeef63b8b8ceb Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 18 Sep 2024 22:14:52 +0800 Subject: [PATCH 12/20] Update src/store-api/src/region_request.rs Co-authored-by: Weny Xu --- src/store-api/src/region_request.rs | 75 +++++++++++++++++------------ 1 file changed, 43 insertions(+), 32 deletions(-) diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index d57184cfd9c2..36df3bda6354 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -464,41 +464,52 @@ impl AlterKind { Ok(()) } - pub fn validate_column_fulltext_to_alter( - metadata: &RegionMetadata, - column_name: &str, - options: &HashMap, - ) -> Result<()> { - let _ = - metadata - .column_by_name(column_name) - .with_context(|| InvalidRegionRequestSnafu { - region_id: metadata.region_id, - err: format!("column {} not found", column_name), - })?; - - for (k, v) in options { - if k.to_ascii_lowercase().eq(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { - if v.to_ascii_lowercase().ne("english") && v.to_ascii_lowercase().ne("chinese") { - return InvalidFulltextOptionsSnafu { column_name }.fail(); - } - } else if k - .to_ascii_lowercase() - .eq(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) - { - if v.to_ascii_lowercase().ne("true") && v.to_ascii_lowercase().ne("false") { - return InvalidFulltextOptionsSnafu { column_name }.fail(); - } - } else { - return InvalidFulltextOptionsSnafu { - column_name, - } - .fail(); +/// Validates full-text options for altering a specific column in the region metadata. +/// +/// This function checks if the provided column exists in the region metadata and validates +/// the full-text options (`analyzer` and `case_sensitive`) for correctness. If any option is +/// invalid, it returns an error. +/// +/// # Parameters: +/// - `metadata`: The region metadata that contains column information. +/// - `column_name`: The name of the column to validate. +/// - `options`: A `HashMap` containing the full-text options to be validated. +/// +/// # Returns: +/// - `Ok(())` if the column exists and the options are valid. +/// - `Err` if the column is not found or if any of the options are invalid. +pub fn validate_column_fulltext_to_alter( + metadata: &RegionMetadata, + column_name: &str, + options: &HashMap, +) -> Result<()> { + // Ensure the column exists in the region metadata, otherwise return an error + metadata + .column_by_name(column_name) + .with_context(|| InvalidRegionRequestSnafu { + region_id: metadata.region_id, + err: format!("column {} not found", column_name), + })?; + + // Validate the full-text options in a simplified manner + for (k, v) in options { + match k.to_ascii_lowercase().as_str() { + // Validate the "analyzer" option + COLUMN_FULLTEXT_OPT_KEY_ANALYZER if !matches!(v.to_ascii_lowercase().as_str(), "english" | "chinese") => { + return InvalidFulltextOptionsSnafu { column_name }.fail(); + } + // Validate the "case_sensitive" option + COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE if !matches!(v.to_ascii_lowercase().as_str(), "true" | "false") => { + return InvalidFulltextOptionsSnafu { column_name }.fail(); } + // Return an error for any unknown options + _ => return InvalidFulltextOptionsSnafu { column_name }.fail(), } - - Ok(()) } + + Ok(()) +} + } impl TryFrom for AlterKind { From 6484664bc8c8fbf72b915f2f76f600ba416552e8 Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 18 Sep 2024 22:15:10 +0800 Subject: [PATCH 13/20] Update src/datatypes/src/schema.rs Co-authored-by: Weny Xu --- src/datatypes/src/schema.rs | 39 +++++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index 3617adb10df8..b71ff4ba790e 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -303,11 +303,28 @@ fn validate_timestamp_index(column_schemas: &[ColumnSchema], timestamp_index: us Ok(()) } -pub fn validate_fulltext_options( - fulltext: &mut FulltextOptions, +/// Parses the provided options to configure full-text search behavior. +/// +/// This function checks for specific keys in the provided `HashMap`: +/// - `COLUMN_FULLTEXT_OPT_KEY_ANALYZER`: Defines the analyzer to use (e.g., "english", "chinese"). +/// - `COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE`: Defines whether the full-text search should be case-sensitive ("true" or "false"). +/// +/// If the provided options contain valid values for the full-text keys, a configured `FulltextOptions` +/// object is returned. If the options are invalid or missing, the function returns `None`. +/// +/// # Parameters: +/// - `options`: A reference to a `HashMap` containing the full-text options. +/// +/// # Returns: +/// - `Some(FulltextOptions)` if valid options are provided. +/// - `None` if the options are invalid or do not contain full-text related keys. +pub fn parse_fulltext_options( options: &HashMap, -) -> bool { +) -> Option { + let mut fulltext = FulltextOptions::default(); let mut is_fulltext_key_exist = false; + + // Check and parse the "analyzer" option if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { match analyzer.to_ascii_lowercase().as_str() { "english" => { @@ -319,12 +336,14 @@ pub fn validate_fulltext_options( fulltext.analyzer = FulltextAnalyzer::Chinese; } _ => { - return false; + // If the analyzer is invalid, return None to indicate failure + return None; } } is_fulltext_key_exist = true; } + // Check and parse the "case_sensitive" option if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { match case_sensitive.to_ascii_lowercase().as_str() { "true" => { @@ -336,15 +355,23 @@ pub fn validate_fulltext_options( fulltext.case_sensitive = false; } _ => { - return false; + // If case sensitivity is invalid, return None + return None; } } is_fulltext_key_exist = true; } - is_fulltext_key_exist + // If any fulltext-related key exists, return the constructed FulltextOptions + if is_fulltext_key_exist { + Some(fulltext) + } else { + // Return None if no valid fulltext keys are found + None + } } + pub type SchemaRef = Arc; impl TryFrom> for Schema { From 820d75b8d97b258c0906a31efd177cb7055f73d4 Mon Sep 17 00:00:00 2001 From: irenjj Date: Thu, 19 Sep 2024 00:14:18 +0800 Subject: [PATCH 14/20] fix error --- src/common/grpc-expr/src/alter.rs | 2 +- src/datatypes/src/schema.rs | 22 ++--- src/operator/src/expr_factory.rs | 2 +- src/store-api/src/metadata.rs | 18 ++-- src/store-api/src/region_request.rs | 91 ++++++++++--------- src/table/src/metadata.rs | 7 +- .../alter/change_fulltext_options.result | 1 + 7 files changed, 69 insertions(+), 74 deletions(-) diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index fa559117ad92..62b9ebf72a9e 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -16,7 +16,7 @@ use api::helper::ColumnDataTypeWrapper; use api::v1::add_column_location::LocationType; use api::v1::alter_expr::Kind; use api::v1::{ - column_def, AddColumnLocation as Location, ChangeFulltext, AlterExpr, ChangeColumnTypes, + column_def, AddColumnLocation as Location, AlterExpr, ChangeColumnTypes, ChangeFulltext, CreateTableExpr, DropColumns, RenameTable, SemanticType, }; use common_query::AddColumnLocation; diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index b71ff4ba790e..1ab87239cb49 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -304,23 +304,21 @@ fn validate_timestamp_index(column_schemas: &[ColumnSchema], timestamp_index: us } /// Parses the provided options to configure full-text search behavior. -/// +/// /// This function checks for specific keys in the provided `HashMap`: /// - `COLUMN_FULLTEXT_OPT_KEY_ANALYZER`: Defines the analyzer to use (e.g., "english", "chinese"). /// - `COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE`: Defines whether the full-text search should be case-sensitive ("true" or "false"). -/// -/// If the provided options contain valid values for the full-text keys, a configured `FulltextOptions` +/// +/// If the provided options contain valid values for the full-text keys, a configured `FulltextOptions` /// object is returned. If the options are invalid or missing, the function returns `None`. -/// +/// /// # Parameters: /// - `options`: A reference to a `HashMap` containing the full-text options. /// /// # Returns: /// - `Some(FulltextOptions)` if valid options are provided. /// - `None` if the options are invalid or do not contain full-text related keys. -pub fn parse_fulltext_options( - options: &HashMap, -) -> Option { +pub fn parse_fulltext_options(options: &HashMap) -> Option { let mut fulltext = FulltextOptions::default(); let mut is_fulltext_key_exist = false; @@ -371,7 +369,6 @@ pub fn parse_fulltext_options( } } - pub type SchemaRef = Arc; impl TryFrom> for Schema { @@ -559,13 +556,10 @@ mod tests { #[test] fn test_validate_fulltext_options() { - let mut fulltext = FulltextOptions::default(); let options = HashMap::from([(String::from("analyzer"), String::from("English"))]); - assert!(validate_fulltext_options(&mut fulltext, &options)); + assert!(parse_fulltext_options(&options).is_some()); - let mut fulltext = FulltextOptions::default(); let options = HashMap::from([(String::from("analyzer"), String::from("Chinglish"))]); - assert!(!validate_fulltext_options(&mut fulltext, &options)); - - } + assert!(parse_fulltext_options(&options).is_some()); + } } diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 8d5c818de38b..6ea1c38b4c62 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -18,7 +18,7 @@ use api::helper::ColumnDataTypeWrapper; use api::v1::alter_expr::Kind; use api::v1::column_def::options_from_column_schema; use api::v1::{ - AddColumn, AddColumns, ChangeFulltext, AlterExpr, ChangeColumnType, ChangeColumnTypes, + AddColumn, AddColumns, AlterExpr, ChangeColumnType, ChangeColumnTypes, ChangeFulltext, ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, DropColumn, DropColumns, ExpireAfter, RenameTable, SemanticType, TableName, }; diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 1df0fde8fd6b..32f956b7d67c 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -28,9 +28,7 @@ use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; use common_macro::stack_trace_debug; use datatypes::arrow::datatypes::FieldRef; -use datatypes::schema::{ - validate_fulltext_options, ColumnSchema, FulltextOptions, Schema, SchemaRef, -}; +use datatypes::schema::{parse_fulltext_options, ColumnSchema, FulltextOptions, Schema, SchemaRef}; use serde::de::Error; use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; @@ -526,9 +524,9 @@ impl RegionMetadataBuilder { AlterKind::DropColumns { names } => self.drop_columns(&names), AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns), AlterKind::ChangeColumnFulltext { - column_name, + column_name: _, options, - } => self.change_column_fulltext(column_name, options)?, + } => self.change_column_fulltext(options)?, } Ok(self) } @@ -630,11 +628,7 @@ impl RegionMetadataBuilder { } /// Changes column fulltext option. - fn change_column_fulltext( - &mut self, - column_name: String, - options: HashMap, - ) -> Result<()> { + fn change_column_fulltext(&mut self, options: HashMap) -> Result<()> { for column_meta in self.column_metadatas.iter_mut() { let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() { f @@ -642,8 +636,8 @@ impl RegionMetadataBuilder { FulltextOptions::default() }; - if !validate_fulltext_options(&mut fulltext, &options) { - return InvalidFulltextOptionsSnafu { column_name }.fail(); + if let Some(f) = parse_fulltext_options(&options) { + fulltext = f; } column_meta diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 36df3bda6354..d6bef66acd47 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -464,52 +464,59 @@ impl AlterKind { Ok(()) } -/// Validates full-text options for altering a specific column in the region metadata. -/// -/// This function checks if the provided column exists in the region metadata and validates -/// the full-text options (`analyzer` and `case_sensitive`) for correctness. If any option is -/// invalid, it returns an error. -/// -/// # Parameters: -/// - `metadata`: The region metadata that contains column information. -/// - `column_name`: The name of the column to validate. -/// - `options`: A `HashMap` containing the full-text options to be validated. -/// -/// # Returns: -/// - `Ok(())` if the column exists and the options are valid. -/// - `Err` if the column is not found or if any of the options are invalid. -pub fn validate_column_fulltext_to_alter( - metadata: &RegionMetadata, - column_name: &str, - options: &HashMap, -) -> Result<()> { - // Ensure the column exists in the region metadata, otherwise return an error - metadata - .column_by_name(column_name) - .with_context(|| InvalidRegionRequestSnafu { - region_id: metadata.region_id, - err: format!("column {} not found", column_name), - })?; + /// Validates full-text options for altering a specific column in the region metadata. + /// + /// This function checks if the provided column exists in the region metadata and validates + /// the full-text options (`analyzer` and `case_sensitive`) for correctness. If any option is + /// invalid, it returns an error. + /// + /// # Parameters: + /// - `metadata`: The region metadata that contains column information. + /// - `column_name`: The name of the column to validate. + /// - `options`: A `HashMap` containing the full-text options to be validated. + /// + /// # Returns: + /// - `Ok(())` if the column exists and the options are valid. + /// - `Err` if the column is not found or if any of the options are invalid. + pub fn validate_column_fulltext_to_alter( + metadata: &RegionMetadata, + column_name: &str, + options: &HashMap, + ) -> Result<()> { + // Ensure the column exists in the region metadata, otherwise return an error + metadata + .column_by_name(column_name) + .with_context(|| InvalidRegionRequestSnafu { + region_id: metadata.region_id, + err: format!("column {} not found", column_name), + })?; - // Validate the full-text options in a simplified manner - for (k, v) in options { - match k.to_ascii_lowercase().as_str() { - // Validate the "analyzer" option - COLUMN_FULLTEXT_OPT_KEY_ANALYZER if !matches!(v.to_ascii_lowercase().as_str(), "english" | "chinese") => { - return InvalidFulltextOptionsSnafu { column_name }.fail(); - } - // Validate the "case_sensitive" option - COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE if !matches!(v.to_ascii_lowercase().as_str(), "true" | "false") => { - return InvalidFulltextOptionsSnafu { column_name }.fail(); + // Validate the full-text options in a simplified manner + for (k, v) in options { + match k.to_ascii_lowercase().as_str() { + // Validate the "analyzer" option + COLUMN_FULLTEXT_OPT_KEY_ANALYZER => { + if !matches!(v.to_ascii_lowercase().as_str(), "english") + && !matches!(v.to_ascii_lowercase().as_str(), "chinese") + { + return InvalidFulltextOptionsSnafu { column_name }.fail(); + } + } + // Validate the "case_sensitive" option + COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE => { + if !matches!(v.to_ascii_lowercase().as_str(), "true") + && !matches!(v.to_ascii_lowercase().as_str(), "false") + { + return InvalidFulltextOptionsSnafu { column_name }.fail(); + } + } + // Return an error for any unknown options + _ => return InvalidFulltextOptionsSnafu { column_name }.fail(), } - // Return an error for any unknown options - _ => return InvalidFulltextOptionsSnafu { column_name }.fail(), } - } - - Ok(()) -} + Ok(()) + } } impl TryFrom for AlterKind { diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 9aac3231b546..af07a9f92b87 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -19,7 +19,7 @@ use common_query::AddColumnLocation; use datafusion_expr::TableProviderFilterPushDown; pub use datatypes::error::{Error as ConvertError, Result as ConvertResult}; use datatypes::schema::{ - validate_fulltext_options, ColumnSchema, FulltextOptions, RawSchema, Schema, SchemaBuilder, + parse_fulltext_options, ColumnSchema, FulltextOptions, RawSchema, Schema, SchemaBuilder, SchemaRef, }; use derive_builder::Builder; @@ -615,8 +615,8 @@ impl TableMeta { FulltextOptions::default() }; - if !validate_fulltext_options(&mut fulltext, &options) { - return error::InvalidFulltextOptionsSnafu { column_name }.fail(); + if let Some(f) = parse_fulltext_options(&options) { + fulltext = f; } let columns: Vec<_> = table_schema @@ -1413,6 +1413,5 @@ mod tests { .err() .unwrap(); assert_eq!(StatusCode::InvalidArguments, err.status_code()); - } } diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result index cfc8db45c66f..58f7b9bb1177 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -28,6 +28,7 @@ ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinese', c Affected Rows: 0 +-- SQLNESS ARG restart=true SHOW CREATE TABLE test; +-------+---------------------------------------------------------------------------------------+ From 5331ed41102010e18834072025fa5424518269e8 Mon Sep 17 00:00:00 2001 From: irenjj Date: Thu, 19 Sep 2024 20:16:25 +0800 Subject: [PATCH 15/20] Update src/store-api/src/region_request.rs Co-authored-by: Lei, HUANG <6406592+v0y4g3r@users.noreply.github.com> --- src/store-api/src/region_request.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index d6bef66acd47..7e87f2f5f17a 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -493,19 +493,20 @@ impl AlterKind { // Validate the full-text options in a simplified manner for (k, v) in options { + let value_lower = v.to_ascii_lowercase(); match k.to_ascii_lowercase().as_str() { // Validate the "analyzer" option COLUMN_FULLTEXT_OPT_KEY_ANALYZER => { - if !matches!(v.to_ascii_lowercase().as_str(), "english") - && !matches!(v.to_ascii_lowercase().as_str(), "chinese") + if !matches!(value_lower.as_str(), "english") + && !matches!(value_lower.as_str(), "chinese") { return InvalidFulltextOptionsSnafu { column_name }.fail(); } } // Validate the "case_sensitive" option COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE => { - if !matches!(v.to_ascii_lowercase().as_str(), "true") - && !matches!(v.to_ascii_lowercase().as_str(), "false") + if !matches!(value_lower.as_str(), "true") + && !matches!(value_lower.as_str(), "false") { return InvalidFulltextOptionsSnafu { column_name }.fail(); } From 6ae407c596be7005d0e6c489b5002b49323b02b2 Mon Sep 17 00:00:00 2001 From: irenjj Date: Thu, 19 Sep 2024 23:57:12 +0800 Subject: [PATCH 16/20] fix --- src/api/src/v1/column_def.rs | 2 +- src/datatypes/src/error.rs | 10 +++- src/datatypes/src/schema.rs | 5 +- src/datatypes/src/schema/column_schema.rs | 20 +++++--- src/mito2/src/sst/index.rs | 2 +- .../src/sst/index/fulltext_index/creator.rs | 6 +-- src/query/src/sql/show_create_table.rs | 2 +- src/sql/src/statements.rs | 2 +- src/store-api/src/metadata.rs | 48 ++++++++++------- src/store-api/src/region_request.rs | 29 ++++++++--- src/table/src/error.rs | 12 ++--- src/table/src/metadata.rs | 51 +++++++++++-------- .../alter/change_fulltext_options.result | 8 +-- 13 files changed, 125 insertions(+), 72 deletions(-) diff --git a/src/api/src/v1/column_def.rs b/src/api/src/v1/column_def.rs index b4d3425215c8..9b05ad3c2f9c 100644 --- a/src/api/src/v1/column_def.rs +++ b/src/api/src/v1/column_def.rs @@ -148,7 +148,7 @@ mod tests { assert!(options.is_none()); let schema = ColumnSchema::new("test", ConcreteDataType::string_datatype(), true) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::English, case_sensitive: false, diff --git a/src/datatypes/src/error.rs b/src/datatypes/src/error.rs index 5a255dc0a644..4efc1a647185 100644 --- a/src/datatypes/src/error.rs +++ b/src/datatypes/src/error.rs @@ -205,6 +205,13 @@ pub enum Error { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Invalid data type {} for fulltext", data_type))] + InvalidFulltextDataType { + data_type: String, + #[snafu(implicit)] + location: Location, + }, } impl ErrorExt for Error { @@ -222,7 +229,8 @@ impl ErrorExt for Error { | DefaultValueType { .. } | DuplicateMeta { .. } | InvalidTimestampPrecision { .. } - | InvalidPrecisionOrScale { .. } => StatusCode::InvalidArguments, + | InvalidPrecisionOrScale { .. } + | InvalidFulltextDataType { .. } => StatusCode::InvalidArguments, ValueExceedsPrecision { .. } | CastType { .. } diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index 1ab87239cb49..fd4ccf7caab3 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -320,7 +320,6 @@ fn validate_timestamp_index(column_schemas: &[ColumnSchema], timestamp_index: us /// - `None` if the options are invalid or do not contain full-text related keys. pub fn parse_fulltext_options(options: &HashMap) -> Option { let mut fulltext = FulltextOptions::default(); - let mut is_fulltext_key_exist = false; // Check and parse the "analyzer" option if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { @@ -338,7 +337,6 @@ pub fn parse_fulltext_options(options: &HashMap) -> Option) -> Option Result { + pub fn with_fulltext_options(mut self, options: &FulltextOptions) -> Result { self.set_fulltext_options(options)?; Ok(self) } - pub fn set_fulltext_options(&mut self, options: FulltextOptions) -> Result<()> { - if self.data_type == ConcreteDataType::string_datatype() { - self.metadata.insert( - FULLTEXT_KEY.to_string(), - serde_json::to_string(&options).context(error::SerializeSnafu)?, - ); + pub fn set_fulltext_options(&mut self, options: &FulltextOptions) -> Result<()> { + if self.data_type != ConcreteDataType::string_datatype() { + return error::InvalidFulltextDataTypeSnafu { + data_type: self.data_type.to_string(), + } + .fail(); } + + self.metadata.insert( + FULLTEXT_KEY.to_string(), + serde_json::to_string(options).context(error::SerializeSnafu)?, + ); + Ok(()) } } diff --git a/src/mito2/src/sst/index.rs b/src/mito2/src/sst/index.rs index b73bd0df7ee6..f9a6913c5f65 100644 --- a/src/mito2/src/sst/index.rs +++ b/src/mito2/src/sst/index.rs @@ -349,7 +349,7 @@ mod tests { if with_fulltext { let column_schema = ColumnSchema::new("text", ConcreteDataType::string_datatype(), true) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, ..Default::default() }) diff --git a/src/mito2/src/sst/index/fulltext_index/creator.rs b/src/mito2/src/sst/index/fulltext_index/creator.rs index 416e39d9dd5e..7bcb89fcc3b7 100644 --- a/src/mito2/src/sst/index/fulltext_index/creator.rs +++ b/src/mito2/src/sst/index/fulltext_index/creator.rs @@ -329,7 +329,7 @@ mod tests { ConcreteDataType::string_datatype(), true, ) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::English, case_sensitive: true, @@ -344,7 +344,7 @@ mod tests { ConcreteDataType::string_datatype(), true, ) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::English, case_sensitive: false, @@ -359,7 +359,7 @@ mod tests { ConcreteDataType::string_datatype(), true, ) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, analyzer: FulltextAnalyzer::Chinese, case_sensitive: false, diff --git a/src/query/src/sql/show_create_table.rs b/src/query/src/sql/show_create_table.rs index 2c560bd36013..35b61503808d 100644 --- a/src/query/src/sql/show_create_table.rs +++ b/src/query/src/sql/show_create_table.rs @@ -222,7 +222,7 @@ mod tests { ColumnSchema::new("cpu", ConcreteDataType::float64_datatype(), true), ColumnSchema::new("disk", ConcreteDataType::float32_datatype(), true), ColumnSchema::new("msg", ConcreteDataType::string_datatype(), true) - .with_fulltext_options(FulltextOptions { + .with_fulltext_options(&FulltextOptions { enable: true, ..Default::default() }) diff --git a/src/sql/src/statements.rs b/src/sql/src/statements.rs index 7568fcb24f2a..7e2fcd36f161 100644 --- a/src/sql/src/statements.rs +++ b/src/sql/src/statements.rs @@ -463,7 +463,7 @@ pub fn column_to_schema( if let Some(options) = column.extensions.build_fulltext_options()? { column_schema = column_schema - .with_fulltext_options(options) + .with_fulltext_options(&options) .context(SetFulltextOptionSnafu)?; } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 32f956b7d67c..3907807daa49 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -524,9 +524,9 @@ impl RegionMetadataBuilder { AlterKind::DropColumns { names } => self.drop_columns(&names), AlterKind::ChangeColumnTypes { columns } => self.change_column_types(columns), AlterKind::ChangeColumnFulltext { - column_name: _, + column_name, options, - } => self.change_column_fulltext(options)?, + } => self.change_column_fulltext(column_name, options)?, } Ok(self) } @@ -628,22 +628,29 @@ impl RegionMetadataBuilder { } /// Changes column fulltext option. - fn change_column_fulltext(&mut self, options: HashMap) -> Result<()> { + fn change_column_fulltext( + &mut self, + column_name: String, + options: HashMap, + ) -> Result<()> { for column_meta in self.column_metadatas.iter_mut() { - let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() { - f - } else { - FulltextOptions::default() - }; - - if let Some(f) = parse_fulltext_options(&options) { - fulltext = f; - } + if column_name.eq(&column_meta.column_schema.name) { + let mut fulltext = if let Ok(Some(f)) = column_meta.column_schema.fulltext_options() + { + f + } else { + FulltextOptions::default() + }; - column_meta - .column_schema - .set_fulltext_options(fulltext) - .expect("set fulltext"); + if let Some(f) = parse_fulltext_options(&options) { + fulltext = f; + } + + column_meta + .column_schema + .set_fulltext_options(&fulltext) + .expect("set fulltext"); + } } Ok(()) @@ -765,11 +772,18 @@ pub enum MetadataError { location: Location, }, - #[snafu(display("Invalid fulltext options, column: {}", column_name))] + #[snafu(display( + "Invalid fulltext options, column: {}, key: {}, value: {}", + column_name, + key, + value + ))] InvalidFulltextOptions { #[snafu(implicit)] location: Location, column_name: String, + key: String, + value: String, }, } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 7e87f2f5f17a..69d90bfbef14 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -492,15 +492,20 @@ impl AlterKind { })?; // Validate the full-text options in a simplified manner - for (k, v) in options { - let value_lower = v.to_ascii_lowercase(); - match k.to_ascii_lowercase().as_str() { + for (key, value) in options { + let value_lower = value.to_ascii_lowercase(); + match key.to_ascii_lowercase().as_str() { // Validate the "analyzer" option COLUMN_FULLTEXT_OPT_KEY_ANALYZER => { if !matches!(value_lower.as_str(), "english") && !matches!(value_lower.as_str(), "chinese") { - return InvalidFulltextOptionsSnafu { column_name }.fail(); + return InvalidFulltextOptionsSnafu { + column_name, + key, + value, + } + .fail(); } } // Validate the "case_sensitive" option @@ -508,11 +513,23 @@ impl AlterKind { if !matches!(value_lower.as_str(), "true") && !matches!(value_lower.as_str(), "false") { - return InvalidFulltextOptionsSnafu { column_name }.fail(); + return InvalidFulltextOptionsSnafu { + column_name, + key, + value, + } + .fail(); } } // Return an error for any unknown options - _ => return InvalidFulltextOptionsSnafu { column_name }.fail(), + _ => { + return InvalidFulltextOptionsSnafu { + column_name, + key, + value, + } + .fail() + } } } diff --git a/src/table/src/error.rs b/src/table/src/error.rs index 8488eba9e8cc..11324b5dcb88 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -138,11 +138,12 @@ pub enum Error { location: Location, }, - #[snafu(display("Invalid fulltext options, column: {}", column_name))] - InvalidFulltextOptions { + #[snafu(display("column {} has invalid colum type to set fulltext", column_name))] + InvalidFulltextColumnType { + column_name: String, + source: datatypes::error::Error, #[snafu(implicit)] location: Location, - column_name: String, }, } @@ -160,11 +161,10 @@ impl ErrorExt for Error { Error::ColumnExists { .. } => StatusCode::TableColumnExists, Error::SchemaBuild { source, .. } => source.status_code(), Error::TableOperation { source } => source.status_code(), + Error::InvalidFulltextColumnType { source, .. } => source.status_code(), Error::ColumnNotExists { .. } => StatusCode::TableColumnNotFound, Error::Unsupported { .. } => StatusCode::Unsupported, - Error::ParseTableOption { .. } | Error::InvalidFulltextOptions { .. } => { - StatusCode::InvalidArguments - } + Error::ParseTableOption { .. } => StatusCode::InvalidArguments, Error::MissingTimeIndexColumn { .. } => StatusCode::IllegalState, } } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index af07a9f92b87..4f59f6947205 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -619,7 +619,7 @@ impl TableMeta { fulltext = f; } - let columns: Vec<_> = table_schema + let columns: Result> = table_schema .column_schemas() .iter() .cloned() @@ -627,31 +627,42 @@ impl TableMeta { if column.name == *column_name { column = column .clone() - .with_fulltext_options(fulltext.clone()) - .unwrap(); + .with_fulltext_options(&fulltext) + .context(error::InvalidFulltextColumnTypeSnafu { column_name })?; } - column + Ok(column) }) .collect(); - 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 datatype with columns {column_name}"), - })?; + match columns { + Ok(columns) => { + 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 datatype with columns {column_name}" + ), + })?; - let _ = meta_builder - .schema(Arc::new(new_schema)) - .primary_key_indices(self.primary_key_indices.clone()); + let _ = meta_builder + .schema(Arc::new(new_schema)) + .primary_key_indices(self.primary_key_indices.clone()); - Ok(meta_builder) + Ok(meta_builder) + } + Err(e) => { + return Err(e); + } + } } /// Split requests into different groups using column location info. diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result index 58f7b9bb1177..688e9b8af27f 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -111,7 +111,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'yes'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: message +Error: 1004(InvalidArguments), Invalid fulltext options, column: message, key: case_sensitive, value: yes SHOW CREATE TABLE test; @@ -132,7 +132,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinglish', case_sensitive = 'false'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: message +Error: 1004(InvalidArguments), Invalid fulltext options, column: message, key: analyzer, value: Chinglish SHOW CREATE TABLE test; @@ -153,7 +153,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); -Affected Rows: 0 +Error: 1004(InvalidArguments), Invalid data type TimestampMillisecond for fulltext SHOW CREATE TABLE test; @@ -174,7 +174,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'no'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: time +Error: 1004(InvalidArguments), Invalid data type TimestampMillisecond for fulltext SHOW CREATE TABLE test; From aaf7b4b824f0ffe8a03d02106317a061eb3ea758 Mon Sep 17 00:00:00 2001 From: irenjj Date: Mon, 14 Oct 2024 21:15:44 +0800 Subject: [PATCH 17/20] wip: replace with struct --- Cargo.lock | 4 +- Cargo.toml | 4 +- src/common/grpc-expr/src/alter.rs | 12 ++- src/common/grpc-expr/src/error.rs | 11 ++- src/datatypes/Cargo.toml | 2 + src/datatypes/src/error.rs | 11 ++- src/datatypes/src/schema.rs | 67 +------------ src/datatypes/src/schema/column_schema.rs | 95 +++++++++++++++++++ src/operator/src/expr_factory.rs | 4 +- src/sql/src/parsers/alter_parser.rs | 12 ++- src/sql/src/statements/alter.rs | 13 +-- src/store-api/src/metadata.rs | 25 +++-- src/store-api/src/region_request.rs | 81 +++------------- src/table/src/error.rs | 12 ++- src/table/src/metadata.rs | 20 ++-- src/table/src/requests.rs | 4 +- .../alter/change_fulltext_options.result | 6 +- 17 files changed, 202 insertions(+), 181 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 1e280d90bd3a..6fb1bd9a0502 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3159,6 +3159,8 @@ dependencies = [ "serde", "serde_json", "snafu 0.8.4", + "sqlparser 0.45.0 (git+https://github.com/GreptimeTeam/sqlparser-rs.git?rev=54a267ac89c09b11c0c88934690530807185d3e7)", + "sqlparser_derive 0.1.1", ] [[package]] @@ -4252,7 +4254,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/irenjj/greptime-proto.git?rev=6df0c7ba6d0c9acfcf155483d19889ffed3ef99d#6df0c7ba6d0c9acfcf155483d19889ffed3ef99d" +source = "git+https://github.com/irenjj/greptime-proto.git?rev=2a8c2dfb7447b7fe5f0b8f3df868f041600caec2#2a8c2dfb7447b7fe5f0b8f3df868f041600caec2" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index bd25cbfa0887..4af1d18beba8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,7 +119,7 @@ etcd-client = { version = "0.13" } fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "6df0c7ba6d0c9acfcf155483d19889ffed3ef99d" } +greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "2a8c2dfb7447b7fe5f0b8f3df868f041600caec2" } humantime = "2.1" humantime-serde = "1.1" itertools = "0.10" @@ -151,7 +151,7 @@ reqwest = { version = "0.12", default-features = false, features = [ "stream", "multipart", ] } -# SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 +# SCRAM-SHA-512 requires https://github.com/dequbed/rsasl/pull/48, https://github.com/influxdata/rskafka/pull/247 rskafka = { git = "https://github.com/WenyXu/rskafka.git", rev = "940c6030012c5b746fad819fb72e3325b26e39de", features = [ "transport-tls", ] } diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index 62b9ebf72a9e..ba5dd8718b8e 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -20,7 +20,7 @@ use api::v1::{ CreateTableExpr, DropColumns, RenameTable, SemanticType, }; use common_query::AddColumnLocation; -use datatypes::schema::{ColumnSchema, RawSchema}; +use datatypes::schema::{ChangeFulltextOptions, ColumnSchema, RawSchema}; use snafu::{ensure, OptionExt, ResultExt}; use table::metadata::TableId; use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest, ChangeColumnTypeRequest}; @@ -94,10 +94,16 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result AlterKind::ChangeFulltext { column_name, - options, + options: ChangeFulltextOptions { + enable, + analyzer, + case_sensitive, + }, }, }; diff --git a/src/common/grpc-expr/src/error.rs b/src/common/grpc-expr/src/error.rs index 2f27c08bbe41..184ce8931ec5 100644 --- a/src/common/grpc-expr/src/error.rs +++ b/src/common/grpc-expr/src/error.rs @@ -121,6 +121,11 @@ pub enum Error { InvalidFulltextColumnType { column_name: String, column_type: ColumnDataType, + }, + + #[snafu(display("Invalid fulltext options"))] + InvalidFulltextOptions { + source: datatypes::error::Error, #[snafu(implicit)] location: Location, }, @@ -145,9 +150,9 @@ impl ErrorExt for Error { StatusCode::InvalidArguments } - Error::UnknownColumnDataType { .. } | Error::InvalidFulltextColumnType { .. } => { - StatusCode::InvalidArguments - } + Error::UnknownColumnDataType { .. } + | Error::InvalidFulltextOptions { .. } + | Error::InvalidFulltextColumnType { .. } => StatusCode::InvalidArguments, } } diff --git a/src/datatypes/Cargo.toml b/src/datatypes/Cargo.toml index 281057ce80c9..7449dc839938 100644 --- a/src/datatypes/Cargo.toml +++ b/src/datatypes/Cargo.toml @@ -30,3 +30,5 @@ paste = "1.0" serde.workspace = true serde_json.workspace = true snafu.workspace = true +sqlparser.workspace = true +sqlparser_derive = "0.1" diff --git a/src/datatypes/src/error.rs b/src/datatypes/src/error.rs index 4efc1a647185..8f68125a5371 100644 --- a/src/datatypes/src/error.rs +++ b/src/datatypes/src/error.rs @@ -212,6 +212,14 @@ pub enum Error { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Invalid Fulltext Options, key: {}, value: {}", key, value))] + InvalidFulltextOptions { + key: String, + value: String, + #[snafu(implicit)] + location: Location, + }, } impl ErrorExt for Error { @@ -230,7 +238,8 @@ impl ErrorExt for Error { | DuplicateMeta { .. } | InvalidTimestampPrecision { .. } | InvalidPrecisionOrScale { .. } - | InvalidFulltextDataType { .. } => StatusCode::InvalidArguments, + | InvalidFulltextDataType { .. } + | InvalidFulltextOptions { .. } => StatusCode::InvalidArguments, ValueExceedsPrecision { .. } | CastType { .. } diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index fd4ccf7caab3..9cfce28a36a7 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -26,8 +26,8 @@ use snafu::{ensure, ResultExt}; use crate::error::{self, DuplicateColumnSnafu, Error, ProjectArrowSchemaSnafu, Result}; pub use crate::schema::column_schema::{ - ColumnSchema, FulltextAnalyzer, FulltextOptions, Metadata, COMMENT_KEY, FULLTEXT_KEY, - TIME_INDEX_KEY, + ChangeFulltextOptions, ColumnSchema, FulltextAnalyzer, FulltextOptions, Metadata, COMMENT_KEY, + FULLTEXT_KEY, TIME_INDEX_KEY, }; pub use crate::schema::constraint::ColumnDefaultConstraint; pub use crate::schema::raw::RawSchema; @@ -303,69 +303,6 @@ fn validate_timestamp_index(column_schemas: &[ColumnSchema], timestamp_index: us Ok(()) } -/// Parses the provided options to configure full-text search behavior. -/// -/// This function checks for specific keys in the provided `HashMap`: -/// - `COLUMN_FULLTEXT_OPT_KEY_ANALYZER`: Defines the analyzer to use (e.g., "english", "chinese"). -/// - `COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE`: Defines whether the full-text search should be case-sensitive ("true" or "false"). -/// -/// If the provided options contain valid values for the full-text keys, a configured `FulltextOptions` -/// object is returned. If the options are invalid or missing, the function returns `None`. -/// -/// # Parameters: -/// - `options`: A reference to a `HashMap` containing the full-text options. -/// -/// # Returns: -/// - `Some(FulltextOptions)` if valid options are provided. -/// - `None` if the options are invalid or do not contain full-text related keys. -pub fn parse_fulltext_options(options: &HashMap) -> Option { - let mut fulltext = FulltextOptions::default(); - - // Check and parse the "analyzer" option - if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { - match analyzer.to_ascii_lowercase().as_str() { - "english" => { - fulltext.enable = true; - fulltext.analyzer = FulltextAnalyzer::English; - } - "chinese" => { - fulltext.enable = true; - fulltext.analyzer = FulltextAnalyzer::Chinese; - } - _ => { - // If the analyzer is invalid, return None to indicate failure - return None; - } - } - } - - // Check and parse the "case_sensitive" option - if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { - match case_sensitive.to_ascii_lowercase().as_str() { - "true" => { - fulltext.enable = true; - fulltext.case_sensitive = true; - } - "false" => { - fulltext.enable = true; - fulltext.case_sensitive = false; - } - _ => { - // If case sensitivity is invalid, return None - return None; - } - } - } - - // If any fulltext-related key exists, return the constructed FulltextOptions - if fulltext.enable { - Some(fulltext) - } else { - // Return None if no valid fulltext keys are found - None - } -} - pub type SchemaRef = Arc; impl TryFrom> for Schema { diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index 80c5ecc9ff77..8a3b2824d969 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -18,7 +18,9 @@ use std::fmt; use arrow::datatypes::Field; use serde::{Deserialize, Serialize}; use snafu::{ensure, ResultExt}; +use sqlparser_derive::{Visit, VisitMut}; +use super::{COLUMN_FULLTEXT_OPT_KEY_ANALYZER, COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE}; use crate::data_type::{ConcreteDataType, DataType}; use crate::error::{self, Error, Result}; use crate::schema::constraint::ColumnDefaultConstraint; @@ -353,6 +355,99 @@ pub enum FulltextAnalyzer { Chinese, } +impl TryFrom for FulltextAnalyzer { + type Error = Error; + + fn try_from(value: i32) -> Result { + match value { + 0 => Ok(FulltextAnalyzer::English), + 1 => Ok(FulltextAnalyzer::Chinese), + _ => Err(error::InvalidFulltextOptionsSnafu { + key: "analyzer".to_string(), + value: value.to_string(), + } + .build()), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Visit, VisitMut, Default)] +pub struct ChangeFulltextOptions { + pub enable: Option, + pub analyzer: Option, + pub case_sensitive: Option, +} + +impl fmt::Display for ChangeFulltextOptions { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "fulltext options: ")?; + + Ok(()) + } +} + +/// Parses the provided options to configure full-text search behavior. +/// +/// This function checks for specific keys in the provided `HashMap`: +/// - `COLUMN_FULLTEXT_OPT_KEY_ANALYZER`: Defines the analyzer to use (e.g., "english", "chinese"). +/// - `COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE`: Defines whether the full-text search should be case-sensitive ("true" or "false"). +/// +/// If the provided options contain valid values for the full-text keys, a configured `ChangeFulltextOptions` +/// object is returned. If the options are invalid or missing, the function returns error. +impl TryFrom> for ChangeFulltextOptions { + type Error = Error; + + fn try_from(options: HashMap) -> Result { + let mut fulltext = ChangeFulltextOptions::default(); + + // Check and parse the "analyzer" option + if let Some(analyzer) = options.get(COLUMN_FULLTEXT_OPT_KEY_ANALYZER) { + match analyzer.to_ascii_lowercase().as_str() { + "english" => { + fulltext.enable = Some(true); + fulltext.analyzer = Some(0); + } + "chinese" => { + fulltext.enable = Some(true); + fulltext.analyzer = Some(1); + } + _ => { + // If the analyzer is invalid, return None to indicate failure + return error::InvalidFulltextOptionsSnafu { + key: COLUMN_FULLTEXT_OPT_KEY_ANALYZER.to_string(), + value: analyzer.to_string(), + } + .fail(); + } + } + } + + // Check and parse the "case_sensitive" option + if let Some(case_sensitive) = options.get(COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE) { + match case_sensitive.to_ascii_lowercase().as_str() { + "true" => { + fulltext.enable = Some(true); + fulltext.case_sensitive = Some(true); + } + "false" => { + fulltext.enable = Some(true); + fulltext.case_sensitive = Some(false); + } + _ => { + // If case sensitivity is invalid, return None + return error::InvalidFulltextOptionsSnafu { + key: COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE.to_string(), + value: case_sensitive.to_string(), + } + .fail(); + } + } + } + + Ok(fulltext) + } +} + impl fmt::Display for FulltextAnalyzer { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 6ea1c38b4c62..61da09d6bff6 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -488,7 +488,9 @@ pub(crate) fn to_alter_expr( options, } => Kind::ChangeFulltext(ChangeFulltext { column_name: column_name.value.to_string(), - options: options.clone().into_map(), + enable: options.enable, + analyzer: options.analyzer, + case_sensitive: options.case_sensitive, }), }; diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index f8427383ad88..8f9f5815acb2 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -13,6 +13,7 @@ use std::collections::HashMap; use common_query::AddColumnLocation; +use datatypes::schema::ChangeFulltextOptions; use snafu::ResultExt; use sqlparser::keywords::Keyword; use sqlparser::parser::ParserError; @@ -94,9 +95,18 @@ impl<'a> ParserContext<'a> { .collect::>>() .unwrap(); + let fulltext_options: ChangeFulltextOptions = match options.try_into() { + Ok(options) => options, + Err(_) => { + return Err(ParserError::ParserError( + "failed to parse fulltext options".to_string(), + )) + } + }; + AlterTableOperation::AlterColumnFulltext { column_name, - options: options.into(), + options: fulltext_options, } } else { let target_type = self.parser.parse_data_type()?; diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 1166d58512ac..eed8fc1f9cc7 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -15,11 +15,10 @@ use std::fmt::{Debug, Display}; use common_query::AddColumnLocation; +use datatypes::schema::ChangeFulltextOptions; use sqlparser::ast::{ColumnDef, DataType, Ident, ObjectName, TableConstraint}; use sqlparser_derive::{Visit, VisitMut}; -use crate::statements::OptionMap; - #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut)] pub struct AlterTable { table_name: ObjectName, @@ -73,10 +72,10 @@ pub enum AlterTableOperation { DropColumn { name: Ident }, /// `RENAME ` RenameTable { new_table_name: String }, - /// `SET COLUMN FULLTEXT WITH` + /// `SET COLUMN FULLTEXT WITH ` AlterColumnFulltext { column_name: Ident, - options: OptionMap, + options: ChangeFulltextOptions, }, } @@ -108,11 +107,7 @@ impl Display for AlterTableOperation { column_name, options, } => { - write!( - f, - r#"SET COLUMN {column_name} FULLTEXT WITH {:?}"#, - options.to_str_map() - ) + write!(f, r#"SET COLUMN {column_name} FULLTEXT WITH {:?}"#, options) } } } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 3907807daa49..909d30a02908 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -28,7 +28,7 @@ use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; use common_macro::stack_trace_debug; use datatypes::arrow::datatypes::FieldRef; -use datatypes::schema::{parse_fulltext_options, ColumnSchema, FulltextOptions, Schema, SchemaRef}; +use datatypes::schema::{ChangeFulltextOptions, ColumnSchema, FulltextOptions, Schema, SchemaRef}; use serde::de::Error; use serde::{Deserialize, Deserializer, Serialize}; use snafu::{ensure, Location, OptionExt, ResultExt, Snafu}; @@ -631,7 +631,7 @@ impl RegionMetadataBuilder { fn change_column_fulltext( &mut self, column_name: String, - options: HashMap, + options: ChangeFulltextOptions, ) -> Result<()> { for column_meta in self.column_metadatas.iter_mut() { if column_name.eq(&column_meta.column_schema.name) { @@ -642,8 +642,14 @@ impl RegionMetadataBuilder { FulltextOptions::default() }; - if let Some(f) = parse_fulltext_options(&options) { - fulltext = f; + if let Some(enable) = options.enable { + fulltext.enable = enable; + } + if let Some(analyzer) = options.analyzer { + fulltext.analyzer = analyzer.try_into().context(InvalidFulltextOptionsSnafu)?; + } + if let Some(case_sensitive) = options.case_sensitive { + fulltext.case_sensitive = case_sensitive; } column_meta @@ -772,18 +778,11 @@ pub enum MetadataError { location: Location, }, - #[snafu(display( - "Invalid fulltext options, column: {}, key: {}, value: {}", - column_name, - key, - value - ))] + #[snafu(display("Invalid fulltext options"))] InvalidFulltextOptions { + source: datatypes::error::Error, #[snafu(implicit)] location: Location, - column_name: String, - key: String, - value: String, }, } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 69d90bfbef14..1204e35a775c 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -25,20 +25,18 @@ use api::v1::region::{ use api::v1::{self, Rows, SemanticType}; pub use common_base::AffectedRows; use datatypes::data_type::ConcreteDataType; +use datatypes::schema::ChangeFulltextOptions; use snafu::{ensure, OptionExt}; use strum::IntoStaticStr; use crate::logstore::entry; use crate::metadata::{ - ColumnMetadata, InvalidFulltextOptionsSnafu, InvalidRawRegionRequestSnafu, - InvalidRegionRequestSnafu, MetadataError, RegionMetadata, Result, + ColumnMetadata, InvalidRawRegionRequestSnafu, InvalidRegionRequestSnafu, MetadataError, + RegionMetadata, Result, }; use crate::path_utils::region_dir; use crate::storage::{ColumnId, RegionId, ScanRequest}; -const COLUMN_FULLTEXT_OPT_KEY_ANALYZER: &str = "analyzer"; -const COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE: &str = "case_sensitive"; - #[derive(Debug, IntoStaticStr)] pub enum RegionRequest { Put(RegionPutRequest), @@ -397,7 +395,7 @@ pub enum AlterKind { /// Name of column to change. column_name: String, /// Fulltext options. - options: HashMap, + options: ChangeFulltextOptions, }, } @@ -422,10 +420,9 @@ impl AlterKind { col_to_change.validate(metadata)?; } } - AlterKind::ChangeColumnFulltext { - column_name, - options, - } => Self::validate_column_fulltext_to_alter(metadata, column_name, options)?, + AlterKind::ChangeColumnFulltext { column_name, .. } => { + Self::validate_column_fulltext_to_alter(metadata, column_name)? + } } Ok(()) } @@ -464,24 +461,10 @@ impl AlterKind { Ok(()) } - /// Validates full-text options for altering a specific column in the region metadata. - /// - /// This function checks if the provided column exists in the region metadata and validates - /// the full-text options (`analyzer` and `case_sensitive`) for correctness. If any option is - /// invalid, it returns an error. - /// - /// # Parameters: - /// - `metadata`: The region metadata that contains column information. - /// - `column_name`: The name of the column to validate. - /// - `options`: A `HashMap` containing the full-text options to be validated. - /// - /// # Returns: - /// - `Ok(())` if the column exists and the options are valid. - /// - `Err` if the column is not found or if any of the options are invalid. + /// Returns an error if the column to change fulltext is invalid. pub fn validate_column_fulltext_to_alter( metadata: &RegionMetadata, column_name: &str, - options: &HashMap, ) -> Result<()> { // Ensure the column exists in the region metadata, otherwise return an error metadata @@ -491,48 +474,6 @@ impl AlterKind { err: format!("column {} not found", column_name), })?; - // Validate the full-text options in a simplified manner - for (key, value) in options { - let value_lower = value.to_ascii_lowercase(); - match key.to_ascii_lowercase().as_str() { - // Validate the "analyzer" option - COLUMN_FULLTEXT_OPT_KEY_ANALYZER => { - if !matches!(value_lower.as_str(), "english") - && !matches!(value_lower.as_str(), "chinese") - { - return InvalidFulltextOptionsSnafu { - column_name, - key, - value, - } - .fail(); - } - } - // Validate the "case_sensitive" option - COLUMN_FULLTEXT_OPT_KEY_CASE_SENSITIVE => { - if !matches!(value_lower.as_str(), "true") - && !matches!(value_lower.as_str(), "false") - { - return InvalidFulltextOptionsSnafu { - column_name, - key, - value, - } - .fail(); - } - } - // Return an error for any unknown options - _ => { - return InvalidFulltextOptionsSnafu { - column_name, - key, - value, - } - .fail() - } - } - } - Ok(()) } } @@ -564,7 +505,11 @@ impl TryFrom for AlterKind { } alter_request::Kind::ChangeFulltext(x) => AlterKind::ChangeColumnFulltext { column_name: x.column_name, - options: x.options, + options: ChangeFulltextOptions { + enable: x.enable, + analyzer: x.analyzer, + case_sensitive: x.case_sensitive, + }, }, }; diff --git a/src/table/src/error.rs b/src/table/src/error.rs index 11324b5dcb88..977c2aa415be 100644 --- a/src/table/src/error.rs +++ b/src/table/src/error.rs @@ -145,6 +145,13 @@ pub enum Error { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Invalid fulltext options"))] + InvalidFulltextOptions { + source: datatypes::error::Error, + #[snafu(implicit)] + location: Location, + }, } impl ErrorExt for Error { @@ -156,12 +163,13 @@ impl ErrorExt for Error { } Error::RemoveColumnInIndex { .. } | Error::BuildColumnDescriptor { .. } - | Error::InvalidAlterRequest { .. } => StatusCode::InvalidArguments, + | Error::InvalidAlterRequest { .. } + | Error::InvalidFulltextColumnType { .. } => StatusCode::InvalidArguments, Error::TablesRecordBatch { .. } => StatusCode::Unexpected, Error::ColumnExists { .. } => StatusCode::TableColumnExists, Error::SchemaBuild { source, .. } => source.status_code(), Error::TableOperation { source } => source.status_code(), - Error::InvalidFulltextColumnType { source, .. } => source.status_code(), + Error::InvalidFulltextOptions { source, .. } => source.status_code(), Error::ColumnNotExists { .. } => StatusCode::TableColumnNotFound, Error::Unsupported { .. } => StatusCode::Unsupported, Error::ParseTableOption { .. } => StatusCode::InvalidArguments, diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 4f59f6947205..e438ebffb080 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -19,7 +19,7 @@ use common_query::AddColumnLocation; use datafusion_expr::TableProviderFilterPushDown; pub use datatypes::error::{Error as ConvertError, Result as ConvertResult}; use datatypes::schema::{ - parse_fulltext_options, ColumnSchema, FulltextOptions, RawSchema, Schema, SchemaBuilder, + ChangeFulltextOptions, ColumnSchema, FulltextOptions, RawSchema, Schema, SchemaBuilder, SchemaRef, }; use derive_builder::Builder; @@ -597,7 +597,7 @@ impl TableMeta { &self, table_name: &str, column_name: &String, - options: HashMap, + options: ChangeFulltextOptions, ) -> Result { let table_schema = &self.schema; let mut meta_builder = self.new_meta_builder(); @@ -615,8 +615,16 @@ impl TableMeta { FulltextOptions::default() }; - if let Some(f) = parse_fulltext_options(&options) { - fulltext = f; + if let Some(enable) = options.enable { + fulltext.enable = enable; + } + if let Some(analyzer) = options.analyzer { + fulltext.analyzer = analyzer + .try_into() + .context(error::InvalidFulltextOptionsSnafu)?; + } + if let Some(case_sensitive) = options.case_sensitive { + fulltext.case_sensitive = case_sensitive; } let columns: Result> = table_schema @@ -659,9 +667,7 @@ impl TableMeta { Ok(meta_builder) } - Err(e) => { - return Err(e); - } + Err(e) => Err(e), } } diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 24d756943354..9be6e4ee8cfd 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -25,7 +25,7 @@ use common_query::AddColumnLocation; use common_time::range::TimestampRange; use datatypes::data_type::ConcreteDataType; use datatypes::prelude::VectorRef; -use datatypes::schema::ColumnSchema; +use datatypes::schema::{ChangeFulltextOptions, ColumnSchema}; 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}; @@ -213,7 +213,7 @@ pub enum AlterKind { }, ChangeFulltext { column_name: String, - options: HashMap, + options: ChangeFulltextOptions, }, } diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result index 688e9b8af27f..abfb1c34a4ac 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -111,7 +111,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'English', case_sensitive = 'yes'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: message, key: case_sensitive, value: yes +Error: 2000(InvalidSyntax), sql parser error: failed to parse fulltext options SHOW CREATE TABLE test; @@ -132,7 +132,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN message SET FULLTEXT WITH(analyzer = 'Chinglish', case_sensitive = 'false'); -Error: 1004(InvalidArguments), Invalid fulltext options, column: message, key: analyzer, value: Chinglish +Error: 2000(InvalidSyntax), sql parser error: failed to parse fulltext options SHOW CREATE TABLE test; @@ -174,7 +174,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'no'); -Error: 1004(InvalidArguments), Invalid data type TimestampMillisecond for fulltext +Error: 2000(InvalidSyntax), sql parser error: failed to parse fulltext options SHOW CREATE TABLE test; From be4bfcbb8e8d6e58368c0cefb0b78f97103fa682 Mon Sep 17 00:00:00 2001 From: irenjj Date: Mon, 14 Oct 2024 23:23:09 +0800 Subject: [PATCH 18/20] fix test --- src/datatypes/src/schema.rs | 9 --------- src/datatypes/src/schema/column_schema.rs | 22 ++++++++++++++++++++++ src/sql/src/parsers/alter_parser.rs | 5 ++++- src/table/src/metadata.rs | 12 ++++++++++-- 4 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/datatypes/src/schema.rs b/src/datatypes/src/schema.rs index 9cfce28a36a7..c2e06433f4b1 100644 --- a/src/datatypes/src/schema.rs +++ b/src/datatypes/src/schema.rs @@ -487,13 +487,4 @@ mod tests { .build() .is_err()); } - - #[test] - fn test_validate_fulltext_options() { - let options = HashMap::from([(String::from("analyzer"), String::from("English"))]); - assert!(parse_fulltext_options(&options).is_some()); - - let options = HashMap::from([(String::from("analyzer"), String::from("Chinglish"))]); - assert!(parse_fulltext_options(&options).is_some()); - } } diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index 8a3b2824d969..24aea025745a 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -382,6 +382,28 @@ impl fmt::Display for ChangeFulltextOptions { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "fulltext options: ")?; + if let Some(enable) = self.enable { + write!(f, "enable={}", enable)?; + } else { + write!(f, "enable=None")?; + } + + if let Some(analyzer) = self.analyzer { + if analyzer == 0 { + write!(f, ", analyzer=English")?; + } else { + write!(f, ", analyzer=Chinese")?; + } + } else { + write!(f, ", analyzer=None")?; + } + + if let Some(case_sensitive) = self.case_sensitive { + write!(f, ", case_sensitive={}", case_sensitive)?; + } else { + write!(f, ", case_sensitive=None")?; + } + Ok(()) } } diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 8f9f5815acb2..307c75ce552e 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -466,7 +466,10 @@ mod tests { options, } => { assert_eq!(column_name.value, r#"message"#); - assert_eq!(options.get("analyzer").unwrap(), "Chinese"); + assert!(options.analyzer.is_some()); + assert_eq!(options.analyzer.unwrap(), 1); + assert!(options.case_sensitive.is_some()); + assert_eq!(options.case_sensitive.unwrap(), true); } _ => unreachable!(), } diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index e438ebffb080..9d362c0e2336 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -1415,7 +1415,11 @@ mod tests { let alter_kind = AlterKind::ChangeFulltext { column_name: "col1".to_string(), - options: HashMap::from([(String::from("analyzer"), String::from("English"))]), + options: ChangeFulltextOptions { + enable: Some(true), + analyzer: Some(0), + case_sensitive: None, + }, }; assert!(meta .builder_with_alter_kind("my_table", &alter_kind, false) @@ -1423,7 +1427,11 @@ mod tests { let alter_kind = AlterKind::ChangeFulltext { column_name: "col1".to_string(), - options: HashMap::from([(String::from("case_sensitive"), String::from("no"))]), + options: ChangeFulltextOptions { + enable: None, + analyzer: None, + case_sensitive: Some(false), + }, }; let err = meta .builder_with_alter_kind("my_table", &alter_kind, false) From e43624200fcd1e9ae8258b6e6a0fc6d92b2cefa2 Mon Sep 17 00:00:00 2001 From: irenjj Date: Tue, 15 Oct 2024 21:48:16 +0800 Subject: [PATCH 19/20] fix --- src/api/src/error.rs | 14 +++++++++--- src/api/src/v1/column_def.rs | 24 ++++++++++++++++++++- src/common/grpc-expr/src/alter.rs | 26 ++++++++--------------- src/common/grpc-expr/src/error.rs | 11 +++++++++- src/datatypes/src/error.rs | 7 +++++- src/datatypes/src/schema/column_schema.rs | 1 + 6 files changed, 60 insertions(+), 23 deletions(-) diff --git a/src/api/src/error.rs b/src/api/src/error.rs index 07e43e477299..711e4791ff42 100644 --- a/src/api/src/error.rs +++ b/src/api/src/error.rs @@ -66,15 +66,23 @@ pub enum Error { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Failed to decode proto"))] + DecodeProto { + #[snafu(implicit)] + location: Location, + #[snafu(source)] + error: prost::DecodeError, + }, } impl ErrorExt for Error { fn status_code(&self) -> StatusCode { match self { Error::UnknownColumnDataType { .. } => StatusCode::InvalidArguments, - Error::IntoColumnDataType { .. } | Error::SerializeJson { .. } => { - StatusCode::Unexpected - } + Error::IntoColumnDataType { .. } + | Error::SerializeJson { .. } + | Error::DecodeProto { .. } => StatusCode::Unexpected, Error::ConvertColumnDefaultConstraint { source, .. } | Error::InvalidColumnDefaultConstraint { source, .. } => source.status_code(), } diff --git a/src/api/src/v1/column_def.rs b/src/api/src/v1/column_def.rs index 9b05ad3c2f9c..bf61811157aa 100644 --- a/src/api/src/v1/column_def.rs +++ b/src/api/src/v1/column_def.rs @@ -15,8 +15,10 @@ use std::collections::HashMap; use datatypes::schema::{ - ColumnDefaultConstraint, ColumnSchema, FulltextOptions, COMMENT_KEY, FULLTEXT_KEY, + ChangeFulltextOptions, ColumnDefaultConstraint, ColumnSchema, FulltextOptions, COMMENT_KEY, + FULLTEXT_KEY, }; +use greptime_proto::v1::{Analyzer, ChangeFulltext}; use snafu::ResultExt; use crate::error::{self, Result}; @@ -93,6 +95,26 @@ pub fn options_from_fulltext(fulltext: &FulltextOptions) -> Result Result { + let _ = analyzer + .map(|a| Analyzer::try_from(a)) + .transpose() + .context(error::DecodeProtoSnafu)?; + Ok(ChangeFulltextOptions { + enable: *enable, + analyzer: *analyzer, + case_sensitive: *case_sensitive, + }) +} + #[cfg(test)] mod tests { diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index ba5dd8718b8e..8d0145cf86bf 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -16,18 +16,18 @@ use api::helper::ColumnDataTypeWrapper; use api::v1::add_column_location::LocationType; use api::v1::alter_expr::Kind; use api::v1::{ - column_def, AddColumnLocation as Location, AlterExpr, ChangeColumnTypes, ChangeFulltext, - CreateTableExpr, DropColumns, RenameTable, SemanticType, + column_def, AddColumnLocation as Location, AlterExpr, ChangeColumnTypes, CreateTableExpr, + DropColumns, RenameTable, SemanticType, }; use common_query::AddColumnLocation; -use datatypes::schema::{ChangeFulltextOptions, ColumnSchema, RawSchema}; +use datatypes::schema::{ColumnSchema, RawSchema}; use snafu::{ensure, OptionExt, ResultExt}; use table::metadata::TableId; use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest, ChangeColumnTypeRequest}; use crate::error::{ - InvalidColumnDefSnafu, MissingFieldSnafu, MissingTimestampColumnSnafu, Result, - UnknownLocationTypeSnafu, + DecodeProtoSnafu, InvalidColumnDefSnafu, MissingFieldSnafu, MissingTimestampColumnSnafu, + Result, UnknownLocationTypeSnafu, }; const LOCATION_TYPE_FIRST: i32 = LocationType::First as i32; @@ -92,18 +92,10 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result { AlterKind::RenameTable { new_table_name } } - Kind::ChangeFulltext(ChangeFulltext { - column_name, - enable, - analyzer, - case_sensitive, - }) => AlterKind::ChangeFulltext { - column_name, - options: ChangeFulltextOptions { - enable, - analyzer, - case_sensitive, - }, + Kind::ChangeFulltext(change) => AlterKind::ChangeFulltext { + column_name: change.column_name.clone(), + options: column_def::try_as_change_fulltext_options(&change) + .context(DecodeProtoSnafu)?, }, }; diff --git a/src/common/grpc-expr/src/error.rs b/src/common/grpc-expr/src/error.rs index 184ce8931ec5..64a0b2e525a9 100644 --- a/src/common/grpc-expr/src/error.rs +++ b/src/common/grpc-expr/src/error.rs @@ -129,6 +129,14 @@ pub enum Error { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Failed to decode proto"))] + DecodeProto { + #[snafu(implicit)] + location: Location, + #[snafu(source)] + error: api::error::Error, + }, } pub type Result = std::result::Result; @@ -152,7 +160,8 @@ impl ErrorExt for Error { Error::UnknownColumnDataType { .. } | Error::InvalidFulltextOptions { .. } - | Error::InvalidFulltextColumnType { .. } => StatusCode::InvalidArguments, + | Error::InvalidFulltextColumnType { .. } + | Error::DecodeProto { .. } => StatusCode::InvalidArguments, } } diff --git a/src/datatypes/src/error.rs b/src/datatypes/src/error.rs index 8f68125a5371..177982dfb0c6 100644 --- a/src/datatypes/src/error.rs +++ b/src/datatypes/src/error.rs @@ -206,9 +206,14 @@ pub enum Error { location: Location, }, - #[snafu(display("Invalid data type {} for fulltext", data_type))] + #[snafu(display( + "Invalid data type {} for fulltext in column {}", + data_type, + column_name + ))] InvalidFulltextDataType { data_type: String, + column_name: String, #[snafu(implicit)] location: Location, }, diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index 24aea025745a..5f0884744b02 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -266,6 +266,7 @@ impl ColumnSchema { if self.data_type != ConcreteDataType::string_datatype() { return error::InvalidFulltextDataTypeSnafu { data_type: self.data_type.to_string(), + column_name: self.name.clone(), } .fail(); } From 6e591b99c213911d04fe8be427f3fa0ed0630831 Mon Sep 17 00:00:00 2001 From: irenjj Date: Wed, 16 Oct 2024 20:46:22 +0800 Subject: [PATCH 20/20] fix --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/api/src/v1/column_def.rs | 22 ++++++---- src/datatypes/src/schema/column_schema.rs | 41 ++++++------------- src/operator/src/expr_factory.rs | 14 ++++--- src/sql/src/parsers/alter_parser.rs | 6 ++- src/store-api/src/metadata.rs | 11 ++++- src/store-api/src/region_request.rs | 11 ++--- src/table/src/metadata.rs | 8 ++-- .../alter/change_fulltext_options.result | 2 +- 10 files changed, 62 insertions(+), 57 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6fb1bd9a0502..ea6cf9997f8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4254,7 +4254,7 @@ checksum = "d2fabcfbdc87f4758337ca535fb41a6d701b65693ce38287d856d1674551ec9b" [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/irenjj/greptime-proto.git?rev=2a8c2dfb7447b7fe5f0b8f3df868f041600caec2#2a8c2dfb7447b7fe5f0b8f3df868f041600caec2" +source = "git+https://github.com/irenjj/greptime-proto.git?rev=ed13098712a87674e1d41be8c9a8ceb23585c009#ed13098712a87674e1d41be8c9a8ceb23585c009" dependencies = [ "prost 0.12.6", "serde", diff --git a/Cargo.toml b/Cargo.toml index 4af1d18beba8..4961430ff7ae 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -119,7 +119,7 @@ etcd-client = { version = "0.13" } fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "2a8c2dfb7447b7fe5f0b8f3df868f041600caec2" } +greptime-proto = { git = "https://github.com/irenjj/greptime-proto.git", rev = "ed13098712a87674e1d41be8c9a8ceb23585c009" } humantime = "2.1" humantime-serde = "1.1" itertools = "0.10" diff --git a/src/api/src/v1/column_def.rs b/src/api/src/v1/column_def.rs index bf61811157aa..9702a445d9cd 100644 --- a/src/api/src/v1/column_def.rs +++ b/src/api/src/v1/column_def.rs @@ -15,8 +15,8 @@ use std::collections::HashMap; use datatypes::schema::{ - ChangeFulltextOptions, ColumnDefaultConstraint, ColumnSchema, FulltextOptions, COMMENT_KEY, - FULLTEXT_KEY, + ChangeFulltextOptions, ColumnDefaultConstraint, ColumnSchema, FulltextAnalyzer, + FulltextOptions, COMMENT_KEY, FULLTEXT_KEY, }; use greptime_proto::v1::{Analyzer, ChangeFulltext}; use snafu::ResultExt; @@ -95,6 +95,17 @@ pub fn options_from_fulltext(fulltext: &FulltextOptions) -> Result) -> Result> { + let analyzer = analyzer + .map(Analyzer::try_from) + .transpose() + .context(error::DecodeProtoSnafu)?; + Ok(analyzer.map(|a| match a { + Analyzer::English => FulltextAnalyzer::English, + Analyzer::Chinese => FulltextAnalyzer::Chinese, + })) +} /// Tries to construct a `ChangeFulltextOptions` from the given `ChangeFulltext`. pub fn try_as_change_fulltext_options( ChangeFulltext { @@ -104,13 +115,10 @@ pub fn try_as_change_fulltext_options( .. }: &ChangeFulltext, ) -> Result { - let _ = analyzer - .map(|a| Analyzer::try_from(a)) - .transpose() - .context(error::DecodeProtoSnafu)?; + let analyzer = try_as_fulltext_option(*analyzer)?; Ok(ChangeFulltextOptions { enable: *enable, - analyzer: *analyzer, + analyzer, case_sensitive: *case_sensitive, }) } diff --git a/src/datatypes/src/schema/column_schema.rs b/src/datatypes/src/schema/column_schema.rs index 5f0884744b02..852fe32ff4f6 100644 --- a/src/datatypes/src/schema/column_schema.rs +++ b/src/datatypes/src/schema/column_schema.rs @@ -349,62 +349,45 @@ pub struct FulltextOptions { } /// Fulltext analyzer. -#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default)] +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Default, Visit, VisitMut)] pub enum FulltextAnalyzer { #[default] English, Chinese, } -impl TryFrom for FulltextAnalyzer { - type Error = Error; - - fn try_from(value: i32) -> Result { - match value { - 0 => Ok(FulltextAnalyzer::English), - 1 => Ok(FulltextAnalyzer::Chinese), - _ => Err(error::InvalidFulltextOptionsSnafu { - key: "analyzer".to_string(), - value: value.to_string(), - } - .build()), - } - } -} - #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, Visit, VisitMut, Default)] pub struct ChangeFulltextOptions { pub enable: Option, - pub analyzer: Option, + pub analyzer: Option, pub case_sensitive: Option, } impl fmt::Display for ChangeFulltextOptions { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!(f, "fulltext options: ")?; - if let Some(enable) = self.enable { write!(f, "enable={}", enable)?; } else { write!(f, "enable=None")?; } - - if let Some(analyzer) = self.analyzer { - if analyzer == 0 { - write!(f, ", analyzer=English")?; - } else { - write!(f, ", analyzer=Chinese")?; + if let Some(analyzer) = &self.analyzer { + match analyzer { + FulltextAnalyzer::English => { + write!(f, ", analyzer=English")?; + } + FulltextAnalyzer::Chinese => { + write!(f, ", analyzer=Chinese")?; + } } } else { write!(f, ", analyzer=None")?; } - if let Some(case_sensitive) = self.case_sensitive { write!(f, ", case_sensitive={}", case_sensitive)?; } else { write!(f, ", case_sensitive=None")?; } - Ok(()) } } @@ -428,11 +411,11 @@ impl TryFrom> for ChangeFulltextOptions { match analyzer.to_ascii_lowercase().as_str() { "english" => { fulltext.enable = Some(true); - fulltext.analyzer = Some(0); + fulltext.analyzer = Some(FulltextAnalyzer::English); } "chinese" => { fulltext.enable = Some(true); - fulltext.analyzer = Some(1); + fulltext.analyzer = Some(FulltextAnalyzer::Chinese); } _ => { // If the analyzer is invalid, return None to indicate failure diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 61da09d6bff6..bea41810b534 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -18,15 +18,15 @@ 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, ChangeColumnType, ChangeColumnTypes, ChangeFulltext, - ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, CreateViewExpr, - DropColumn, DropColumns, ExpireAfter, RenameTable, SemanticType, TableName, + AddColumn, AddColumns, AlterExpr, Analyzer, ChangeColumnType, ChangeColumnTypes, + ChangeFulltext, ColumnDataType, ColumnDataTypeExtension, CreateFlowExpr, CreateTableExpr, + CreateViewExpr, DropColumn, DropColumns, ExpireAfter, RenameTable, SemanticType, TableName, }; use common_error::ext::BoxedError; use common_grpc_expr::util::ColumnExpr; use common_time::Timezone; use datafusion::sql::planner::object_name_to_table_reference; -use datatypes::schema::{ColumnSchema, COMMENT_KEY}; +use datatypes::schema::{ColumnSchema, FulltextAnalyzer, COMMENT_KEY}; use file_engine::FileOptions; use query::sql::{ check_file_to_table_schema_compatibility, file_column_schemas_to_table, @@ -489,7 +489,11 @@ pub(crate) fn to_alter_expr( } => Kind::ChangeFulltext(ChangeFulltext { column_name: column_name.value.to_string(), enable: options.enable, - analyzer: options.analyzer, + analyzer: match options.analyzer { + Some(FulltextAnalyzer::Chinese) => Some(Analyzer::Chinese as i32), + Some(FulltextAnalyzer::English) => Some(Analyzer::English as i32), + None => None, + }, case_sensitive: options.case_sensitive, }), }; diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 307c75ce552e..e45fed7fe5a4 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -143,6 +143,7 @@ mod tests { use std::assert_matches::assert_matches; use common_error::ext::ErrorExt; + use datatypes::schema::FulltextAnalyzer; use sqlparser::ast::{ColumnOption, DataType}; use super::*; @@ -467,7 +468,10 @@ mod tests { } => { assert_eq!(column_name.value, r#"message"#); assert!(options.analyzer.is_some()); - assert_eq!(options.analyzer.unwrap(), 1); + assert_eq!( + options.analyzer.as_ref().unwrap().clone(), + FulltextAnalyzer::Chinese + ); assert!(options.case_sensitive.is_some()); assert_eq!(options.case_sensitive.unwrap(), true); } diff --git a/src/store-api/src/metadata.rs b/src/store-api/src/metadata.rs index 909d30a02908..c4f133ef2e97 100644 --- a/src/store-api/src/metadata.rs +++ b/src/store-api/src/metadata.rs @@ -645,8 +645,8 @@ impl RegionMetadataBuilder { if let Some(enable) = options.enable { fulltext.enable = enable; } - if let Some(analyzer) = options.analyzer { - fulltext.analyzer = analyzer.try_into().context(InvalidFulltextOptionsSnafu)?; + if let Some(ref analyzer) = options.analyzer { + fulltext.analyzer = analyzer.clone(); } if let Some(case_sensitive) = options.case_sensitive { fulltext.case_sensitive = case_sensitive; @@ -784,6 +784,13 @@ pub enum MetadataError { #[snafu(implicit)] location: Location, }, + + #[snafu(display("Invalid fulltext options in proto"))] + InvalidFulltextOptionsProto { + source: api::error::Error, + #[snafu(implicit)] + location: Location, + }, } impl ErrorExt for MetadataError { diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 1204e35a775c..95304a144838 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -22,17 +22,17 @@ use api::v1::region::{ CompactRequest, CreateRequest, CreateRequests, DeleteRequests, DropRequest, DropRequests, FlushRequest, InsertRequests, OpenRequest, TruncateRequest, }; -use api::v1::{self, Rows, SemanticType}; +use api::v1::{self, column_def, Rows, SemanticType}; pub use common_base::AffectedRows; use datatypes::data_type::ConcreteDataType; use datatypes::schema::ChangeFulltextOptions; -use snafu::{ensure, OptionExt}; +use snafu::{ensure, OptionExt, ResultExt}; use strum::IntoStaticStr; use crate::logstore::entry; use crate::metadata::{ - ColumnMetadata, InvalidRawRegionRequestSnafu, InvalidRegionRequestSnafu, MetadataError, - RegionMetadata, Result, + ColumnMetadata, InvalidFulltextOptionsProtoSnafu, InvalidRawRegionRequestSnafu, + InvalidRegionRequestSnafu, MetadataError, RegionMetadata, Result, }; use crate::path_utils::region_dir; use crate::storage::{ColumnId, RegionId, ScanRequest}; @@ -507,7 +507,8 @@ impl TryFrom for AlterKind { column_name: x.column_name, options: ChangeFulltextOptions { enable: x.enable, - analyzer: x.analyzer, + analyzer: column_def::try_as_fulltext_option(x.analyzer) + .context(InvalidFulltextOptionsProtoSnafu)?, case_sensitive: x.case_sensitive, }, }, diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 9d362c0e2336..08affb5907db 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -619,9 +619,7 @@ impl TableMeta { fulltext.enable = enable; } if let Some(analyzer) = options.analyzer { - fulltext.analyzer = analyzer - .try_into() - .context(error::InvalidFulltextOptionsSnafu)?; + fulltext.analyzer = analyzer; } if let Some(case_sensitive) = options.case_sensitive { fulltext.case_sensitive = case_sensitive; @@ -943,7 +941,7 @@ mod tests { use common_error::ext::ErrorExt; use common_error::status_code::StatusCode; use datatypes::data_type::ConcreteDataType; - use datatypes::schema::{ColumnSchema, Schema, SchemaBuilder}; + use datatypes::schema::{ColumnSchema, FulltextAnalyzer, Schema, SchemaBuilder}; use super::*; @@ -1417,7 +1415,7 @@ mod tests { column_name: "col1".to_string(), options: ChangeFulltextOptions { enable: Some(true), - analyzer: Some(0), + analyzer: Some(FulltextAnalyzer::English), case_sensitive: None, }, }; diff --git a/tests/cases/standalone/common/alter/change_fulltext_options.result b/tests/cases/standalone/common/alter/change_fulltext_options.result index abfb1c34a4ac..b344f61fb5d5 100644 --- a/tests/cases/standalone/common/alter/change_fulltext_options.result +++ b/tests/cases/standalone/common/alter/change_fulltext_options.result @@ -153,7 +153,7 @@ SHOW CREATE TABLE test; ALTER TABLE test MODIFY COLUMN time SET FULLTEXT WITH(analyzer = 'Chinese', case_sensitive = 'false'); -Error: 1004(InvalidArguments), Invalid data type TimestampMillisecond for fulltext +Error: 1004(InvalidArguments), Invalid data type TimestampMillisecond for fulltext in column time SHOW CREATE TABLE test;