From 5da14d62a62fa1685f85d9c93a035c9799e2f6de Mon Sep 17 00:00:00 2001 From: Dennis Zhuang Date: Thu, 17 Nov 2022 17:47:47 +0800 Subject: [PATCH 1/5] feat: drop column for alter table --- src/api/greptime/v1/admin.proto | 9 +++++++ src/datanode/src/server/grpc/ddl.rs | 15 +++++++++++- src/datanode/src/sql/alter.rs | 3 +++ src/mito/src/table.rs | 2 +- src/sql/src/parsers/alter_parser.rs | 37 ++++++++++++++++++++++++++++- src/sql/src/statements/alter.rs | 14 ++++++++--- src/table/src/metadata.rs | 2 +- src/table/src/requests.rs | 2 +- 8 files changed, 76 insertions(+), 8 deletions(-) diff --git a/src/api/greptime/v1/admin.proto b/src/api/greptime/v1/admin.proto index 03e058523e91..9c2c95ecdfe4 100644 --- a/src/api/greptime/v1/admin.proto +++ b/src/api/greptime/v1/admin.proto @@ -51,6 +51,7 @@ message AlterExpr { string table_name = 3; oneof kind { AddColumns add_columns = 4; + DropColumns drop_columns = 5; } } @@ -58,11 +59,19 @@ message AddColumns { repeated AddColumn add_columns = 1; } +message DropColumns { + repeated DropColumn drop_columns = 1; +} + message AddColumn { ColumnDef column_def = 1; bool is_key = 2; } +message DropColumn { + string name = 1; +} + message CreateDatabaseExpr { //TODO(hl): maybe rename to schema_name? string database_name = 1; diff --git a/src/datanode/src/server/grpc/ddl.rs b/src/datanode/src/server/grpc/ddl.rs index 1a0a650cf1a2..90dc0e4a2b55 100644 --- a/src/datanode/src/server/grpc/ddl.rs +++ b/src/datanode/src/server/grpc/ddl.rs @@ -17,7 +17,7 @@ use std::sync::Arc; use api::helper::ColumnDataTypeWrapper; use api::result::AdminResultBuilder; use api::v1::alter_expr::Kind; -use api::v1::{AdminResult, AlterExpr, ColumnDef, CreateExpr}; +use api::v1::{AdminResult, AlterExpr, ColumnDef, CreateExpr, DropColumns}; use common_catalog::consts::{DEFAULT_CATALOG_NAME, DEFAULT_SCHEMA_NAME}; use common_error::prelude::{ErrorExt, StatusCode}; use common_query::Output; @@ -191,6 +191,19 @@ fn alter_expr_to_request(expr: AlterExpr) -> Result> { }; Ok(Some(request)) } + Some(Kind::DropColumns(DropColumns { drop_columns })) => { + let alter_kind = AlterKind::DropColumns { + names: drop_columns.into_iter().map(|c| c.name).collect(), + }; + + let request = AlterTableRequest { + catalog_name: expr.catalog_name, + schema_name: expr.schema_name, + table_name: expr.table_name, + alter_kind, + }; + Ok(Some(request)) + } None => Ok(None), } } diff --git a/src/datanode/src/sql/alter.rs b/src/datanode/src/sql/alter.rs index b30dc9c5aca2..077ebd0a9cdb 100644 --- a/src/datanode/src/sql/alter.rs +++ b/src/datanode/src/sql/alter.rs @@ -72,6 +72,9 @@ impl SqlHandler { is_key: false, }], }, + AlterTableOperation::DropColumn { name } => AlterKind::DropColumns { + names: vec![name.value.clone()], + }, }; Ok(AlterTableRequest { catalog_name: Some(catalog_name), diff --git a/src/mito/src/table.rs b/src/mito/src/table.rs index 705b8b035624..689a2b4c1b18 100644 --- a/src/mito/src/table.rs +++ b/src/mito/src/table.rs @@ -481,7 +481,7 @@ fn create_alter_operation( AlterKind::AddColumns { columns } => { create_add_columns_operation(table_name, columns, table_meta) } - AlterKind::RemoveColumns { names } => Ok(AlterOperation::DropColumns { + AlterKind::DropColumns { names } => Ok(AlterOperation::DropColumns { names: names.to_vec(), }), } diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 08661e1c1678..49f7e5079be1 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -41,9 +41,19 @@ impl<'a> ParserContext<'a> { let column_def = parser.parse_column_def()?; AlterTableOperation::AddColumn { column_def } } + } else if parser.parse_keyword(Keyword::DROP) { + if parser.parse_keyword(Keyword::COLUMN) { + let name = self.parser.parse_identifier()?; + AlterTableOperation::DropColumn { name } + } else { + return Err(ParserError::ParserError(format!( + "expect keyword COLUMN after DROP, found {}", + parser.peek_token() + ))); + } } else { return Err(ParserError::ParserError(format!( - "expect ADD or DROP after ALTER TABLE, found {}", + "expect keyword ADD or DROP after ALTER TABLE, found {}", parser.peek_token() ))); }; @@ -89,4 +99,29 @@ mod tests { _ => unreachable!(), } } + + #[test] + fn test_parse_alter_drop_column() { + let sql = "ALTER TABLE my_metric_1 DROP column a"; + let mut result = ParserContext::create_with_dialect(sql, &GenericDialect {}).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::DropColumn { .. }); + match alter_operation { + AlterTableOperation::DropColumn { name } => { + assert_eq!("a", name.value); + } + _ => unreachable!(), + } + } + _ => unreachable!(), + } + } } diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index a9dfaad8d9d5..2add0c78e490 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -12,8 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. -use api::v1::{alter_expr, AddColumn, AlterExpr}; -use sqlparser::ast::{ColumnDef, ObjectName, TableConstraint}; +use api::v1::{alter_expr, AddColumn, AlterExpr, DropColumn}; +use sqlparser::ast::{ColumnDef, Ident, ObjectName, TableConstraint}; use crate::error::UnsupportedAlterTableStatementSnafu; use crate::statements::{sql_column_def_to_grpc_column_def, table_idents_to_full_name}; @@ -47,7 +47,8 @@ pub enum AlterTableOperation { AddConstraint(TableConstraint), /// `ADD [ COLUMN ] ` AddColumn { column_def: ColumnDef }, - // TODO(hl): support remove column + /// `DROP COLUMN ` + DropColumn { name: Ident }, } /// Convert `AlterTable` statement to `AlterExpr` for gRPC @@ -72,6 +73,13 @@ impl TryFrom for AlterExpr { }], }) } + AlterTableOperation::DropColumn { name } => { + alter_expr::Kind::DropColumns(api::v1::DropColumns { + drop_columns: vec![DropColumn { + name: name.value.clone(), + }], + }) + } }; let expr = AlterExpr { catalog_name: Some(catalog), diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index 4676823c21fe..d556e76962e6 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -130,7 +130,7 @@ impl TableMeta { ) -> Result { match alter_kind { AlterKind::AddColumns { columns } => self.add_columns(table_name, columns), - AlterKind::RemoveColumns { names } => self.remove_columns(table_name, names), + AlterKind::DropColumns { names } => self.remove_columns(table_name, names), } } diff --git a/src/table/src/requests.rs b/src/table/src/requests.rs index 344d722103f3..a921cf3bd2e7 100644 --- a/src/table/src/requests.rs +++ b/src/table/src/requests.rs @@ -77,7 +77,7 @@ pub struct AddColumnRequest { #[derive(Debug)] pub enum AlterKind { AddColumns { columns: Vec }, - RemoveColumns { names: Vec }, + DropColumns { names: Vec }, } /// Drop table request From 33a8e612e64e8363657bd1268c238e0e309580f7 Mon Sep 17 00:00:00 2001 From: Dennis Zhuang Date: Thu, 17 Nov 2022 20:59:44 +0800 Subject: [PATCH 2/5] refactor: rename RemoveColumns to DropColumns --- src/mito/src/engine.rs | 2 +- src/table/src/metadata.rs | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/mito/src/engine.rs b/src/mito/src/engine.rs index 8154ad2ecd9c..468b0013e520 100644 --- a/src/mito/src/engine.rs +++ b/src/mito/src/engine.rs @@ -952,7 +952,7 @@ mod tests { catalog_name: None, schema_name: None, table_name: TABLE_NAME.to_string(), - alter_kind: AlterKind::RemoveColumns { + alter_kind: AlterKind::DropColumns { names: vec![String::from("memory"), String::from("my_field")], }, }; diff --git a/src/table/src/metadata.rs b/src/table/src/metadata.rs index d556e76962e6..3fb367589ebe 100644 --- a/src/table/src/metadata.rs +++ b/src/table/src/metadata.rs @@ -576,7 +576,7 @@ mod tests { // Add more columns so we have enough candidate columns to remove. let meta = add_columns_to_meta(&meta); - let alter_kind = AlterKind::RemoveColumns { + let alter_kind = AlterKind::DropColumns { names: vec![String::from("col2"), String::from("my_field")], }; let new_meta = meta @@ -625,7 +625,7 @@ mod tests { .unwrap(); // Remove columns in reverse order to test whether timestamp index is valid. - let alter_kind = AlterKind::RemoveColumns { + let alter_kind = AlterKind::DropColumns { names: vec![String::from("col3"), String::from("col1")], }; let new_meta = meta @@ -685,7 +685,7 @@ mod tests { .build() .unwrap(); - let alter_kind = AlterKind::RemoveColumns { + let alter_kind = AlterKind::DropColumns { names: vec![String::from("unknown")], }; @@ -708,7 +708,7 @@ mod tests { .unwrap(); // Remove column in primary key. - let alter_kind = AlterKind::RemoveColumns { + let alter_kind = AlterKind::DropColumns { names: vec![String::from("col1")], }; @@ -719,7 +719,7 @@ mod tests { assert_eq!(StatusCode::InvalidArguments, err.status_code()); // Remove timestamp column. - let alter_kind = AlterKind::RemoveColumns { + let alter_kind = AlterKind::DropColumns { names: vec![String::from("ts")], }; From addb5aeffb0981bf4efe4a43347fb0e6317c125e Mon Sep 17 00:00:00 2001 From: Dennis Zhuang Date: Thu, 17 Nov 2022 21:22:59 +0800 Subject: [PATCH 3/5] test: alter table --- src/datanode/src/tests/instance_test.rs | 60 +++++++++++++++++++++++-- src/sql/src/parsers/alter_parser.rs | 8 +++- src/sql/src/statements/alter.rs | 4 +- 3 files changed, 65 insertions(+), 7 deletions(-) diff --git a/src/datanode/src/tests/instance_test.rs b/src/datanode/src/tests/instance_test.rs index 5f0b9d1104dd..c8c7631f87ca 100644 --- a/src/datanode/src/tests/instance_test.rs +++ b/src/datanode/src/tests/instance_test.rs @@ -179,8 +179,7 @@ async fn assert_query_result(instance: &Instance, sql: &str, ts: i64, host: &str } } -#[tokio::test(flavor = "multi_thread")] -async fn test_execute_insert() { +async fn setup_test_instance() -> Instance { common_telemetry::init_default_ut_logging(); let (opts, _guard) = test_util::create_tmp_dir_and_datanode_opts("execute_insert"); @@ -195,6 +194,12 @@ async fn test_execute_insert() { .await .unwrap(); + instance +} + +#[tokio::test(flavor = "multi_thread")] +async fn test_execute_insert() { + let instance = setup_test_instance().await; let output = instance .execute_sql( r#"insert into demo(host, cpu, memory, ts) values @@ -479,6 +484,7 @@ async fn test_alter_table() { .await .unwrap(); + // Add column let output = instance .execute_sql("alter table demo add my_tag string null") .await @@ -498,7 +504,10 @@ async fn test_alter_table() { .unwrap(); assert!(matches!(output, Output::AffectedRows(1))); - let output = instance.execute_sql("select * from demo").await.unwrap(); + let output = instance + .execute_sql("select * from demo order by ts") + .await + .unwrap(); let expected = vec![ "+-------+-----+--------+---------------------+--------+", "| host | cpu | memory | ts | my_tag |", @@ -509,6 +518,51 @@ async fn test_alter_table() { "+-------+-----+--------+---------------------+--------+", ]; check_output_stream(output, expected).await; + + // Drop a column + let output = instance + .execute_sql("alter table demo drop column memory") + .await + .unwrap(); + assert!(matches!(output, Output::AffectedRows(0))); + + let output = instance + .execute_sql("select * from demo order by ts") + .await + .unwrap(); + let expected = vec![ + "+-------+-----+---------------------+--------+", + "| host | cpu | ts | my_tag |", + "+-------+-----+---------------------+--------+", + "| host1 | 1.1 | 1970-01-01 00:00:01 | |", + "| host2 | 2.2 | 1970-01-01 00:00:02 | hello |", + "| host3 | 3.3 | 1970-01-01 00:00:03 | |", + "+-------+-----+---------------------+--------+", + ]; + check_output_stream(output, expected).await; + + // insert a new row + let output = instance + .execute_sql("insert into demo(host, cpu, ts, my_tag) values ('host4', 400, 4000, 'world')") + .await + .unwrap(); + assert!(matches!(output, Output::AffectedRows(1))); + + let output = instance + .execute_sql("select * from demo order by ts") + .await + .unwrap(); + let expected = vec![ + "+-------+-----+---------------------+--------+", + "| host | cpu | ts | my_tag |", + "+-------+-----+---------------------+--------+", + "| host1 | 1.1 | 1970-01-01 00:00:01 | |", + "| host2 | 2.2 | 1970-01-01 00:00:02 | hello |", + "| host3 | 3.3 | 1970-01-01 00:00:03 | |", + "| host4 | 400 | 1970-01-01 00:00:04 | world |", + "+-------+-----+---------------------+--------+", + ]; + check_output_stream(output, expected).await; } async fn test_insert_with_default_value_for_type(type_name: &str) { diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 49f7e5079be1..f2193a22b7cb 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -102,7 +102,13 @@ mod tests { #[test] fn test_parse_alter_drop_column() { - let sql = "ALTER TABLE my_metric_1 DROP column a"; + let sql = "ALTER TABLE my_metric_1 DROP a"; + let result = ParserContext::create_with_dialect(sql, &GenericDialect {}).unwrap_err(); + assert!(result + .to_string() + .contains("expect keyword COLUMN after DROP")); + + let sql = "ALTER TABLE my_metric_1 DROP COLUMN a"; let mut result = ParserContext::create_with_dialect(sql, &GenericDialect {}).unwrap(); assert_eq!(1, result.len()); diff --git a/src/sql/src/statements/alter.rs b/src/sql/src/statements/alter.rs index 2add0c78e490..389140e5a61e 100644 --- a/src/sql/src/statements/alter.rs +++ b/src/sql/src/statements/alter.rs @@ -75,9 +75,7 @@ impl TryFrom for AlterExpr { } AlterTableOperation::DropColumn { name } => { alter_expr::Kind::DropColumns(api::v1::DropColumns { - drop_columns: vec![DropColumn { - name: name.value.clone(), - }], + drop_columns: vec![DropColumn { name: name.value }], }) } }; From a1a18cdc2d1a62aac1c4d9fe90bf7bea512e1a86 Mon Sep 17 00:00:00 2001 From: dennis zhuang Date: Thu, 17 Nov 2022 22:10:45 +0800 Subject: [PATCH 4/5] chore: error msg Co-authored-by: Ruihang Xia --- 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 f2193a22b7cb..ba2f9397b508 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -47,7 +47,7 @@ impl<'a> ParserContext<'a> { AlterTableOperation::DropColumn { name } } else { return Err(ParserError::ParserError(format!( - "expect keyword COLUMN after DROP, found {}", + "expect keyword COLUMN after ALTER TABLE DROP, found {}", parser.peek_token() ))); } From bb03581d2d501092cfb461a44a8f273658272e37 Mon Sep 17 00:00:00 2001 From: Dennis Zhuang Date: Thu, 17 Nov 2022 22:25:47 +0800 Subject: [PATCH 5/5] fix: test_parse_alter_drop_column --- 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 ba2f9397b508..9d0204279740 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -106,7 +106,7 @@ mod tests { let result = ParserContext::create_with_dialect(sql, &GenericDialect {}).unwrap_err(); assert!(result .to_string() - .contains("expect keyword COLUMN after DROP")); + .contains("expect keyword COLUMN after ALTER TABLE DROP")); let sql = "ALTER TABLE my_metric_1 DROP COLUMN a"; let mut result = ParserContext::create_with_dialect(sql, &GenericDialect {}).unwrap();