From 2a9561e7e9b80f58fc3b7bc6e9b6c15aef4f03bc Mon Sep 17 00:00:00 2001 From: kould Date: Wed, 24 Apr 2024 18:21:05 +0800 Subject: [PATCH 01/15] feat: support `ALTER COLUMN xxx TYPE xxx` --- src/common/grpc-expr/src/alter.rs | 67 ++++++- .../src/ddl/alter_table/region_request.rs | 179 ++++++++++++------ src/operator/src/expr_factory.rs | 61 +++++- src/sql/src/parsers/alter_parser.rs | 68 ++++++- src/sql/src/statements/alter.rs | 34 +++- src/store-api/src/region_request.rs | 26 +++ .../common/alter/change_col_type.result | 91 +++++++++ .../common/alter/change_col_type.sql | 29 +++ .../alter/change_col_type_not_null.result | 43 +++++ .../common/alter/change_col_type_not_null.sql | 13 ++ 10 files changed, 545 insertions(+), 66 deletions(-) create mode 100644 tests/cases/standalone/common/alter/change_col_type.result create mode 100644 tests/cases/standalone/common/alter/change_col_type.sql create mode 100644 tests/cases/standalone/common/alter/change_col_type_not_null.result create mode 100644 tests/cases/standalone/common/alter/change_col_type_not_null.sql diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index fcf4486b4628..baf1c4529c05 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -12,17 +12,18 @@ // See the License for the specific language governing permissions and // limitations under the License. +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, CreateTableExpr, DropColumns, - RenameTable, SemanticType, + column_def, AddColumnLocation as Location, AlterExpr, ChangeColumnTypes, CreateTableExpr, + DropColumns, RenameTable, SemanticType, }; use common_query::AddColumnLocation; use datatypes::schema::{ColumnSchema, RawSchema}; use snafu::{ensure, OptionExt, ResultExt}; use table::metadata::TableId; -use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest}; +use table::requests::{AddColumnRequest, AlterKind, AlterTableRequest, ChangeColumnTypeRequest}; use crate::error::{ InvalidColumnDefSnafu, MissingFieldSnafu, MissingTimestampColumnSnafu, Result, @@ -64,6 +65,27 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result { + let change_column_type_requests = change_column_types + .into_iter() + .map(|cct| { + let target_type = + ColumnDataTypeWrapper::new(cct.target_type(), cct.target_type_extension) + .into(); + + Ok(ChangeColumnTypeRequest { + column_name: cct.column_name, + target_type, + }) + }) + .collect::>>()?; + + AlterKind::ChangeColumnTypes { + columns: change_column_type_requests, + } + } Kind::DropColumns(DropColumns { drop_columns }) => AlterKind::DropColumns { names: drop_columns.into_iter().map(|c| c.name).collect(), }, @@ -138,7 +160,10 @@ fn parse_location(location: Option) -> Result columns, + _ => unreachable!(), + }; + + let change_column_type = change_column_types.pop().unwrap(); + assert_eq!("mem_usage", change_column_type.column_name); + assert_eq!( + ConcreteDataType::string_datatype(), + change_column_type.target_type + ); + } + #[test] fn test_drop_column_expr() { let expr = AlterExpr { 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 265466b69442..8ebc62c49bdc 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -15,8 +15,8 @@ use api::v1::alter_expr::Kind; use api::v1::region::region_request::Body; use api::v1::region::{ - alter_request, AddColumn, AddColumns, AlterRequest, DropColumn, DropColumns, RegionColumnDef, - RegionRequest, RegionRequestHeader, + alter_request, AddColumn, AddColumns, AlterRequest, ChangeColumnType, ChangeColumnTypes, + DropColumn, DropColumns, RegionColumnDef, RegionRequest, RegionRequestHeader, }; use common_telemetry::tracing_context::TracingContext; use snafu::OptionExt; @@ -91,6 +91,23 @@ fn create_proto_alter_kind( add_columns, }))) } + Kind::ChangeColumnTypes(x) => { + let change_column_types = x + .change_column_types + .iter() + .map(|change_column_type| ChangeColumnType { + column_name: change_column_type.column_name.clone(), + target_type: change_column_type.target_type, + target_type_extension: change_column_type.target_type_extension.clone(), + }) + .collect(); + + Ok(Some(alter_request::Kind::ChangeColumnTypes( + ChangeColumnTypes { + change_column_types, + }, + ))) + } Kind::DropColumns(x) => { let drop_columns = x .drop_columns @@ -119,8 +136,8 @@ mod tests { use api::v1::region::region_request::Body; use api::v1::region::RegionColumnDef; use api::v1::{ - region, AddColumn, AddColumnLocation, AddColumns, AlterExpr, ColumnDataType, - ColumnDef as PbColumnDef, SemanticType, + region, AddColumn, AddColumnLocation, AddColumns, AlterExpr, ChangeColumnType, + ChangeColumnTypes, ColumnDataType, ColumnDef as PbColumnDef, SemanticType, }; use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME}; use store_api::storage::RegionId; @@ -194,48 +211,14 @@ mod tests { ) .await .unwrap(); - - let task = AlterTableTask { - alter_table: AlterExpr { - catalog_name: DEFAULT_CATALOG_NAME.to_string(), - schema_name: DEFAULT_SCHEMA_NAME.to_string(), - table_name: table_name.to_string(), - kind: Some(Kind::AddColumns(AddColumns { - add_columns: vec![AddColumn { - column_def: Some(PbColumnDef { - name: "my_tag3".to_string(), - data_type: ColumnDataType::String as i32, - is_nullable: true, - default_constraint: b"hello".to_vec(), - semantic_type: SemanticType::Tag as i32, - comment: String::new(), - ..Default::default() - }), - location: Some(AddColumnLocation { - location_type: LocationType::After as i32, - after_column_name: "my_tag2".to_string(), - }), - }], - })), - }, - }; - - let mut procedure = - AlterTableProcedure::new(cluster_id, table_id, task, ddl_context).unwrap(); - procedure.on_prepare().await.unwrap(); - let Some(Body::Alter(alter_region_request)) = - procedure.make_alter_region_request(region_id).unwrap().body - else { - unreachable!() - }; - assert_eq!(alter_region_request.region_id, region_id.as_u64()); - assert_eq!(alter_region_request.schema_version, 1); - assert_eq!( - alter_region_request.kind, - Some(region::alter_request::Kind::AddColumns( - region::AddColumns { - add_columns: vec![region::AddColumn { - column_def: Some(RegionColumnDef { + { + let add_column_task = AlterTableTask { + alter_table: AlterExpr { + catalog_name: DEFAULT_CATALOG_NAME.to_string(), + schema_name: DEFAULT_SCHEMA_NAME.to_string(), + table_name: table_name.to_string(), + kind: Some(Kind::AddColumns(AddColumns { + add_columns: vec![AddColumn { column_def: Some(PbColumnDef { name: "my_tag3".to_string(), data_type: ColumnDataType::String as i32, @@ -245,15 +228,99 @@ mod tests { comment: String::new(), ..Default::default() }), - column_id: 3, - }), - location: Some(AddColumnLocation { - location_type: LocationType::After as i32, - after_column_name: "my_tag2".to_string(), - }), - }] - } - )) - ); + location: Some(AddColumnLocation { + location_type: LocationType::After as i32, + after_column_name: "my_tag2".to_string(), + }), + }], + })), + }, + }; + + let mut procedure = AlterTableProcedure::new( + cluster_id, + table_id, + add_column_task, + ddl_context.clone(), + ) + .unwrap(); + procedure.on_prepare().await.unwrap(); + let Some(Body::Alter(alter_region_request)) = + procedure.make_alter_region_request(region_id).unwrap().body + else { + unreachable!() + }; + assert_eq!(alter_region_request.region_id, region_id.as_u64()); + assert_eq!(alter_region_request.schema_version, 1); + assert_eq!( + alter_region_request.kind, + Some(region::alter_request::Kind::AddColumns( + region::AddColumns { + add_columns: vec![region::AddColumn { + column_def: Some(RegionColumnDef { + column_def: Some(PbColumnDef { + name: "my_tag3".to_string(), + data_type: ColumnDataType::String as i32, + is_nullable: true, + default_constraint: b"hello".to_vec(), + semantic_type: SemanticType::Tag as i32, + comment: String::new(), + ..Default::default() + }), + column_id: 3, + }), + location: Some(AddColumnLocation { + location_type: LocationType::After as i32, + after_column_name: "my_tag2".to_string(), + }), + }] + } + )) + ); + } + { + let change_column_type_task = AlterTableTask { + alter_table: AlterExpr { + catalog_name: DEFAULT_CATALOG_NAME.to_string(), + schema_name: DEFAULT_SCHEMA_NAME.to_string(), + table_name: table_name.to_string(), + kind: Some(Kind::ChangeColumnTypes(ChangeColumnTypes { + change_column_types: vec![ChangeColumnType { + column_name: "cpu".to_string(), + target_type: ColumnDataType::String as i32, + target_type_extension: None, + }], + })), + }, + }; + + let mut procedure = AlterTableProcedure::new( + cluster_id, + table_id, + change_column_type_task, + ddl_context, + ) + .unwrap(); + procedure.on_prepare().await.unwrap(); + let Some(Body::Alter(alter_region_request)) = + procedure.make_alter_region_request(region_id).unwrap().body + else { + unreachable!() + }; + assert_eq!(alter_region_request.region_id, region_id.as_u64()); + assert_eq!(alter_region_request.schema_version, 1); + assert_eq!( + alter_region_request.kind, + Some(region::alter_request::Kind::ChangeColumnTypes( + region::ChangeColumnTypes { + change_column_types: vec![region::ChangeColumnType { + column_name: "cpu".to_string(), + target_type: ColumnDataType::String as i32, + target_type_extension: None, + }] + } + )) + ); + } } } diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 0a84c4a7315f..200e1dba0b78 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -17,8 +17,8 @@ use std::collections::{HashMap, HashSet}; use api::helper::ColumnDataTypeWrapper; use api::v1::alter_expr::Kind; use api::v1::{ - AddColumn, AddColumns, AlterExpr, Column, ColumnDataType, ColumnDataTypeExtension, - CreateTableExpr, DropColumn, DropColumns, RenameTable, SemanticType, + AddColumn, AddColumns, AlterExpr, ChangeColumnType, ChangeColumnTypes, Column, ColumnDataType, + ColumnDataTypeExtension, CreateTableExpr, DropColumn, DropColumns, RenameTable, SemanticType, }; use common_error::ext::BoxedError; use common_grpc_expr::util::ColumnExpr; @@ -35,7 +35,10 @@ use snafu::{ensure, OptionExt, ResultExt}; use sql::ast::{ColumnDef, ColumnOption, TableConstraint}; use sql::statements::alter::{AlterTable, AlterTableOperation}; use sql::statements::create::{CreateExternalTable, CreateTable, TIME_INDEX}; -use sql::statements::{column_def_to_schema, sql_column_def_to_grpc_column_def}; +use sql::statements::{ + column_def_to_schema, sql_column_def_to_grpc_column_def, sql_data_type_to_concrete_data_type, +}; +use sql::util::to_lowercase_options_map; use table::requests::{TableOptions, FILE_TABLE_META_KEY}; use table::table_reference::TableReference; @@ -469,6 +472,23 @@ pub(crate) fn to_alter_expr( location: location.as_ref().map(From::from), }], }), + AlterTableOperation::ChangeColumnType { + column_name, + target_type, + } => { + let target_type = + sql_data_type_to_concrete_data_type(target_type).context(ParseSqlSnafu)?; + let (target_type, target_type_extension) = ColumnDataTypeWrapper::try_from(target_type) + .map(|w| w.to_parts()) + .context(ColumnDataTypeSnafu)?; + Kind::ChangeColumnTypes(ChangeColumnTypes { + change_column_types: vec![ChangeColumnType { + column_name: column_name.value.to_string(), + target_type: target_type as i32, + target_type_extension, + }], + }) + } AlterTableOperation::DropColumn { name } => Kind::DropColumns(DropColumns { drop_columns: vec![DropColumn { name: name.value.to_string(), @@ -640,4 +660,39 @@ mod tests { if ts.to_iso8601_string() == "2024-01-29 16:01:01+0000") ); } + + #[test] + fn test_to_alter_change_column_type_expr() { + let sql = "ALTER TABLE monitor alter column mem_usage TYPE STRING;"; + let stmt = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap() + .pop() + .unwrap(); + + let Statement::Alter(alter_table) = stmt else { + unreachable!() + }; + + // query context with system timezone UTC. + let expr = to_alter_expr(alter_table.clone(), QueryContext::arc()).unwrap(); + let kind = expr.kind.unwrap(); + + let Kind::ChangeColumnTypes(ChangeColumnTypes { + change_column_types, + }) = kind + else { + unreachable!() + }; + + assert_eq!(1, change_column_types.len()); + let change_column_type = &change_column_types[0]; + + assert_eq!("mem_usage", change_column_type.column_name); + assert_eq!( + ColumnDataType::String as i32, + change_column_type.target_type + ); + assert!(change_column_type.target_type_extension.is_none()); + } } diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 687604e37022..e16ccaabf898 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -73,6 +73,26 @@ impl<'a> ParserContext<'a> { parser.peek_token() ))); } + } else if parser + .expect_keywords(&[Keyword::ALTER, Keyword::COLUMN]) + .is_ok() + { + let column_name = Self::canonicalize_identifier(parser.parse_identifier(false)?); + + if parser.parse_keyword(Keyword::TYPE) { + let target_type = parser.parse_data_type()?; + + AlterTableOperation::ChangeColumnType { + column_name, + target_type, + } + } else { + return Err(ParserError::ParserError(format!( + "expect keyword TYPE or TO after ALTER TABLE ALTER COLUMN {}, found {}", + column_name, + parser.peek_token() + ))); + } } else if parser.parse_keyword(Keyword::RENAME) { let new_table_name_obj_raw = self.parse_object_name()?; let new_table_name_obj = Self::canonicalize_object_name(new_table_name_obj_raw); @@ -87,7 +107,7 @@ impl<'a> ParserContext<'a> { AlterTableOperation::RenameTable { new_table_name } } else { return Err(ParserError::ParserError(format!( - "expect keyword ADD or DROP or RENAME after ALTER TABLE, found {}", + "expect keyword ADD or DROP or ALERT COLUMN or RENAME after ALTER TABLE, found {}", parser.peek_token() ))); }; @@ -253,6 +273,48 @@ mod tests { } } + #[test] + fn test_parse_alter_change_column_type() { + let sql = "ALTER TABLE my_metric_1 ALTER COLUMN a STRING"; + let result = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap_err(); + + let err = result.output_msg(); + assert!(err.contains("expect keyword TYPE after ALTER TABLE ALTER COLUMN a, found STRING")); + + let sql = "ALTER TABLE my_metric_1 ALTER COLUMN a TYPE STRING"; + 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!("my_metric_1", alter_table.table_name().0[0].value); + + let alter_operation = alter_table.alter_operation(); + assert_matches!( + alter_operation, + AlterTableOperation::ChangeColumnType { .. } + ); + match alter_operation { + AlterTableOperation::ChangeColumnType { + column_name, + target_type, + } => { + assert_eq!("a", column_name.value); + assert_eq!(DataType::String(None), *target_type); + } + _ => unreachable!(), + } + } + _ => unreachable!(), + } + } + #[test] fn test_parse_alter_rename_table() { let sql = "ALTER TABLE test_table table_t"; @@ -260,7 +322,9 @@ mod tests { ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) .unwrap_err(); let err = result.output_msg(); - assert!(err.contains("expect keyword ADD or DROP or RENAME after ALTER TABLE")); + assert!( + err.contains("expect keyword ADD or DROP or ALERT COLUMN or RENAME after ALTER TABLE") + ); let sql = "ALTER TABLE test_table RENAME table_t"; let mut result = diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index a54ba2d41b74..b2aee19481eb 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -15,7 +15,7 @@ use std::fmt::{Debug, Display}; use common_query::AddColumnLocation; -use sqlparser::ast::{ColumnDef, Ident, ObjectName, TableConstraint}; +use sqlparser::ast::{ColumnDef, DataType, Ident, ObjectName, TableConstraint}; use sqlparser_derive::{Visit, VisitMut}; #[derive(Debug, Clone, PartialEq, Eq, Visit, VisitMut)] @@ -58,6 +58,11 @@ pub enum AlterTableOperation { column_def: ColumnDef, location: Option, }, + /// `ALTER COLUMN TYPE [target_type]` + ChangeColumnType { + column_name: Ident, + target_type: DataType, + }, /// `DROP COLUMN ` DropColumn { name: Ident }, /// `RENAME ` @@ -82,6 +87,12 @@ impl Display for AlterTableOperation { AlterTableOperation::RenameTable { new_table_name } => { write!(f, r#"RENAME {new_table_name}"#) } + AlterTableOperation::ChangeColumnType { + column_name, + target_type, + } => { + write!(f, r#"ALTER COLUMN {column_name} TYPE {target_type}"#) + } } } } @@ -117,6 +128,27 @@ ALTER TABLE monitor ADD COLUMN app STRING DEFAULT 'shop' PRIMARY KEY"#, } } + let sql = r"alter table monitor alter column load_15 type string;"; + let stmts = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(1, stmts.len()); + assert_matches!(&stmts[0], Statement::Alter { .. }); + + match &stmts[0] { + Statement::Alter(set) => { + let new_sql = format!("\n{}", set); + assert_eq!( + r#" +ALTER TABLE monitor ALTER COLUMN load_15 TYPE STRING"#, + &new_sql + ); + } + _ => { + unreachable!(); + } + } + let sql = r"alter table monitor drop column load_15;"; let stmts = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 58210e45c2b7..866b929a9d71 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -15,6 +15,7 @@ use std::collections::HashMap; use std::fmt::{self}; +use api::helper::ColumnDataTypeWrapper; use api::v1::add_column_location::LocationType; use api::v1::region::{ alter_request, region_request, AlterRequest, AlterRequests, CloseRequest, CompactRequest, @@ -457,6 +458,14 @@ impl TryFrom for AlterKind { .collect::>>()?; AlterKind::AddColumns { columns } } + alter_request::Kind::ChangeColumnTypes(x) => { + let columns = x + .change_column_types + .into_iter() + .map(|x| x.try_into()) + .collect::>>()?; + AlterKind::ChangeColumnTypes { columns } + } alter_request::Kind::DropColumns(x) => { let names = x.drop_columns.into_iter().map(|x| x.name).collect(); AlterKind::DropColumns { names } @@ -615,6 +624,23 @@ impl ChangeColumnType { } } +impl TryFrom for ChangeColumnType { + type Error = MetadataError; + + fn try_from(change_column_type: v1::region::ChangeColumnType) -> Result { + let target_type = ColumnDataTypeWrapper::new( + change_column_type.target_type(), + change_column_type.target_type_extension, + ) + .into(); + + Ok(ChangeColumnType { + column_name: change_column_type.column_name, + target_type, + }) + } +} + #[derive(Debug, Default)] pub struct RegionFlushRequest { pub row_group_size: Option, diff --git a/tests/cases/standalone/common/alter/change_col_type.result b/tests/cases/standalone/common/alter/change_col_type.result new file mode 100644 index 000000000000..57eb65ff4b63 --- /dev/null +++ b/tests/cases/standalone/common/alter/change_col_type.result @@ -0,0 +1,91 @@ +CREATE TABLE test(id INTEGER PRIMARY KEY, i INTEGER NULL, j TIMESTAMP TIME INDEX, k BOOLEAN); + +Affected Rows: 0 + +INSERT INTO test VALUES (1, 1, 1, false), (2, 2, 2, true); + +Affected Rows: 2 + +ALTER TABLE test ALTER COLUMN "I" TYPE STRING; + +Error: 4002(TableColumnNotFound), Column I not exists in table test + +ALTER TABLE test ALTER COLUMN k TYPE DATE; + +Error: 1004(InvalidArguments), Invalid alter table(test) request: column 'k' cannot be cast automatically to type 'Date' + +ALTER TABLE test ALTER COLUMN id TYPE STRING; + +Error: 1004(InvalidArguments), Invalid alter table(test) request: Not allowed to change primary key index column 'id' + +ALTER TABLE test ALTER COLUMN j TYPE STRING; + +Error: 1004(InvalidArguments), Invalid alter table(test) request: Not allowed to change timestamp index column 'j' datatype + +ALTER TABLE test ALTER COLUMN I TYPE STRING; + +Affected Rows: 0 + +SELECT * FROM test; + ++----+---+-------------------------+-------+ +| id | i | j | k | ++----+---+-------------------------+-------+ +| 1 | 1 | 1970-01-01T00:00:00.001 | false | +| 2 | 2 | 1970-01-01T00:00:00.002 | true | ++----+---+-------------------------+-------+ + +INSERT INTO test VALUES (3, "greptime", 3, true); + +Affected Rows: 1 + +SELECT * FROM test; + ++----+----------+-------------------------+-------+ +| id | i | j | k | ++----+----------+-------------------------+-------+ +| 1 | 1 | 1970-01-01T00:00:00.001 | false | +| 2 | 2 | 1970-01-01T00:00:00.002 | true | +| 3 | greptime | 1970-01-01T00:00:00.003 | true | ++----+----------+-------------------------+-------+ + +DESCRIBE test; + ++--------+----------------------+-----+------+---------+---------------+ +| Column | Type | Key | Null | Default | Semantic Type | ++--------+----------------------+-----+------+---------+---------------+ +| id | Int32 | PRI | YES | | TAG | +| i | String | | YES | | FIELD | +| j | TimestampMillisecond | PRI | NO | | TIMESTAMP | +| k | Boolean | | YES | | FIELD | ++--------+----------------------+-----+------+---------+---------------+ + +ALTER TABLE test ALTER COLUMN I TYPE INTEGER; + +Affected Rows: 0 + +SELECT * FROM test; + ++----+---+-------------------------+-------+ +| id | i | j | k | ++----+---+-------------------------+-------+ +| 1 | 1 | 1970-01-01T00:00:00.001 | false | +| 2 | 2 | 1970-01-01T00:00:00.002 | true | +| 3 | | 1970-01-01T00:00:00.003 | true | ++----+---+-------------------------+-------+ + +DESCRIBE test; + ++--------+----------------------+-----+------+---------+---------------+ +| Column | Type | Key | Null | Default | Semantic Type | ++--------+----------------------+-----+------+---------+---------------+ +| id | Int32 | PRI | YES | | TAG | +| i | Int32 | | YES | | FIELD | +| j | TimestampMillisecond | PRI | NO | | TIMESTAMP | +| k | Boolean | | YES | | FIELD | ++--------+----------------------+-----+------+---------+---------------+ + +DROP TABLE test; + +Affected Rows: 0 + diff --git a/tests/cases/standalone/common/alter/change_col_type.sql b/tests/cases/standalone/common/alter/change_col_type.sql new file mode 100644 index 000000000000..cf7741892542 --- /dev/null +++ b/tests/cases/standalone/common/alter/change_col_type.sql @@ -0,0 +1,29 @@ +CREATE TABLE test(id INTEGER PRIMARY KEY, i INTEGER NULL, j TIMESTAMP TIME INDEX, k BOOLEAN); + +INSERT INTO test VALUES (1, 1, 1, false), (2, 2, 2, true); + +ALTER TABLE test ALTER COLUMN "I" TYPE STRING; + +ALTER TABLE test ALTER COLUMN k TYPE DATE; + +ALTER TABLE test ALTER COLUMN id TYPE STRING; + +ALTER TABLE test ALTER COLUMN j TYPE STRING; + +ALTER TABLE test ALTER COLUMN I TYPE STRING; + +SELECT * FROM test; + +INSERT INTO test VALUES (3, "greptime", 3, true); + +SELECT * FROM test; + +DESCRIBE test; + +ALTER TABLE test ALTER COLUMN I TYPE INTEGER; + +SELECT * FROM test; + +DESCRIBE test; + +DROP TABLE test; diff --git a/tests/cases/standalone/common/alter/change_col_type_not_null.result b/tests/cases/standalone/common/alter/change_col_type_not_null.result new file mode 100644 index 000000000000..71ac5a3ee85c --- /dev/null +++ b/tests/cases/standalone/common/alter/change_col_type_not_null.result @@ -0,0 +1,43 @@ +CREATE TABLE test(i TIMESTAMP TIME INDEX, j INTEGER NOT NULL); + +Affected Rows: 0 + +INSERT INTO test VALUES (1, 1), (2, 2); + +Affected Rows: 2 + +SELECT * FROM test; + ++-------------------------+---+ +| i | j | ++-------------------------+---+ +| 1970-01-01T00:00:00.001 | 1 | +| 1970-01-01T00:00:00.002 | 2 | ++-------------------------+---+ + +ALTER TABLE test ALTER COLUMN j TYPE STRING; + +Error: 1004(InvalidArguments), Invalid alter table(test) request: column 'j' must be nullable to ensure safe conversion. + +SELECT * FROM test; + ++-------------------------+---+ +| i | j | ++-------------------------+---+ +| 1970-01-01T00:00:00.001 | 1 | +| 1970-01-01T00:00:00.002 | 2 | ++-------------------------+---+ + +DESCRIBE test; + ++--------+----------------------+-----+------+---------+---------------+ +| Column | Type | Key | Null | Default | Semantic Type | ++--------+----------------------+-----+------+---------+---------------+ +| i | TimestampMillisecond | PRI | NO | | TIMESTAMP | +| j | Int32 | | NO | | FIELD | ++--------+----------------------+-----+------+---------+---------------+ + +DROP TABLE test; + +Affected Rows: 0 + diff --git a/tests/cases/standalone/common/alter/change_col_type_not_null.sql b/tests/cases/standalone/common/alter/change_col_type_not_null.sql new file mode 100644 index 000000000000..996867134f9b --- /dev/null +++ b/tests/cases/standalone/common/alter/change_col_type_not_null.sql @@ -0,0 +1,13 @@ +CREATE TABLE test(i TIMESTAMP TIME INDEX, j INTEGER NOT NULL); + +INSERT INTO test VALUES (1, 1), (2, 2); + +SELECT * FROM test; + +ALTER TABLE test ALTER COLUMN j TYPE STRING; + +SELECT * FROM test; + +DESCRIBE test; + +DROP TABLE test; From 2b434702880b8a9f46e20ccbe393cf7d54f69356 Mon Sep 17 00:00:00 2001 From: kould Date: Wed, 24 Apr 2024 18:55:45 +0800 Subject: [PATCH 02/15] fix: test `test_parse_alter_change_column_type` --- src/sql/src/parsers/alter_parser.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index e16ccaabf898..9caba1b187a4 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -281,7 +281,7 @@ mod tests { .unwrap_err(); let err = result.output_msg(); - assert!(err.contains("expect keyword TYPE after ALTER TABLE ALTER COLUMN a, found STRING")); + assert!(err.contains("expect keyword TYPE or TO after ALTER TABLE ALTER COLUMN a, found STRING")); let sql = "ALTER TABLE my_metric_1 ALTER COLUMN a TYPE STRING"; let mut result = From 93ed92076ad292a607f07e0add96790135a86b67 Mon Sep 17 00:00:00 2001 From: kould Date: Wed, 24 Apr 2024 19:07:58 +0800 Subject: [PATCH 03/15] style: code fmt --- src/sql/src/parsers/alter_parser.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 9caba1b187a4..db3f8ccaf9c0 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -281,7 +281,8 @@ mod tests { .unwrap_err(); let err = result.output_msg(); - assert!(err.contains("expect keyword TYPE or TO after ALTER TABLE ALTER COLUMN a, found STRING")); + assert!(err + .contains("expect keyword TYPE or TO after ALTER TABLE ALTER COLUMN a, found STRING")); let sql = "ALTER TABLE my_metric_1 ALTER COLUMN a TYPE STRING"; let mut result = From 98c828143368dd9053c286a7facb5bfbb0821d9f Mon Sep 17 00:00:00 2001 From: kould Date: Wed, 24 Apr 2024 23:23:44 +0800 Subject: [PATCH 04/15] style: move to new test: `test_make_alter_column_type_region_request` --- .../src/ddl/alter_table/region_request.rs | 244 +++++++++++------- src/store-api/src/region_request.rs | 14 +- 2 files changed, 152 insertions(+), 106 deletions(-) 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 8ebc62c49bdc..21dafa2696be 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -211,14 +211,48 @@ mod tests { ) .await .unwrap(); - { - let add_column_task = AlterTableTask { - alter_table: AlterExpr { - catalog_name: DEFAULT_CATALOG_NAME.to_string(), - schema_name: DEFAULT_SCHEMA_NAME.to_string(), - table_name: table_name.to_string(), - kind: Some(Kind::AddColumns(AddColumns { - add_columns: vec![AddColumn { + + let task = AlterTableTask { + alter_table: AlterExpr { + catalog_name: DEFAULT_CATALOG_NAME.to_string(), + schema_name: DEFAULT_SCHEMA_NAME.to_string(), + table_name: table_name.to_string(), + kind: Some(Kind::AddColumns(AddColumns { + add_columns: vec![AddColumn { + column_def: Some(PbColumnDef { + name: "my_tag3".to_string(), + data_type: ColumnDataType::String as i32, + is_nullable: true, + default_constraint: b"hello".to_vec(), + semantic_type: SemanticType::Tag as i32, + comment: String::new(), + ..Default::default() + }), + location: Some(AddColumnLocation { + location_type: LocationType::After as i32, + after_column_name: "my_tag2".to_string(), + }), + }], + })), + }, + }; + + let mut procedure = + AlterTableProcedure::new(cluster_id, table_id, task, ddl_context).unwrap(); + procedure.on_prepare().await.unwrap(); + let Some(Body::Alter(alter_region_request)) = + procedure.make_alter_region_request(region_id).unwrap().body + else { + unreachable!() + }; + assert_eq!(alter_region_request.region_id, region_id.as_u64()); + assert_eq!(alter_region_request.schema_version, 1); + assert_eq!( + alter_region_request.kind, + Some(region::alter_request::Kind::AddColumns( + region::AddColumns { + add_columns: vec![region::AddColumn { + column_def: Some(RegionColumnDef { column_def: Some(PbColumnDef { name: "my_tag3".to_string(), data_type: ColumnDataType::String as i32, @@ -228,99 +262,113 @@ mod tests { comment: String::new(), ..Default::default() }), - location: Some(AddColumnLocation { - location_type: LocationType::After as i32, - after_column_name: "my_tag2".to_string(), - }), - }], - })), - }, - }; + column_id: 3, + }), + location: Some(AddColumnLocation { + location_type: LocationType::After as i32, + after_column_name: "my_tag2".to_string(), + }), + }] + } + )) + ); + } - let mut procedure = AlterTableProcedure::new( - cluster_id, - table_id, - add_column_task, - ddl_context.clone(), - ) - .unwrap(); - procedure.on_prepare().await.unwrap(); - let Some(Body::Alter(alter_region_request)) = - procedure.make_alter_region_request(region_id).unwrap().body - else { - unreachable!() - }; - assert_eq!(alter_region_request.region_id, region_id.as_u64()); - assert_eq!(alter_region_request.schema_version, 1); - assert_eq!( - alter_region_request.kind, - Some(region::alter_request::Kind::AddColumns( - region::AddColumns { - add_columns: vec![region::AddColumn { - column_def: Some(RegionColumnDef { - column_def: Some(PbColumnDef { - name: "my_tag3".to_string(), - data_type: ColumnDataType::String as i32, - is_nullable: true, - default_constraint: b"hello".to_vec(), - semantic_type: SemanticType::Tag as i32, - comment: String::new(), - ..Default::default() - }), - column_id: 3, - }), - location: Some(AddColumnLocation { - location_type: LocationType::After as i32, - after_column_name: "my_tag2".to_string(), - }), - }] - } - )) - ); - } - { - let change_column_type_task = AlterTableTask { - alter_table: AlterExpr { - catalog_name: DEFAULT_CATALOG_NAME.to_string(), - schema_name: DEFAULT_SCHEMA_NAME.to_string(), - table_name: table_name.to_string(), - kind: Some(Kind::ChangeColumnTypes(ChangeColumnTypes { - change_column_types: vec![ChangeColumnType { - column_name: "cpu".to_string(), - target_type: ColumnDataType::String as i32, - target_type_extension: None, - }], - })), - }, - }; + #[tokio::test] + async fn test_make_alter_column_type_region_request() { + let datanode_manager = Arc::new(MockDatanodeManager::new(())); + let ddl_context = new_ddl_context(datanode_manager); + let cluster_id = 1; + let table_id = 1024; + let region_id = RegionId::new(table_id, 1); + let table_name = "foo"; + + let create_table = TestCreateTableExprBuilder::default() + .column_defs([ + TestColumnDefBuilder::default() + .name("ts") + .data_type(ColumnDataType::TimestampMillisecond) + .semantic_type(SemanticType::Timestamp) + .build() + .unwrap() + .into(), + TestColumnDefBuilder::default() + .name("host") + .data_type(ColumnDataType::String) + .semantic_type(SemanticType::Tag) + .build() + .unwrap() + .into(), + TestColumnDefBuilder::default() + .name("cpu") + .data_type(ColumnDataType::Float64) + .semantic_type(SemanticType::Field) + .build() + .unwrap() + .into(), + ]) + .table_id(table_id) + .time_index("ts") + .primary_keys(["host".into()]) + .table_name(table_name) + .build() + .unwrap() + .into(); + let table_info = build_raw_table_info_from_expr(&create_table); - let mut procedure = AlterTableProcedure::new( - cluster_id, - table_id, - change_column_type_task, - ddl_context, + // Puts a value to table name key. + ddl_context + .table_metadata_manager + .create_table_metadata( + table_info, + TableRouteValue::physical(vec![RegionRoute { + region: Region::new_test(region_id), + leader_peer: Some(Peer::empty(1)), + follower_peers: vec![], + leader_status: None, + leader_down_since: None, + }]), + HashMap::new(), ) + .await .unwrap(); - procedure.on_prepare().await.unwrap(); - let Some(Body::Alter(alter_region_request)) = - procedure.make_alter_region_request(region_id).unwrap().body - else { - unreachable!() - }; - assert_eq!(alter_region_request.region_id, region_id.as_u64()); - assert_eq!(alter_region_request.schema_version, 1); - assert_eq!( - alter_region_request.kind, - Some(region::alter_request::Kind::ChangeColumnTypes( - region::ChangeColumnTypes { - change_column_types: vec![region::ChangeColumnType { - column_name: "cpu".to_string(), - target_type: ColumnDataType::String as i32, - target_type_extension: None, - }] - } - )) - ); - } + + let task = AlterTableTask { + alter_table: AlterExpr { + catalog_name: DEFAULT_CATALOG_NAME.to_string(), + schema_name: DEFAULT_SCHEMA_NAME.to_string(), + table_name: table_name.to_string(), + kind: Some(Kind::ChangeColumnTypes(ChangeColumnTypes { + change_column_types: vec![ChangeColumnType { + column_name: "cpu".to_string(), + target_type: ColumnDataType::String as i32, + target_type_extension: None, + }], + })), + }, + }; + + let mut procedure = + AlterTableProcedure::new(cluster_id, table_id, task, ddl_context).unwrap(); + procedure.on_prepare().await.unwrap(); + let Some(Body::Alter(alter_region_request)) = + procedure.make_alter_region_request(region_id).unwrap().body + else { + unreachable!() + }; + assert_eq!(alter_region_request.region_id, region_id.as_u64()); + assert_eq!(alter_region_request.schema_version, 1); + assert_eq!( + alter_region_request.kind, + Some(region::alter_request::Kind::ChangeColumnTypes( + region::ChangeColumnTypes { + change_column_types: vec![region::ChangeColumnType { + column_name: "cpu".to_string(), + target_type: ColumnDataType::String as i32, + target_type_extension: None, + }] + } + )) + ); } } diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 866b929a9d71..c791baf5f5f3 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -462,8 +462,8 @@ impl TryFrom for AlterKind { let columns = x .change_column_types .into_iter() - .map(|x| x.try_into()) - .collect::>>()?; + .map(|x| x.into()) + .collect::>(); AlterKind::ChangeColumnTypes { columns } } alter_request::Kind::DropColumns(x) => { @@ -624,20 +624,18 @@ impl ChangeColumnType { } } -impl TryFrom for ChangeColumnType { - type Error = MetadataError; - - fn try_from(change_column_type: v1::region::ChangeColumnType) -> Result { +impl From for ChangeColumnType { + fn from(change_column_type: v1::region::ChangeColumnType) -> Self { let target_type = ColumnDataTypeWrapper::new( change_column_type.target_type(), change_column_type.target_type_extension, ) .into(); - Ok(ChangeColumnType { + ChangeColumnType { column_name: change_column_type.column_name, target_type, - }) + } } } From ce8efbf9aba7217ffeb7f01e21afdc723355c747 Mon Sep 17 00:00:00 2001 From: kould Date: Thu, 25 Apr 2024 15:23:36 +0800 Subject: [PATCH 05/15] style: simplify the code --- .../src/ddl/alter_table/region_request.rs | 85 +++++-------------- 1 file changed, 22 insertions(+), 63 deletions(-) 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 21dafa2696be..0281a9f8495f 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -140,23 +140,23 @@ mod tests { ChangeColumnTypes, ColumnDataType, ColumnDef as PbColumnDef, SemanticType, }; use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME}; - use store_api::storage::RegionId; + use store_api::storage::{RegionId, TableId}; use crate::ddl::alter_table::AlterTableProcedure; use crate::ddl::test_util::columns::TestColumnDefBuilder; use crate::ddl::test_util::create_table::{ build_raw_table_info_from_expr, TestCreateTableExprBuilder, }; + use crate::ddl::DdlContext; use crate::key::table_route::TableRouteValue; use crate::peer::Peer; use crate::rpc::ddl::AlterTableTask; use crate::rpc::router::{Region, RegionRoute}; use crate::test_util::{new_ddl_context, MockDatanodeManager}; - #[tokio::test] - async fn test_make_alter_region_request() { - let node_manager = Arc::new(MockDatanodeManager::new(())); - let ddl_context = new_ddl_context(node_manager); + async fn prepare_ddl_context() -> (DdlContext, u64, TableId, RegionId, String) { + let datanode_manager = Arc::new(MockDatanodeManager::new(())); + let ddl_context = new_ddl_context(datanode_manager); let cluster_id = 1; let table_id = 1024; let region_id = RegionId::new(table_id, 1); @@ -211,12 +211,25 @@ mod tests { ) .await .unwrap(); + ( + ddl_context, + cluster_id, + table_id, + region_id, + table_name.to_string(), + ) + } + + #[tokio::test] + async fn test_make_alter_region_request() { + let (ddl_context, cluster_id, table_id, region_id, table_name) = + prepare_ddl_context().await; let task = AlterTableTask { alter_table: AlterExpr { catalog_name: DEFAULT_CATALOG_NAME.to_string(), schema_name: DEFAULT_SCHEMA_NAME.to_string(), - table_name: table_name.to_string(), + table_name, kind: Some(Kind::AddColumns(AddColumns { add_columns: vec![AddColumn { column_def: Some(PbColumnDef { @@ -276,68 +289,14 @@ mod tests { #[tokio::test] async fn test_make_alter_column_type_region_request() { - let datanode_manager = Arc::new(MockDatanodeManager::new(())); - let ddl_context = new_ddl_context(datanode_manager); - let cluster_id = 1; - let table_id = 1024; - let region_id = RegionId::new(table_id, 1); - let table_name = "foo"; - - let create_table = TestCreateTableExprBuilder::default() - .column_defs([ - TestColumnDefBuilder::default() - .name("ts") - .data_type(ColumnDataType::TimestampMillisecond) - .semantic_type(SemanticType::Timestamp) - .build() - .unwrap() - .into(), - TestColumnDefBuilder::default() - .name("host") - .data_type(ColumnDataType::String) - .semantic_type(SemanticType::Tag) - .build() - .unwrap() - .into(), - TestColumnDefBuilder::default() - .name("cpu") - .data_type(ColumnDataType::Float64) - .semantic_type(SemanticType::Field) - .build() - .unwrap() - .into(), - ]) - .table_id(table_id) - .time_index("ts") - .primary_keys(["host".into()]) - .table_name(table_name) - .build() - .unwrap() - .into(); - let table_info = build_raw_table_info_from_expr(&create_table); - - // Puts a value to table name key. - ddl_context - .table_metadata_manager - .create_table_metadata( - table_info, - TableRouteValue::physical(vec![RegionRoute { - region: Region::new_test(region_id), - leader_peer: Some(Peer::empty(1)), - follower_peers: vec![], - leader_status: None, - leader_down_since: None, - }]), - HashMap::new(), - ) - .await - .unwrap(); + let (ddl_context, cluster_id, table_id, region_id, table_name) = + prepare_ddl_context().await; let task = AlterTableTask { alter_table: AlterExpr { catalog_name: DEFAULT_CATALOG_NAME.to_string(), schema_name: DEFAULT_SCHEMA_NAME.to_string(), - table_name: table_name.to_string(), + table_name, kind: Some(Kind::ChangeColumnTypes(ChangeColumnTypes { change_column_types: vec![ChangeColumnType { column_name: "cpu".to_string(), From 14500f2c702a1a712edb19a77d6634719779b78d Mon Sep 17 00:00:00 2001 From: kould Date: Thu, 25 Apr 2024 18:45:07 +0800 Subject: [PATCH 06/15] style: remove `v1::region::ChangeColumnType` --- src/common/meta/src/ddl/alter_table/region_request.rs | 5 +++-- src/store-api/src/region_request.rs | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) 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 0281a9f8495f..cbb4a166cb0b 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -15,9 +15,10 @@ use api::v1::alter_expr::Kind; use api::v1::region::region_request::Body; use api::v1::region::{ - alter_request, AddColumn, AddColumns, AlterRequest, ChangeColumnType, ChangeColumnTypes, - DropColumn, DropColumns, RegionColumnDef, RegionRequest, RegionRequestHeader, + alter_request, AddColumn, AddColumns, AlterRequest, DropColumn, DropColumns, RegionColumnDef, + RegionRequest, RegionRequestHeader, }; +use api::v1::{ChangeColumnType, ChangeColumnTypes}; use common_telemetry::tracing_context::TracingContext; use snafu::OptionExt; use store_api::storage::RegionId; diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index c791baf5f5f3..a2e722080c4a 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -624,8 +624,8 @@ impl ChangeColumnType { } } -impl From for ChangeColumnType { - fn from(change_column_type: v1::region::ChangeColumnType) -> Self { +impl From for ChangeColumnType { + fn from(change_column_type: v1::ChangeColumnType) -> Self { let target_type = ColumnDataTypeWrapper::new( change_column_type.target_type(), change_column_type.target_type_extension, From 88dd0cf2866d77d1c1ee14b16ad99d05a58d893a Mon Sep 17 00:00:00 2001 From: kould Date: Thu, 25 Apr 2024 23:22:06 +0800 Subject: [PATCH 07/15] resolve conflicts --- src/operator/src/expr_factory.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 200e1dba0b78..3d90b0c341ca 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -38,7 +38,6 @@ use sql::statements::create::{CreateExternalTable, CreateTable, TIME_INDEX}; use sql::statements::{ column_def_to_schema, sql_column_def_to_grpc_column_def, sql_data_type_to_concrete_data_type, }; -use sql::util::to_lowercase_options_map; use table::requests::{TableOptions, FILE_TABLE_META_KEY}; use table::table_reference::TableReference; From b192f912f738328d63d2eb04bebba6101bbf8a6b Mon Sep 17 00:00:00 2001 From: kould Date: Thu, 25 Apr 2024 23:32:54 +0800 Subject: [PATCH 08/15] fix: test `test_make_alter_column_type_region_request` --- src/common/meta/src/ddl/alter_table/region_request.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 cbb4a166cb0b..95bf600ed123 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -321,8 +321,8 @@ mod tests { assert_eq!( alter_region_request.kind, Some(region::alter_request::Kind::ChangeColumnTypes( - region::ChangeColumnTypes { - change_column_types: vec![region::ChangeColumnType { + ChangeColumnTypes { + change_column_types: vec![ChangeColumnType { column_name: "cpu".to_string(), target_type: ColumnDataType::String as i32, target_type_extension: None, From d52db6785926abd9ac2694649252d7629b788b94 Mon Sep 17 00:00:00 2001 From: kould Date: Fri, 26 Apr 2024 15:34:48 +0800 Subject: [PATCH 09/15] style: simplify the code --- .../src/ddl/alter_table/region_request.rs | 19 +------------------ src/store-api/src/region_request.rs | 2 +- 2 files changed, 2 insertions(+), 19 deletions(-) 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 95bf600ed123..1dfa808463f3 100644 --- a/src/common/meta/src/ddl/alter_table/region_request.rs +++ b/src/common/meta/src/ddl/alter_table/region_request.rs @@ -18,7 +18,6 @@ use api::v1::region::{ alter_request, AddColumn, AddColumns, AlterRequest, DropColumn, DropColumns, RegionColumnDef, RegionRequest, RegionRequestHeader, }; -use api::v1::{ChangeColumnType, ChangeColumnTypes}; use common_telemetry::tracing_context::TracingContext; use snafu::OptionExt; use store_api::storage::RegionId; @@ -92,23 +91,7 @@ fn create_proto_alter_kind( add_columns, }))) } - Kind::ChangeColumnTypes(x) => { - let change_column_types = x - .change_column_types - .iter() - .map(|change_column_type| ChangeColumnType { - column_name: change_column_type.column_name.clone(), - target_type: change_column_type.target_type, - target_type_extension: change_column_type.target_type_extension.clone(), - }) - .collect(); - - Ok(Some(alter_request::Kind::ChangeColumnTypes( - ChangeColumnTypes { - change_column_types, - }, - ))) - } + Kind::ChangeColumnTypes(x) => Ok(Some(alter_request::Kind::ChangeColumnTypes(x.clone()))), Kind::DropColumns(x) => { let drop_columns = x .drop_columns diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index a2e722080c4a..6c788be1e5eb 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -13,7 +13,7 @@ // limitations under the License. use std::collections::HashMap; -use std::fmt::{self}; +use std::fmt; use api::helper::ColumnDataTypeWrapper; use api::v1::add_column_location::LocationType; From 1fcd3b6b28087554e36b3c77225fb85ef0aa805e Mon Sep 17 00:00:00 2001 From: kould Date: Fri, 26 Apr 2024 22:14:16 +0800 Subject: [PATCH 10/15] rebase --- src/store-api/src/region_request.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index 6c788be1e5eb..6c4aaa28f47c 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -470,9 +470,6 @@ impl TryFrom for AlterKind { let names = x.drop_columns.into_iter().map(|x| x.name).collect(); AlterKind::DropColumns { names } } - alter_request::Kind::ChangeColumnTypes(_) => { - unimplemented!() - } }; Ok(alter_kind) From a7f1ed62d94492e8d0bd68b9fb06f7b8c39ca850 Mon Sep 17 00:00:00 2001 From: kould Date: Fri, 26 Apr 2024 22:22:15 +0800 Subject: [PATCH 11/15] rebase --- src/common/grpc-expr/src/alter.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/common/grpc-expr/src/alter.rs b/src/common/grpc-expr/src/alter.rs index baf1c4529c05..ac49069412cd 100644 --- a/src/common/grpc-expr/src/alter.rs +++ b/src/common/grpc-expr/src/alter.rs @@ -92,7 +92,6 @@ pub fn alter_expr_to_request(table_id: TableId, expr: AlterExpr) -> Result { AlterKind::RenameTable { new_table_name } } - Kind::ChangeColumnTypes(_) => unimplemented!(), }; let request = AlterTableRequest { From b938f905d5c25e99df82c607b406b2ef1f92ac86 Mon Sep 17 00:00:00 2001 From: kould Date: Fri, 26 Apr 2024 22:57:57 +0800 Subject: [PATCH 12/15] rebase --- src/common/meta/src/ddl/alter_table/region_request.rs | 1 - 1 file changed, 1 deletion(-) 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 1dfa808463f3..b4223b8ea05d 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,6 @@ fn create_proto_alter_kind( }))) } Kind::RenameTable(_) => Ok(None), - Kind::ChangeColumnTypes(_) => unimplemented!(), } } From 83efb1cfbcbcb932ca01336ee357a5aba29606c6 Mon Sep 17 00:00:00 2001 From: kould Date: Sun, 28 Apr 2024 14:12:11 +0800 Subject: [PATCH 13/15] fix: `ALTER COLUMN ... TYPE` -> `MODIFY COLUMN` --- src/operator/src/expr_factory.rs | 2 +- src/sql/src/parser.rs | 16 ++++- src/sql/src/parsers/alter_parser.rs | 64 ++++++++----------- src/sql/src/statements/alter.rs | 8 +-- .../common/alter/change_col_type.result | 12 ++-- .../common/alter/change_col_type.sql | 12 ++-- .../alter/change_col_type_not_null.result | 2 +- .../common/alter/change_col_type_not_null.sql | 2 +- 8 files changed, 59 insertions(+), 59 deletions(-) diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 3d90b0c341ca..07cc9338a3cc 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -662,7 +662,7 @@ mod tests { #[test] fn test_to_alter_change_column_type_expr() { - let sql = "ALTER TABLE monitor alter column mem_usage TYPE STRING;"; + let sql = "ALTER TABLE monitor MODIFY mem_usage STRING;"; let stmt = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) .unwrap() diff --git a/src/sql/src/parser.rs b/src/sql/src/parser.rs index e873343bbdcb..f342a486191e 100644 --- a/src/sql/src/parser.rs +++ b/src/sql/src/parser.rs @@ -187,8 +187,13 @@ impl<'a> ParserContext<'a> { } pub fn consume_token(&mut self, expected: &str) -> bool { - if self.peek_token_as_string().to_uppercase() == *expected.to_uppercase() { - let _ = self.parser.next_token(); + Self::inner_consume_token(&mut self.parser, expected) + } + + #[inline] + pub(crate) fn inner_consume_token(parser: &mut Parser<'a>, expected: &str) -> bool { + if Self::inner_peek_token_as_string(parser).to_uppercase() == *expected.to_uppercase() { + let _ = parser.next_token(); true } else { false @@ -197,7 +202,12 @@ impl<'a> ParserContext<'a> { #[inline] pub(crate) fn peek_token_as_string(&self) -> String { - self.parser.peek_token().to_string() + Self::inner_peek_token_as_string(&self.parser) + } + + #[inline] + pub(crate) fn inner_peek_token_as_string(parser: &Parser<'a>) -> String { + parser.peek_token().to_string() } /// Canonicalize the identifier to lowercase if it's not quoted. diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index db3f8ccaf9c0..142806696036 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -73,25 +73,14 @@ impl<'a> ParserContext<'a> { parser.peek_token() ))); } - } else if parser - .expect_keywords(&[Keyword::ALTER, Keyword::COLUMN]) - .is_ok() - { + } else if ParserContext::inner_consume_token(parser, "MODIFY") { + let _ = parser.parse_keyword(Keyword::COLUMN); let column_name = Self::canonicalize_identifier(parser.parse_identifier(false)?); + let target_type = parser.parse_data_type()?; - if parser.parse_keyword(Keyword::TYPE) { - let target_type = parser.parse_data_type()?; - - AlterTableOperation::ChangeColumnType { - column_name, - target_type, - } - } else { - return Err(ParserError::ParserError(format!( - "expect keyword TYPE or TO after ALTER TABLE ALTER COLUMN {}, found {}", - column_name, - parser.peek_token() - ))); + AlterTableOperation::ChangeColumnType { + column_name, + target_type, } } else if parser.parse_keyword(Keyword::RENAME) { let new_table_name_obj_raw = self.parse_object_name()?; @@ -107,7 +96,7 @@ impl<'a> ParserContext<'a> { AlterTableOperation::RenameTable { new_table_name } } else { return Err(ParserError::ParserError(format!( - "expect keyword ADD or DROP or ALERT COLUMN or RENAME after ALTER TABLE, found {}", + "expect keyword ADD or DROP or MODIFY or RENAME after ALTER TABLE, found {}", parser.peek_token() ))); }; @@ -275,22 +264,25 @@ mod tests { #[test] fn test_parse_alter_change_column_type() { - let sql = "ALTER TABLE my_metric_1 ALTER COLUMN a STRING"; - let result = - ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) - .unwrap_err(); - - let err = result.output_msg(); - assert!(err - .contains("expect keyword TYPE or TO after ALTER TABLE ALTER COLUMN a, found STRING")); - - let sql = "ALTER TABLE my_metric_1 ALTER COLUMN a TYPE STRING"; - let mut result = - ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) - .unwrap(); - assert_eq!(1, result.len()); - - let statement = result.remove(0); + let sql_1 = "ALTER TABLE my_metric_1 MODIFY COLUMN a STRING"; + let result_1 = ParserContext::create_with_dialect( + sql_1, + &GreptimeDbDialect {}, + ParseOptions::default(), + ) + .unwrap(); + + let sql_2 = "ALTER TABLE my_metric_1 MODIFY a STRING"; + let mut result_2 = ParserContext::create_with_dialect( + sql_2, + &GreptimeDbDialect {}, + ParseOptions::default(), + ) + .unwrap(); + assert_eq!(result_1, result_2); + assert_eq!(1, result_2.len()); + + let statement = result_2.remove(0); assert_matches!(statement, Statement::Alter { .. }); match statement { Statement::Alter(alter_table) => { @@ -323,9 +315,7 @@ mod tests { ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) .unwrap_err(); let err = result.output_msg(); - assert!( - err.contains("expect keyword ADD or DROP or ALERT COLUMN or RENAME after ALTER TABLE") - ); + assert!(err.contains("expect keyword ADD or DROP or MODIFY or RENAME after ALTER TABLE")); let sql = "ALTER TABLE test_table RENAME table_t"; let mut result = diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index b2aee19481eb..41a0997ecdb3 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -58,7 +58,7 @@ pub enum AlterTableOperation { column_def: ColumnDef, location: Option, }, - /// `ALTER COLUMN TYPE [target_type]` + /// `MODIFY [target_type]` ChangeColumnType { column_name: Ident, target_type: DataType, @@ -91,7 +91,7 @@ impl Display for AlterTableOperation { column_name, target_type, } => { - write!(f, r#"ALTER COLUMN {column_name} TYPE {target_type}"#) + write!(f, r#"MODIFY COLUMN {column_name} {target_type}"#) } } } @@ -128,7 +128,7 @@ ALTER TABLE monitor ADD COLUMN app STRING DEFAULT 'shop' PRIMARY KEY"#, } } - let sql = r"alter table monitor alter column load_15 type string;"; + let sql = r"alter table monitor modify column load_15 string;"; let stmts = ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) .unwrap(); @@ -140,7 +140,7 @@ ALTER TABLE monitor ADD COLUMN app STRING DEFAULT 'shop' PRIMARY KEY"#, let new_sql = format!("\n{}", set); assert_eq!( r#" -ALTER TABLE monitor ALTER COLUMN load_15 TYPE STRING"#, +ALTER TABLE monitor MODIFY COLUMN load_15 STRING"#, &new_sql ); } diff --git a/tests/cases/standalone/common/alter/change_col_type.result b/tests/cases/standalone/common/alter/change_col_type.result index 57eb65ff4b63..3d9500105a6d 100644 --- a/tests/cases/standalone/common/alter/change_col_type.result +++ b/tests/cases/standalone/common/alter/change_col_type.result @@ -6,23 +6,23 @@ INSERT INTO test VALUES (1, 1, 1, false), (2, 2, 2, true); Affected Rows: 2 -ALTER TABLE test ALTER COLUMN "I" TYPE STRING; +ALTER TABLE test MODIFY "I" STRING; Error: 4002(TableColumnNotFound), Column I not exists in table test -ALTER TABLE test ALTER COLUMN k TYPE DATE; +ALTER TABLE test MODIFY k DATE; Error: 1004(InvalidArguments), Invalid alter table(test) request: column 'k' cannot be cast automatically to type 'Date' -ALTER TABLE test ALTER COLUMN id TYPE STRING; +ALTER TABLE test MODIFY id STRING; Error: 1004(InvalidArguments), Invalid alter table(test) request: Not allowed to change primary key index column 'id' -ALTER TABLE test ALTER COLUMN j TYPE STRING; +ALTER TABLE test MODIFY j STRING; Error: 1004(InvalidArguments), Invalid alter table(test) request: Not allowed to change timestamp index column 'j' datatype -ALTER TABLE test ALTER COLUMN I TYPE STRING; +ALTER TABLE test MODIFY I STRING; Affected Rows: 0 @@ -60,7 +60,7 @@ DESCRIBE test; | k | Boolean | | YES | | FIELD | +--------+----------------------+-----+------+---------+---------------+ -ALTER TABLE test ALTER COLUMN I TYPE INTEGER; +ALTER TABLE test MODIFY I INTEGER; Affected Rows: 0 diff --git a/tests/cases/standalone/common/alter/change_col_type.sql b/tests/cases/standalone/common/alter/change_col_type.sql index cf7741892542..1eb95c719cdc 100644 --- a/tests/cases/standalone/common/alter/change_col_type.sql +++ b/tests/cases/standalone/common/alter/change_col_type.sql @@ -2,15 +2,15 @@ CREATE TABLE test(id INTEGER PRIMARY KEY, i INTEGER NULL, j TIMESTAMP TIME INDEX INSERT INTO test VALUES (1, 1, 1, false), (2, 2, 2, true); -ALTER TABLE test ALTER COLUMN "I" TYPE STRING; +ALTER TABLE test MODIFY "I" STRING; -ALTER TABLE test ALTER COLUMN k TYPE DATE; +ALTER TABLE test MODIFY k DATE; -ALTER TABLE test ALTER COLUMN id TYPE STRING; +ALTER TABLE test MODIFY id STRING; -ALTER TABLE test ALTER COLUMN j TYPE STRING; +ALTER TABLE test MODIFY j STRING; -ALTER TABLE test ALTER COLUMN I TYPE STRING; +ALTER TABLE test MODIFY I STRING; SELECT * FROM test; @@ -20,7 +20,7 @@ SELECT * FROM test; DESCRIBE test; -ALTER TABLE test ALTER COLUMN I TYPE INTEGER; +ALTER TABLE test MODIFY I INTEGER; SELECT * FROM test; diff --git a/tests/cases/standalone/common/alter/change_col_type_not_null.result b/tests/cases/standalone/common/alter/change_col_type_not_null.result index 71ac5a3ee85c..79f03c9cb023 100644 --- a/tests/cases/standalone/common/alter/change_col_type_not_null.result +++ b/tests/cases/standalone/common/alter/change_col_type_not_null.result @@ -15,7 +15,7 @@ SELECT * FROM test; | 1970-01-01T00:00:00.002 | 2 | +-------------------------+---+ -ALTER TABLE test ALTER COLUMN j TYPE STRING; +ALTER TABLE test MODIFY j STRING; Error: 1004(InvalidArguments), Invalid alter table(test) request: column 'j' must be nullable to ensure safe conversion. diff --git a/tests/cases/standalone/common/alter/change_col_type_not_null.sql b/tests/cases/standalone/common/alter/change_col_type_not_null.sql index 996867134f9b..c91ae44a2c14 100644 --- a/tests/cases/standalone/common/alter/change_col_type_not_null.sql +++ b/tests/cases/standalone/common/alter/change_col_type_not_null.sql @@ -4,7 +4,7 @@ INSERT INTO test VALUES (1, 1), (2, 2); SELECT * FROM test; -ALTER TABLE test ALTER COLUMN j TYPE STRING; +ALTER TABLE test MODIFY j STRING; SELECT * FROM test; From be18907317debdcfea13f833a75c4f92668aa153 Mon Sep 17 00:00:00 2001 From: kould <2435992353@qq.com> Date: Mon, 29 Apr 2024 18:31:32 +0800 Subject: [PATCH 14/15] fix: `parser` -> `self.parser` --- src/sql/src/parser.rs | 16 +++--------- src/sql/src/parsers/alter_parser.rs | 38 ++++++++++++++--------------- 2 files changed, 22 insertions(+), 32 deletions(-) diff --git a/src/sql/src/parser.rs b/src/sql/src/parser.rs index f342a486191e..e873343bbdcb 100644 --- a/src/sql/src/parser.rs +++ b/src/sql/src/parser.rs @@ -187,13 +187,8 @@ impl<'a> ParserContext<'a> { } pub fn consume_token(&mut self, expected: &str) -> bool { - Self::inner_consume_token(&mut self.parser, expected) - } - - #[inline] - pub(crate) fn inner_consume_token(parser: &mut Parser<'a>, expected: &str) -> bool { - if Self::inner_peek_token_as_string(parser).to_uppercase() == *expected.to_uppercase() { - let _ = parser.next_token(); + if self.peek_token_as_string().to_uppercase() == *expected.to_uppercase() { + let _ = self.parser.next_token(); true } else { false @@ -202,12 +197,7 @@ impl<'a> ParserContext<'a> { #[inline] pub(crate) fn peek_token_as_string(&self) -> String { - Self::inner_peek_token_as_string(&self.parser) - } - - #[inline] - pub(crate) fn inner_peek_token_as_string(parser: &Parser<'a>) -> String { - parser.peek_token().to_string() + self.parser.peek_token().to_string() } /// Canonicalize the identifier to lowercase if it's not quoted. diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 142806696036..cf3e1d0fb8ed 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -30,24 +30,24 @@ impl<'a> ParserContext<'a> { } fn parse_alter_table(&mut self) -> std::result::Result { - let parser = &mut self.parser; - parser.expect_keywords(&[Keyword::ALTER, Keyword::TABLE])?; + self.parser + .expect_keywords(&[Keyword::ALTER, Keyword::TABLE])?; - let raw_table_name = parser.parse_object_name(false)?; + let raw_table_name = self.parser.parse_object_name(false)?; let table_name = Self::canonicalize_object_name(raw_table_name); - let alter_operation = if parser.parse_keyword(Keyword::ADD) { - if let Some(constraint) = parser.parse_optional_table_constraint()? { + let alter_operation = if self.parser.parse_keyword(Keyword::ADD) { + if let Some(constraint) = self.parser.parse_optional_table_constraint()? { AlterTableOperation::AddConstraint(constraint) } else { - let _ = parser.parse_keyword(Keyword::COLUMN); - let mut column_def = parser.parse_column_def()?; + let _ = self.parser.parse_keyword(Keyword::COLUMN); + let mut column_def = self.parser.parse_column_def()?; column_def.name = Self::canonicalize_identifier(column_def.name); - let location = if parser.parse_keyword(Keyword::FIRST) { + let location = if self.parser.parse_keyword(Keyword::FIRST) { Some(AddColumnLocation::First) - } else if let Token::Word(word) = parser.peek_token().token { + } else if let Token::Word(word) = self.parser.peek_token().token { if word.value.to_ascii_uppercase() == "AFTER" { - let _ = parser.next_token(); + let _ = self.parser.next_token(); let name = Self::canonicalize_identifier(self.parse_identifier()?); Some(AddColumnLocation::After { column_name: name.value, @@ -63,26 +63,26 @@ impl<'a> ParserContext<'a> { location, } } - } else if parser.parse_keyword(Keyword::DROP) { - if parser.parse_keyword(Keyword::COLUMN) { + } else if self.parser.parse_keyword(Keyword::DROP) { + if self.parser.parse_keyword(Keyword::COLUMN) { let name = Self::canonicalize_identifier(self.parse_identifier()?); AlterTableOperation::DropColumn { name } } else { return Err(ParserError::ParserError(format!( "expect keyword COLUMN after ALTER TABLE DROP, found {}", - parser.peek_token() + self.parser.peek_token() ))); } - } else if ParserContext::inner_consume_token(parser, "MODIFY") { - let _ = parser.parse_keyword(Keyword::COLUMN); - let column_name = Self::canonicalize_identifier(parser.parse_identifier(false)?); - let target_type = parser.parse_data_type()?; + } 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, } - } else if parser.parse_keyword(Keyword::RENAME) { + } else if self.parser.parse_keyword(Keyword::RENAME) { let new_table_name_obj_raw = self.parse_object_name()?; let new_table_name_obj = Self::canonicalize_object_name(new_table_name_obj_raw); let new_table_name = match &new_table_name_obj.0[..] { @@ -97,7 +97,7 @@ impl<'a> ParserContext<'a> { } else { return Err(ParserError::ParserError(format!( "expect keyword ADD or DROP or MODIFY or RENAME after ALTER TABLE, found {}", - parser.peek_token() + self.parser.peek_token() ))); }; Ok(AlterTable::new(table_name, alter_operation)) From 2c8b92cc1aa86c35d17801a92276cc052f78d964 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Mon, 29 Apr 2024 20:31:34 +0800 Subject: [PATCH 15/15] Apply suggestions from code review --- src/operator/src/expr_factory.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index 0a85c8e80046..ff9531169cc2 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -18,8 +18,8 @@ use api::helper::ColumnDataTypeWrapper; use api::v1::alter_expr::Kind; use api::v1::{ AddColumn, AddColumns, AlterExpr, ChangeColumnType, ChangeColumnTypes, Column, ColumnDataType, - ColumnDataTypeExtension, CreateFlowTaskExpr, CreateTableExpr, DropColumn, DropColumns, RenameTable, - SemanticType, TableName, + ColumnDataTypeExtension, CreateFlowTaskExpr, CreateTableExpr, DropColumn, DropColumns, + RenameTable, SemanticType, TableName, }; use common_error::ext::BoxedError; use common_grpc_expr::util::ColumnExpr;