From c79ee3802c5de355fbee3ee5b3b083ad9110bfb8 Mon Sep 17 00:00:00 2001 From: NiwakaDev Date: Fri, 10 Jan 2025 22:43:51 +0900 Subject: [PATCH 1/2] feat: add column if not exists --- src/operator/src/expr_factory.rs | 3 +- src/operator/src/statement/ddl.rs | 28 +++++++++++++++---- src/query/src/datafusion/planner.rs | 1 - src/sql/src/parsers/alter_parser.rs | 2 ++ src/sql/src/statements/alter.rs | 1 + .../standalone/common/alter/add_col.result | 12 ++++++++ .../cases/standalone/common/alter/add_col.sql | 6 ++++ 7 files changed, 45 insertions(+), 8 deletions(-) diff --git a/src/operator/src/expr_factory.rs b/src/operator/src/expr_factory.rs index e2f19efbd4d3..3acf23ae3878 100644 --- a/src/operator/src/expr_factory.rs +++ b/src/operator/src/expr_factory.rs @@ -507,8 +507,7 @@ pub(crate) fn to_alter_table_expr( Ok(AddColumn { column_def: Some(column_def), location: add_column.location.as_ref().map(From::from), - // TODO(yingwen): We don't support `IF NOT EXISTS` for `ADD COLUMN` yet. - add_if_not_exists: false, + add_if_not_exists: add_column.add_if_not_exists, }) }) .collect::>>()?, diff --git a/src/operator/src/statement/ddl.rs b/src/operator/src/statement/ddl.rs index a6f2e6c1c2d5..0bf6928b2224 100644 --- a/src/operator/src/statement/ddl.rs +++ b/src/operator/src/statement/ddl.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use std::sync::Arc; use api::helper::ColumnDataTypeWrapper; @@ -885,12 +885,13 @@ impl StatementExecutor { Ok(Output::new_with_affected_rows(0)) } + /// Verifies an alter and returns whether it is necessary to perform the alter. fn verify_alter( &self, table_id: TableId, table_info: Arc, expr: AlterTableExpr, - ) -> Result<()> { + ) -> Result { let request: AlterTableRequest = common_grpc_expr::alter_expr_to_request(table_id, expr) .context(AlterExprToRequestSnafu)?; @@ -907,6 +908,21 @@ impl StatementExecutor { violated: format!("Invalid table name: {}", new_table_name) } ); + } else if let AlterKind::AddColumns { columns } = alter_kind { + // If all the columns are marked as add_if_not_exists and they already exist in the table, + // there is no need to perform the alter. + let column_names: HashSet = table_info + .meta + .schema + .column_schemas() + .iter() + .map(|schema| schema.name.clone()) + .collect(); + if columns.iter().all(|column| { + column_names.contains(&column.column_schema.name) && column.add_if_not_exists + }) { + return Ok(false); + } } let _ = table_info @@ -916,7 +932,7 @@ impl StatementExecutor { .build() .context(error::BuildTableMetaSnafu { table_name })?; - Ok(()) + Ok(true) } #[tracing::instrument(skip_all)] @@ -971,8 +987,10 @@ impl StatementExecutor { })?; let table_id = table.table_info().ident.table_id; - self.verify_alter(table_id, table.table_info(), expr.clone())?; - + let need_alter = self.verify_alter(table_id, table.table_info(), expr.clone())?; + if !need_alter { + return Ok(Output::new_with_affected_rows(0)); + } info!( "Table info before alter is {:?}, expr: {:?}", table.table_info(), diff --git a/src/query/src/datafusion/planner.rs b/src/query/src/datafusion/planner.rs index c3e8e1f5442f..7927ba16854a 100644 --- a/src/query/src/datafusion/planner.rs +++ b/src/query/src/datafusion/planner.rs @@ -70,7 +70,6 @@ impl DfContextProviderAdapter { ); let tables = resolve_tables(table_names, &mut table_provider).await?; - Ok(Self { engine_state, session_state, diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 4456d28b7591..29c6c729edb7 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -291,6 +291,7 @@ fn parse_string_options(parser: &mut Parser) -> std::result::Result<(String, Str fn parse_add_columns(parser: &mut Parser) -> std::result::Result { parser.expect_keyword(Keyword::ADD)?; let _ = parser.parse_keyword(Keyword::COLUMN); + let add_if_not_exists = parser.parse_keywords(&[Keyword::IF, Keyword::NOT, Keyword::EXISTS]); let mut column_def = parser.parse_column_def()?; column_def.name = ParserContext::canonicalize_identifier(column_def.name); let location = if parser.parse_keyword(Keyword::FIRST) { @@ -312,6 +313,7 @@ fn parse_add_columns(parser: &mut Parser) -> std::result::Result, + pub add_if_not_exists: bool, } impl Display for AddColumn { diff --git a/tests/cases/standalone/common/alter/add_col.result b/tests/cases/standalone/common/alter/add_col.result index b2c7049eff1b..8d951e9419a3 100644 --- a/tests/cases/standalone/common/alter/add_col.result +++ b/tests/cases/standalone/common/alter/add_col.result @@ -19,6 +19,10 @@ ALTER TABLE test ADD COLUMN k INTEGER; Affected Rows: 0 +ALTER TABLE test ADD COLUMN IF NOT EXISTS k INTEGER; + +Affected Rows: 0 + SELECT * FROM test; +---+-------------------------+---+ @@ -108,6 +112,14 @@ ALTER TABLE test ADD COLUMN "foo" STRING default 'foo' PRIMARY KEY, ADD COLUMN " Affected Rows: 0 +ALTER TABLE test ADD COLUMN IF NOT EXISTS "foo" STRING default 'foo' PRIMARY KEY, ADD COLUMN "bar" STRING default 'bar' PRIMARY KEY; + +Error: 4003(TableColumnExists), Column bar already exists in table test + +ALTER TABLE test ADD COLUMN IF NOT EXISTS "foo" STRING default 'foo' PRIMARY KEY, ADD COLUMN IF NOT EXISTS "bar" STRING default 'bar' PRIMARY KEY; + +Affected Rows: 0 + DESC TABLE test; +--------+----------------------+-----+------+---------+---------------+ diff --git a/tests/cases/standalone/common/alter/add_col.sql b/tests/cases/standalone/common/alter/add_col.sql index 7a868a85084b..b666bf24cdad 100644 --- a/tests/cases/standalone/common/alter/add_col.sql +++ b/tests/cases/standalone/common/alter/add_col.sql @@ -6,6 +6,8 @@ INSERT INTO test VALUES (1, 1), (2, 2); ALTER TABLE test ADD COLUMN k INTEGER; +ALTER TABLE test ADD COLUMN IF NOT EXISTS k INTEGER; + SELECT * FROM test; DESC TABLE test; @@ -28,6 +30,10 @@ DESC TABLE test; ALTER TABLE test ADD COLUMN "foo" STRING default 'foo' PRIMARY KEY, ADD COLUMN "bar" STRING default 'bar' PRIMARY KEY; +ALTER TABLE test ADD COLUMN IF NOT EXISTS "foo" STRING default 'foo' PRIMARY KEY, ADD COLUMN "bar" STRING default 'bar' PRIMARY KEY; + +ALTER TABLE test ADD COLUMN IF NOT EXISTS "foo" STRING default 'foo' PRIMARY KEY, ADD COLUMN IF NOT EXISTS "bar" STRING default 'bar' PRIMARY KEY; + DESC TABLE test; DROP TABLE test; From aa55ff15e764961e7c2eaca6e7a0196f2bd39cb6 Mon Sep 17 00:00:00 2001 From: NiwakaDev Date: Mon, 20 Jan 2025 21:12:36 +0900 Subject: [PATCH 2/2] chore: address reviews --- src/operator/src/statement/ddl.rs | 8 +++- src/sql/src/parsers/alter_parser.rs | 61 +++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 2 deletions(-) diff --git a/src/operator/src/statement/ddl.rs b/src/operator/src/statement/ddl.rs index 0bf6928b2224..23cba11de8da 100644 --- a/src/operator/src/statement/ddl.rs +++ b/src/operator/src/statement/ddl.rs @@ -886,6 +886,10 @@ impl StatementExecutor { } /// Verifies an alter and returns whether it is necessary to perform the alter. + /// + /// # Returns + /// + /// Returns true if the alter need to be porformed; otherwise, it returns false. fn verify_alter( &self, table_id: TableId, @@ -911,12 +915,12 @@ impl StatementExecutor { } else if let AlterKind::AddColumns { columns } = alter_kind { // If all the columns are marked as add_if_not_exists and they already exist in the table, // there is no need to perform the alter. - let column_names: HashSet = table_info + let column_names: HashSet<_> = table_info .meta .schema .column_schemas() .iter() - .map(|schema| schema.name.clone()) + .map(|schema| &schema.name) .collect(); if columns.iter().all(|column| { column_names.contains(&column.column_schema.name) && column.add_if_not_exists diff --git a/src/sql/src/parsers/alter_parser.rs b/src/sql/src/parsers/alter_parser.rs index 29c6c729edb7..511caee0f47f 100644 --- a/src/sql/src/parsers/alter_parser.rs +++ b/src/sql/src/parsers/alter_parser.rs @@ -524,6 +524,67 @@ mod tests { } } + #[test] + fn test_parse_add_column_if_not_exists() { + let sql = "ALTER TABLE test ADD COLUMN IF NOT EXISTS a INTEGER, ADD COLUMN b STRING, ADD COLUMN IF NOT EXISTS c INT;"; + let mut result = + ParserContext::create_with_dialect(sql, &GreptimeDbDialect {}, ParseOptions::default()) + .unwrap(); + assert_eq!(result.len(), 1); + let statement = result.remove(0); + assert_matches!(statement, Statement::AlterTable { .. }); + match statement { + Statement::AlterTable(alter) => { + assert_eq!(alter.table_name.0[0].value, "test"); + assert_matches!( + alter.alter_operation, + AlterTableOperation::AddColumns { .. } + ); + match alter.alter_operation { + AlterTableOperation::AddColumns { add_columns } => { + let expected = vec![ + AddColumn { + column_def: ColumnDef { + name: Ident::new("a"), + data_type: DataType::Integer(None), + collation: None, + options: vec![], + }, + location: None, + add_if_not_exists: true, + }, + AddColumn { + column_def: ColumnDef { + name: Ident::new("b"), + data_type: DataType::String(None), + collation: None, + options: vec![], + }, + location: None, + add_if_not_exists: false, + }, + AddColumn { + column_def: ColumnDef { + name: Ident::new("c"), + data_type: DataType::Int(None), + collation: None, + options: vec![], + }, + location: None, + add_if_not_exists: true, + }, + ]; + for (idx, add_column) in add_columns.into_iter().enumerate() { + assert_eq!(add_column, expected[idx]); + } + } + _ => unreachable!(), + } + } + _ => unreachable!(), + } + } + #[test] fn test_parse_alter_drop_column() { let sql = "ALTER TABLE my_metric_1 DROP a";