This is an automated email from the ASF dual-hosted git repository. michaelsmith pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/impala.git
commit 8b375a66a29cfea33a4e418b1fa3b9b5139cf907 Author: xiabaike <[email protected]> AuthorDate: Thu Sep 8 09:18:50 2022 +0000 IMPALA-11565: Support IF NOT EXISTS in alter table add columns for kudu/iceberg table Impala already supports IF NOT EXISTS in alter table add columns for general hive table in IMPALA-7832, but not for kudu/iceberg table. This patch try to add such semantics for kudu/iceberg table. Testing: - Updated E2E DDL tests - Added fe tests Change-Id: I82590e5372e881f2e81d4ed3dd0d32a2d3ddb517 Reviewed-on: http://gerrit.cloudera.org:8080/18953 Tested-by: Impala Public Jenkins <[email protected]> Reviewed-by: Wenzhe Zhou <[email protected]> --- common/thrift/JniCatalog.thrift | 2 +- .../impala/analysis/AlterTableAddColsStmt.java | 17 +++- .../apache/impala/service/CatalogOpExecutor.java | 36 ++++++--- .../apache/impala/analysis/AnalyzeKuduDDLTest.java | 5 ++ .../queries/QueryTest/iceberg-alter.test | 57 ++++++++++++++ .../queries/QueryTest/kudu_alter.test | 92 ++++++++++++++++++++++ 6 files changed, 194 insertions(+), 15 deletions(-) diff --git a/common/thrift/JniCatalog.thrift b/common/thrift/JniCatalog.thrift index 3efe0d0e5..c41762a6f 100755 --- a/common/thrift/JniCatalog.thrift +++ b/common/thrift/JniCatalog.thrift @@ -222,7 +222,7 @@ struct TAlterTableOrViewRenameParams { // Parameters for ALTER TABLE ADD COLUMNS commands. struct TAlterTableAddColsParams { // List of columns to add to the table - 1: required list<CatalogObjects.TColumn> columns + 1: optional list<CatalogObjects.TColumn> columns // If true, no error is raised when a column already exists. 2: required bool if_not_exists diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java index e373e9fa3..dece3c4f8 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java @@ -32,6 +32,7 @@ import org.apache.impala.thrift.TAlterTableType; import org.apache.impala.util.KuduUtil; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Set; @@ -74,7 +75,9 @@ public class AlterTableAddColsStmt extends AlterTableStmt { // are all valid and unique, and that none of the columns conflict with // partition columns. Set<String> colNames = new HashSet<>(); - for (ColumnDef c: columnDefs_) { + Iterator<ColumnDef> iterator = columnDefs_.iterator(); + while (iterator.hasNext()){ + ColumnDef c = iterator.next(); c.analyze(analyzer); String colName = c.getColName().toLowerCase(); if (existingPartitionKeys.contains(colName)) { @@ -83,9 +86,15 @@ public class AlterTableAddColsStmt extends AlterTableStmt { } Column col = t.getColumn(colName); - if (col != null && !ifNotExists_) { - throw new AnalysisException("Column already exists: " + colName); - } else if (!colNames.add(colName)) { + if (col != null) { + if (!ifNotExists_) { + throw new AnalysisException("Column already exists: " + colName); + } + // remove the existing column + iterator.remove(); + continue; + } + if (!colNames.add(colName)) { throw new AnalysisException("Duplicate column name: " + colName); } diff --git a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java index dcf11da26..bc8f1af1a 100644 --- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java +++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java @@ -1035,10 +1035,16 @@ public class CatalogOpExecutor { } switch (params.getAlter_type()) { case ADD_COLUMNS: - TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); - boolean added = alterTableAddCols(tbl, addColParams.getColumns(), - addColParams.isIf_not_exists()); - reloadTableSchema = true; + boolean added = false; + // Columns could be ignored/cleared in AlterTableAddColsStmt, + // that may cause columns to be empty. + if (params.getAdd_cols_params() != null + && params.getAdd_cols_params().getColumnsSize() != 0) { + TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); + added = alterTableAddCols(tbl, addColParams.getColumns(), + addColParams.isIf_not_exists()); + reloadTableSchema = true; + } if (added) { responseSummaryMsg = "New column(s) have been added to the table."; } else { @@ -1267,9 +1273,14 @@ public class CatalogOpExecutor { Preconditions.checkState(tbl.isWriteLockedByCurrentThread()); switch (params.getAlter_type()) { case ADD_COLUMNS: - TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); - KuduCatalogOpExecutor.addColumn(tbl, addColParams.getColumns()); - addSummary(response, "Column(s) have been added."); + if (params.getAdd_cols_params() != null + && params.getAdd_cols_params().getColumnsSize() != 0) { + TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); + KuduCatalogOpExecutor.addColumn(tbl, addColParams.getColumns()); + addSummary(response, "Column(s) have been added."); + } else { + addSummary(response, "No new column(s) have been added to the table."); + } break; case REPLACE_COLUMNS: TAlterTableReplaceColsParams replaceColParams = params.getReplace_cols_params(); @@ -1334,9 +1345,14 @@ public class CatalogOpExecutor { org.apache.iceberg.Transaction iceTxn = IcebergUtil.getIcebergTransaction(tbl); switch (params.getAlter_type()) { case ADD_COLUMNS: - TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); - IcebergCatalogOpExecutor.addColumns(iceTxn, addColParams.getColumns()); - addSummary(response, "Column(s) have been added."); + if (params.getAdd_cols_params() != null + && params.getAdd_cols_params().getColumnsSize() != 0) { + TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); + IcebergCatalogOpExecutor.addColumns(iceTxn, addColParams.getColumns()); + addSummary(response, "Column(s) have been added."); + } else { + addSummary(response, "No new column(s) have been added to the table."); + } break; case DROP_COLUMN: TAlterTableDropColParams dropColParams = params.getDrop_col_params(); diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java index 5f90441d7..ed6d8d775 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeKuduDDLTest.java @@ -729,6 +729,11 @@ public class AnalyzeKuduDDLTest extends FrontendTestBase { AnalyzesOk("alter table functional_kudu.testtbl add columns (a int encoding rle)"); AnalyzesOk("alter table functional_kudu.testtbl add columns (a int compression lz4)"); AnalyzesOk("alter table functional_kudu.testtbl add columns (a int block_size 10)"); + // Test 'if not exists' + AnalyzesOk("alter table functional_kudu.testtbl add if not exists columns " + + "(name string null)"); + AnalyzesOk("alter table functional_kudu.testtbl add if not exists columns " + + "(a string null, b int, c string null)"); // REPLACE columns is not supported for Kudu tables AnalysisError("alter table functional_kudu.testtbl replace columns (a int null)", diff --git a/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test b/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test index b653199d0..5d20f8660 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test +++ b/testdata/workloads/functional-query/queries/QueryTest/iceberg-alter.test @@ -436,3 +436,60 @@ DESCRIBE FORMATTED iceberg_upgrade_v2_merge_mode; ---- TYPES string, string, string ==== +---- QUERY +# Add a column that already exists and a new column that does not exist with +# "if not exists" clause. +ALTER TABLE ice_alter_cols ADD IF NOT EXISTS COLUMNS (a bigint, d bigint) +---- RESULTS +'Column(s) have been added.' +---- TYPES +string +==== +---- QUERY +describe ice_alter_cols; +---- RESULTS +'d','decimal(22,3)','','true' +'a','bigint','','true' +---- TYPES +STRING,STRING,STRING,STRING +==== +---- QUERY +# Add column for the same name, but of a different type +ALTER TABLE ice_alter_cols ADD IF NOT EXISTS COLUMNS (a string, d string) +---- RESULTS +'No new column(s) have been added to the table.' +---- TYPES +string +==== +---- QUERY +# Add a column that already exists and a new column that does not exist with +# "if not exists" clause. +ALTER TABLE ice_alter_cols ADD IF NOT EXISTS COLUMNS (a bigint, b bigint) +---- RESULTS +'Column(s) have been added.' +---- TYPES +string +==== +---- QUERY +DESCRIBE ice_alter_cols +---- RESULTS +'d','decimal(22,3)','','true' +'a','bigint','','true' +'b','bigint','','true' +---- TYPES +STRING,STRING,STRING,STRING +==== +---- QUERY +ALTER TABLE ice_alter_cols ADD IF NOT EXISTS COLUMNS (c bigint, c string) +---- CATCH +AnalysisException: Duplicate column name: c +==== +---- QUERY +ALTER TABLE ice_alter_cols DROP COLUMN a; +ALTER TABLE ice_alter_cols DROP COLUMN b; +DESCRIBE ice_alter_cols; +---- RESULTS +'d','decimal(22,3)','','true' +---- TYPES +STRING,STRING,STRING,STRING +==== \ No newline at end of file diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test index 63e5c6a45..0ed93f69f 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test +++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test @@ -347,6 +347,98 @@ alter table tbl_to_alter add columns (invalid_col int not null) A new non-null column must have a default value ==== ---- QUERY +# Add a column that already exists and a new column that does not exist. +alter table tbl_to_alter add columns (new_col4 string, new_col5 int) +---- CATCH +AnalysisException: Column already exists: new_col4 +==== +---- QUERY +describe tbl_to_alter; +---- LABELS +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4 +---- TYPES +INT,STRING,BIGINT,INT,BIGINT,STRING,INT +==== +---- QUERY +# Add a column that already exists and a new column that does not exist with +# "if not exists" clause. +alter table tbl_to_alter add if not exists columns (new_col4 int, new_col5 int) +---- RESULTS +'Column(s) have been added.' +---- TYPES +string +==== +---- QUERY +describe tbl_to_alter; +---- LABELS +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4, NEW_COL5 +---- TYPES +INT,STRING,BIGINT,INT,BIGINT,STRING,INT,INT +==== +---- QUERY +# Add a column that already exists and a new column that does not exist with +# "if not exists" clause. +alter table tbl_to_alter add if not exists columns (new_col4 string, new_col6 int) +---- RESULTS +'Column(s) have been added.' +---- TYPES +string +==== +---- QUERY +describe tbl_to_alter; +---- LABELS +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4, NEW_COL5, NEW_COL6 +---- TYPES +INT,STRING,BIGINT,INT,BIGINT,STRING,INT,INT,INT +==== +---- QUERY +# All new columns are existing columns. +alter table tbl_to_alter add if not exists columns (new_col3 string, new_col4 int); +---- RESULTS +'No new column(s) have been added to the table.' +---- TYPES +string +==== +---- QUERY +# Test for duplicated columns. +alter table tbl_to_alter add if not exists columns (new_col7 string, new_col7 int); +---- CATCH +AnalysisException: Duplicate column name: new_col7 +==== +---- QUERY +describe tbl_to_alter; +---- LABELS +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4, NEW_COL5, NEW_COL6 +---- TYPES +INT,STRING,BIGINT,INT,BIGINT,STRING,INT,INT,INT +==== +---- QUERY +# Recover status after add 'new_col5' column +alter table tbl_to_alter drop column new_col5 +---- RESULTS +'Column has been dropped.' +==== +---- QUERY +describe tbl_to_alter; +---- LABELS +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4, NEW_COL6 +---- TYPES +INT,STRING,BIGINT,INT,BIGINT,STRING,INT,INT +==== +---- QUERY +# Recover status after add 'new_col6' column +alter table tbl_to_alter drop column new_col6 +---- RESULTS +'Column has been dropped.' +==== +---- QUERY +describe tbl_to_alter; +---- LABELS +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4 +---- TYPES +INT,STRING,BIGINT,INT,BIGINT,STRING,INT +==== +---- QUERY # Add a column with name reserved by Kudu engine alter table tbl_to_alter add columns (auto_incrementing_id bigint) ---- CATCH
