diff --git a/common/thrift/JniCatalog.thrift b/common/thrift/JniCatalog.thrift index b9367734a..2d97f4363 100644 --- a/common/thrift/JniCatalog.thrift +++ b/common/thrift/JniCatalog.thrift @@ -92,7 +92,8 @@ struct TAlterDbParams { // Types of ALTER TABLE commands supported. enum TAlterTableType { - ADD_REPLACE_COLUMNS, + ADD_COLUMNS, + REPLACE_COLUMNS, ADD_PARTITION, ADD_DROP_RANGE_PARTITION, ALTER_COLUMN, @@ -207,13 +208,19 @@ struct TAlterTableOrViewRenameParams { 1: required CatalogObjects.TTableName new_table_name } -// Parameters for ALTER TABLE ADD|REPLACE COLUMNS commands. -struct TAlterTableAddReplaceColsParams { +// Parameters for ALTER TABLE ADD COLUMNS commands. +struct TAlterTableAddColsParams { // List of columns to add to the table 1: required list columns - // If true, replace all existing columns. If false add (append) columns to the table. - 2: required bool replace_existing_cols + // If true, no error is raised when a column already exists. + 2: required bool if_not_exists +} + +// Parameters for ALTER TABLE REPLACE COLUMNS commands. +struct TAlterTableReplaceColsParams { + // List of columns to replace to the table + 1: required list columns } // Parameters for specifying a single partition in ALTER TABLE ADD PARTITION @@ -385,7 +392,7 @@ struct TAlterTableParams { 3: optional TAlterTableOrViewRenameParams rename_params // Parameters for ALTER TABLE ADD COLUMNS - 4: optional TAlterTableAddReplaceColsParams add_replace_cols_params + 4: optional TAlterTableAddColsParams add_cols_params // Parameters for ALTER TABLE ADD PARTITION 5: optional TAlterTableAddPartitionParams add_partition_params @@ -422,6 +429,9 @@ struct TAlterTableParams { // Parameters for ALTER TABLE/VIEW SET OWNER 16: optional TAlterTableOrViewSetOwnerParams set_owner_params + + // Parameters for ALTER TABLE REPLACE COLUMNS + 17: optional TAlterTableReplaceColsParams replace_cols_params } // Parameters of CREATE TABLE LIKE commands diff --git a/fe/src/main/cup/sql-parser.cup b/fe/src/main/cup/sql-parser.cup index bbd5ccf67..051bb4863 100644 --- a/fe/src/main/cup/sql-parser.cup +++ b/fe/src/main/cup/sql-parser.cup @@ -480,7 +480,6 @@ nonterminal THdfsFileFormat file_format_create_table_val; nonterminal Boolean if_exists_val; nonterminal Boolean if_not_exists_val; nonterminal Boolean is_primary_key_val; -nonterminal Boolean replace_existing_cols_val; nonterminal HdfsUri location_val; nonterminal RowFormat row_format_val, opt_row_format_val; nonterminal String field_terminator_val; @@ -1095,9 +1094,19 @@ alter_db_stmt ::= // a partition clause does not make sense for this stmt. If a partition // is given, manually throw a parse error. alter_tbl_stmt ::= - KW_ALTER KW_TABLE table_name:table replace_existing_cols_val:replace KW_COLUMNS - LPAREN column_def_list:col_defs RPAREN - {: RESULT = new AlterTableAddReplaceColsStmt(table, col_defs, replace); :} + KW_ALTER KW_TABLE table_name:table KW_ADD KW_COLUMN if_not_exists_val:if_not_exists + column_def:col_def + {: + List list = new ArrayList<>(); + list.add(col_def); + RESULT = new AlterTableAddColsStmt(table, if_not_exists, list); + :} + | KW_ALTER KW_TABLE table_name:table KW_ADD if_not_exists_val:if_not_exists KW_COLUMNS + LPAREN column_def_list:col_defs RPAREN + {: RESULT = new AlterTableAddColsStmt(table, if_not_exists, col_defs); :} + | KW_ALTER KW_TABLE table_name:table KW_REPLACE KW_COLUMNS + LPAREN column_def_list:col_defs RPAREN + {: RESULT = new AlterTableReplaceColsStmt(table, col_defs); :} | KW_ALTER KW_TABLE table_name:table KW_ADD if_not_exists_val:if_not_exists partition_def_list:partitions {: RESULT = new AlterTableAddPartitionStmt(table, if_not_exists, partitions); :} @@ -1199,13 +1208,6 @@ opt_kw_column ::= | /* empty */ ; -replace_existing_cols_val ::= - KW_REPLACE - {: RESULT = true; :} - | KW_ADD - {: RESULT = false; :} - ; - create_db_stmt ::= KW_CREATE db_or_schema_kw if_not_exists_val:if_not_exists ident_or_default:db_name opt_comment_val:comment location_val:location diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java similarity index 74% rename from fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java rename to fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java index 1a506a1b4..f8495378f 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddColsStmt.java @@ -17,56 +17,38 @@ package org.apache.impala.analysis; -import java.util.HashSet; -import java.util.List; -import java.util.Set; - +import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; import org.apache.hadoop.hive.metastore.api.FieldSchema; - import org.apache.impala.catalog.Column; import org.apache.impala.catalog.FeHBaseTable; import org.apache.impala.catalog.FeKuduTable; import org.apache.impala.catalog.FeTable; import org.apache.impala.common.AnalysisException; -import org.apache.impala.thrift.TAlterTableAddReplaceColsParams; +import org.apache.impala.thrift.TAlterTableAddColsParams; import org.apache.impala.thrift.TAlterTableParams; import org.apache.impala.thrift.TAlterTableType; -import com.google.common.base.Preconditions; -import com.google.common.collect.Lists; + +import java.util.HashSet; +import java.util.List; +import java.util.Set; /** - * Represents an ALTER TABLE ADD|REPLACE COLUMNS (colDef1, colDef2, ...) statement. + * Represents + * - ALTER TABLE ADD [IF NOT EXISTS] COLUMNS (colDef1, colDef2, ...) + * - ALTER TABLE ADD COLUMN [IF NOT EXISTS] colDef + * statements. */ -public class AlterTableAddReplaceColsStmt extends AlterTableStmt { +public class AlterTableAddColsStmt extends AlterTableStmt { + private final boolean ifNotExists_; private final List columnDefs_; - private final boolean replaceExistingCols_; - public AlterTableAddReplaceColsStmt(TableName tableName, List columnDefs, - boolean replaceExistingCols) { + public AlterTableAddColsStmt(TableName tableName, boolean ifNotExists, + List columnDefs) { super(tableName); + ifNotExists_ = ifNotExists; Preconditions.checkState(columnDefs != null && columnDefs.size() > 0); columnDefs_ = Lists.newArrayList(columnDefs); - replaceExistingCols_ = replaceExistingCols; - } - - public List getColumnDescs() { return columnDefs_; } - - // Replace columns instead of appending new columns. - public boolean getReplaceExistingCols() { - return replaceExistingCols_; - } - - @Override - public TAlterTableParams toThrift() { - TAlterTableParams params = super.toThrift(); - params.setAlter_type(TAlterTableType.ADD_REPLACE_COLUMNS); - TAlterTableAddReplaceColsParams colParams = new TAlterTableAddReplaceColsParams(); - for (ColumnDef col: getColumnDescs()) { - colParams.addToColumns(col.toThrift()); - } - colParams.setReplace_existing_cols(replaceExistingCols_); - params.setAdd_replace_cols_params(colParams); - return params; } @Override @@ -76,16 +58,10 @@ public class AlterTableAddReplaceColsStmt extends AlterTableStmt { // TODO: Support column-level DDL on HBase tables. Requires updating the column // mappings along with the table columns. if (t instanceof FeHBaseTable) { - throw new AnalysisException("ALTER TABLE ADD|REPLACE COLUMNS not currently " + + throw new AnalysisException("ALTER TABLE ADD COLUMNS not currently " + "supported on HBase tables."); } - boolean isKuduTable = t instanceof FeKuduTable; - if (isKuduTable && replaceExistingCols_) { - throw new AnalysisException("ALTER TABLE REPLACE COLUMNS is not " + - "supported on Kudu tables."); - } - // Build a set of the partition keys for the table. Set existingPartitionKeys = new HashSet<>(); for (FieldSchema fs: t.getMetaStoreTable().getPartitionKeys()) { @@ -105,13 +81,13 @@ public class AlterTableAddReplaceColsStmt extends AlterTableStmt { } Column col = t.getColumn(colName); - if (col != null && !replaceExistingCols_) { + if (col != null && !ifNotExists_) { throw new AnalysisException("Column already exists: " + colName); } else if (!colNames.add(colName)) { throw new AnalysisException("Duplicate column name: " + colName); } - if (isKuduTable) { + if (t instanceof FeKuduTable) { if (c.getType().isComplexType()) { throw new AnalysisException("Kudu tables do not support complex types: " + c.toString()); @@ -130,4 +106,17 @@ public class AlterTableAddReplaceColsStmt extends AlterTableStmt { } } } + + @Override + public TAlterTableParams toThrift() { + TAlterTableParams params = super.toThrift(); + params.setAlter_type(TAlterTableType.ADD_COLUMNS); + TAlterTableAddColsParams colParams = new TAlterTableAddColsParams(); + for (ColumnDef col: columnDefs_) { + colParams.addToColumns(col.toThrift()); + } + colParams.setIf_not_exists(ifNotExists_); + params.setAdd_cols_params(colParams); + return params; + } } diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java new file mode 100644 index 000000000..c9c4dd939 --- /dev/null +++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableReplaceColsStmt.java @@ -0,0 +1,100 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.impala.analysis; + +import com.google.common.base.Preconditions; +import com.google.common.collect.Lists; +import org.apache.hadoop.hive.metastore.api.FieldSchema; +import org.apache.impala.catalog.Column; +import org.apache.impala.catalog.FeHBaseTable; +import org.apache.impala.catalog.FeKuduTable; +import org.apache.impala.catalog.FeTable; +import org.apache.impala.common.AnalysisException; +import org.apache.impala.thrift.TAlterTableParams; +import org.apache.impala.thrift.TAlterTableReplaceColsParams; +import org.apache.impala.thrift.TAlterTableType; + +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +/** + * Represents an ALTER TABLE REPLACE COLUMNS (colDef1, colDef2, ...) statement. + */ +public class AlterTableReplaceColsStmt extends AlterTableStmt { + private final List columnDefs_; + + public AlterTableReplaceColsStmt(TableName tableName, List columnDefs) { + super(tableName); + Preconditions.checkState(columnDefs != null && columnDefs.size() > 0); + columnDefs_ = Lists.newArrayList(columnDefs); + } + + @Override + public void analyze(Analyzer analyzer) throws AnalysisException { + super.analyze(analyzer); + FeTable t = getTargetTable(); + // TODO: Support column-level DDL on HBase tables. Requires updating the column + // mappings along with the table columns. + if (t instanceof FeHBaseTable) { + throw new AnalysisException("ALTER TABLE REPLACE COLUMNS not currently " + + "supported on HBase tables."); + } + + boolean isKuduTable = t instanceof FeKuduTable; + if (isKuduTable) { + throw new AnalysisException("ALTER TABLE REPLACE COLUMNS is not " + + "supported on Kudu tables."); + } + + // Build a set of the partition keys for the table. + Set existingPartitionKeys = new HashSet<>(); + for (FieldSchema fs: t.getMetaStoreTable().getPartitionKeys()) { + existingPartitionKeys.add(fs.getName().toLowerCase()); + } + + // Make sure the new columns don't already exist in the table, that the names + // are all valid and unique, and that none of the columns conflict with + // partition columns. + Set colNames = new HashSet<>(); + for (ColumnDef c: columnDefs_) { + c.analyze(analyzer); + String colName = c.getColName().toLowerCase(); + if (existingPartitionKeys.contains(colName)) { + throw new AnalysisException( + "Column name conflicts with existing partition column: " + colName); + } + + if (!colNames.add(colName)) { + throw new AnalysisException("Duplicate column name: " + colName); + } + } + } + + @Override + public TAlterTableParams toThrift() { + TAlterTableParams params = super.toThrift(); + params.setAlter_type(TAlterTableType.REPLACE_COLUMNS); + TAlterTableReplaceColsParams colParams = new TAlterTableReplaceColsParams(); + for (ColumnDef col: columnDefs_) { + colParams.addToColumns(col.toThrift()); + } + params.setReplace_cols_params(colParams); + return params; + } +} 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 04ec23d7b..66f27cd41 100644 --- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java +++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java @@ -90,14 +90,15 @@ import org.apache.impala.compat.MetastoreShim; import org.apache.impala.thrift.JniCatalogConstants; import org.apache.impala.thrift.TAlterDbParams; import org.apache.impala.thrift.TAlterDbSetOwnerParams; +import org.apache.impala.thrift.TAlterTableAddColsParams; import org.apache.impala.thrift.TAlterTableAddDropRangePartitionParams; import org.apache.impala.thrift.TAlterTableAddPartitionParams; -import org.apache.impala.thrift.TAlterTableAddReplaceColsParams; import org.apache.impala.thrift.TAlterTableAlterColParams; import org.apache.impala.thrift.TAlterTableDropColParams; import org.apache.impala.thrift.TAlterTableDropPartitionParams; import org.apache.impala.thrift.TAlterTableOrViewSetOwnerParams; import org.apache.impala.thrift.TAlterTableParams; +import org.apache.impala.thrift.TAlterTableReplaceColsParams; import org.apache.impala.thrift.TAlterTableSetCachedParams; import org.apache.impala.thrift.TAlterTableSetFileFormatParams; import org.apache.impala.thrift.TAlterTableSetLocationParams; @@ -420,18 +421,23 @@ public class CatalogOpExecutor { return; } switch (params.getAlter_type()) { - case ADD_REPLACE_COLUMNS: - TAlterTableAddReplaceColsParams addReplaceColParams = - params.getAdd_replace_cols_params(); - alterTableAddReplaceCols(tbl, addReplaceColParams.getColumns(), - addReplaceColParams.isReplace_existing_cols()); + case ADD_COLUMNS: + TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); + boolean added = alterTableAddCols(tbl, addColParams.getColumns(), + addColParams.isIf_not_exists()); reloadTableSchema = true; - if (addReplaceColParams.isReplace_existing_cols()) { - addSummary(response, "Table columns have been replaced."); - } else { + if (added) { addSummary(response, "New column(s) have been added to the table."); + } else { + addSummary(response, "No new column(s) have been added to the table."); } break; + case REPLACE_COLUMNS: + TAlterTableReplaceColsParams replaceColParams = params.getReplace_cols_params(); + alterTableReplaceCols(tbl, replaceColParams.getColumns()); + reloadTableSchema = true; + addSummary(response, "Table columns have been replaced."); + break; case ADD_PARTITION: // Create and add HdfsPartition objects to the corresponding HdfsTable and load // their block metadata. Get the new table object with an updated catalog @@ -585,7 +591,8 @@ public class CatalogOpExecutor { * Kudu in addition to the HMS table. */ private boolean altersKuduTable(TAlterTableType type) { - return type == TAlterTableType.ADD_REPLACE_COLUMNS + return type == TAlterTableType.ADD_COLUMNS + || type == TAlterTableType.REPLACE_COLUMNS || type == TAlterTableType.DROP_COLUMN || type == TAlterTableType.ALTER_COLUMN || type == TAlterTableType.ADD_DROP_RANGE_PARTITION; @@ -598,29 +605,31 @@ public class CatalogOpExecutor { KuduTable tbl, long newCatalogVersion) throws ImpalaException { Preconditions.checkState(tbl.getLock().isHeldByCurrentThread()); switch (params.getAlter_type()) { - case ADD_REPLACE_COLUMNS: - TAlterTableAddReplaceColsParams addReplaceColParams = - params.getAdd_replace_cols_params(); - KuduCatalogOpExecutor.addColumn((KuduTable) tbl, - addReplaceColParams.getColumns()); - addSummary(response, "Column has been added/replaced."); + case ADD_COLUMNS: + TAlterTableAddColsParams addColParams = params.getAdd_cols_params(); + KuduCatalogOpExecutor.addColumn(tbl, addColParams.getColumns()); + addSummary(response, "Column(s) have been added."); + break; + case REPLACE_COLUMNS: + TAlterTableReplaceColsParams replaceColParams = params.getReplace_cols_params(); + KuduCatalogOpExecutor.addColumn(tbl, replaceColParams.getColumns()); + addSummary(response, "Column(s) have been replaced."); break; case DROP_COLUMN: TAlterTableDropColParams dropColParams = params.getDrop_col_params(); - KuduCatalogOpExecutor.dropColumn((KuduTable) tbl, - dropColParams.getCol_name()); + KuduCatalogOpExecutor.dropColumn(tbl, dropColParams.getCol_name()); addSummary(response, "Column has been dropped."); break; case ALTER_COLUMN: TAlterTableAlterColParams alterColParams = params.getAlter_col_params(); - KuduCatalogOpExecutor.alterColumn((KuduTable) tbl, alterColParams.getCol_name(), + KuduCatalogOpExecutor.alterColumn(tbl, alterColParams.getCol_name(), alterColParams.getNew_col_def()); addSummary(response, "Column has been altered."); break; case ADD_DROP_RANGE_PARTITION: TAlterTableAddDropRangePartitionParams partParams = params.getAdd_drop_range_partition_params(); - KuduCatalogOpExecutor.addDropRangePartition((KuduTable) tbl, partParams); + KuduCatalogOpExecutor.addDropRangePartition(tbl, partParams); addSummary(response, "Range partition has been " + (partParams.type == TRangePartitionOperationType.ADD ? "added." : "dropped.")); @@ -2027,28 +2036,49 @@ public class CatalogOpExecutor { } /** - * Appends one or more columns to the given table, optionally replacing all existing - * columns. + * Appends one or more columns to the given table. Returns true if there a column was + * added; false otherwise. */ - private void alterTableAddReplaceCols(Table tbl, List columns, - boolean replaceExistingCols) throws ImpalaException { + private boolean alterTableAddCols(Table tbl, List columns, boolean ifNotExists) + throws ImpalaException { + Preconditions.checkState(tbl.getLock().isHeldByCurrentThread()); + org.apache.hadoop.hive.metastore.api.Table msTbl = tbl.getMetaStoreTable().deepCopy(); + List colsToAdd = new ArrayList<>(); + for (TColumn column: columns) { + Column col = tbl.getColumn(column.getColumnName()); + if (ifNotExists && col != null) continue; + if (col != null) { + throw new CatalogException( + String.format("Column '%s' in table '%s' already exists.", + col.getName(), tbl.getName())); + } + colsToAdd.add(column); + } + // Only add columns that do not exist. + if (!colsToAdd.isEmpty()) { + // Append the new column to the existing list of columns. + msTbl.getSd().getCols().addAll(buildFieldSchemaList(colsToAdd)); + applyAlterTable(msTbl, true); + return true; + } + return false; + } + + /** + * Replaces all existing columns to the given table. + */ + private void alterTableReplaceCols(Table tbl, List columns) + throws ImpalaException { Preconditions.checkState(tbl.getLock().isHeldByCurrentThread()); org.apache.hadoop.hive.metastore.api.Table msTbl = tbl.getMetaStoreTable().deepCopy(); List newColumns = buildFieldSchemaList(columns); - if (replaceExistingCols) { - msTbl.getSd().setCols(newColumns); - String sortByKey = AlterTableSortByStmt.TBL_PROP_SORT_COLUMNS; - if (msTbl.getParameters().containsKey(sortByKey)) { - String oldColumns = msTbl.getParameters().get(sortByKey); - String alteredColumns = MetaStoreUtil.intersectCsvListWithColumNames(oldColumns, - columns); - msTbl.getParameters().put(sortByKey, alteredColumns); - } - } else { - // Append the new column to the existing list of columns. - for (FieldSchema fs: buildFieldSchemaList(columns)) { - msTbl.getSd().addToCols(fs); - } + msTbl.getSd().setCols(newColumns); + String sortByKey = AlterTableSortByStmt.TBL_PROP_SORT_COLUMNS; + if (msTbl.getParameters().containsKey(sortByKey)) { + String oldColumns = msTbl.getParameters().get(sortByKey); + String alteredColumns = MetaStoreUtil.intersectCsvListWithColumNames(oldColumns, + columns); + msTbl.getParameters().put(sortByKey, alteredColumns); } applyAlterTable(msTbl, true); } diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java index b7949ba2f..7ffa719eb 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java @@ -321,39 +321,104 @@ public class AnalyzeDDLTest extends FrontendTestBase { } @Test - public void TestAlterTableAddReplaceColumns() throws AnalysisException { + public void TestAlterTableAddColumn() { + AnalyzesOk("alter table functional.alltypes add column new_col int"); + AnalyzesOk("alter table functional.alltypes add column NEW_COL int"); + AnalyzesOk("alter table functional.alltypes add column if not exists int_col int"); + AnalyzesOk("alter table functional.alltypes add column if not exists INT_COL int"); + + // Column name must be unique for add. + AnalysisError("alter table functional.alltypes add column int_col int", + "Column already exists: int_col"); + AnalysisError("alter table functional.alltypes add column INT_COL int", + "Column already exists: int_col"); + // Add a column with same name as a partition column. + AnalysisError("alter table functional.alltypes add column year int", + "Column name conflicts with existing partition column: year"); + AnalysisError("alter table functional.alltypes add column if not exists year int", + "Column name conflicts with existing partition column: year"); + AnalysisError("alter table functional.alltypes add column YEAR int", + "Column name conflicts with existing partition column: year"); + AnalysisError("alter table functional.alltypes add column if not exists YEAR int", + "Column name conflicts with existing partition column: year"); + // Invalid column name. + AnalysisError("alter table functional.alltypes add column `???` int", + "Invalid column/field name: ???"); + + // Table/Db does not exist. + AnalysisError("alter table db_does_not_exist.alltypes add column i int", + "Could not resolve table reference: 'db_does_not_exist.alltypes'"); + AnalysisError("alter table functional.table_does_not_exist add column i int", + "Could not resolve table reference: 'functional.table_does_not_exist'"); + + // Cannot ALTER TABLE a view. + AnalysisError("alter table functional.alltypes_view add column c1 string", + "ALTER TABLE not allowed on a view: functional.alltypes_view"); + // Cannot ALTER TABLE a nested collection. + AnalysisError("alter table allcomplextypes.int_array_col add column c1 string", + createAnalysisCtx("functional"), + "ALTER TABLE not allowed on a nested collection: allcomplextypes.int_array_col"); + // Cannot ALTER TABLE produced by a data source. + AnalysisError("alter table functional.alltypes_datasource add column c1 string", + "ALTER TABLE not allowed on a table produced by a data source: " + + "functional.alltypes_datasource"); + + // Cannot ALTER TABLE ADD COLUMNS on an HBase table. + AnalysisError("alter table functional_hbase.alltypes add column i int", + "ALTER TABLE ADD COLUMNS not currently supported on HBase tables."); + + // Cannot ALTER ADD COLUMN primary key on Kudu table. + AnalysisError("alter table functional_kudu.alltypes add column " + + "new_col int primary key", + "Cannot add a primary key using an ALTER TABLE ADD COLUMNS statement: " + + "new_col INT PRIMARY KEY"); + + // A non-null column must have a default on Kudu table. + AnalysisError("alter table functional_kudu.alltypes add column new_col int not null", + "A new non-null column must have a default value: new_col INT NOT NULL"); + + // Cannot ALTER ADD COLUMN complex type on Kudu table. + AnalysisError("alter table functional_kudu.alltypes add column c struct", + "Kudu tables do not support complex types: c STRUCT"); + + // A not null is a Kudu only option.. + AnalysisError("alter table functional.alltypes add column new_col int not null", + "The specified column options are only supported in Kudu tables: " + + "new_col INT NOT NULL"); + } + + @Test + public void TestAlterTableAddColumns() { AnalyzesOk("alter table functional.alltypes add columns (new_col int)"); + AnalyzesOk("alter table functional.alltypes add columns (NEW_COL int)"); AnalyzesOk("alter table functional.alltypes add columns (c1 string comment 'hi')"); AnalyzesOk("alter table functional.alltypes add columns (c struct)"); - AnalyzesOk( - "alter table functional.alltypes replace columns (c1 int comment 'c', c2 int)"); - AnalyzesOk("alter table functional.alltypes replace columns (c array)"); + AnalyzesOk("alter table functional.alltypes add if not exists columns (int_col int)"); + AnalyzesOk("alter table functional.alltypes add if not exists columns (INT_COL int)"); - // Column name must be unique for add + // Column name must be unique for add. AnalysisError("alter table functional.alltypes add columns (int_col int)", "Column already exists: int_col"); - // Add a column with same name as a partition column + // Add a column with same name as a partition column. AnalysisError("alter table functional.alltypes add columns (year int)", "Column name conflicts with existing partition column: year"); + AnalysisError("alter table functional.alltypes add if not exists columns (year int)", + "Column name conflicts with existing partition column: year"); // Invalid column name. AnalysisError("alter table functional.alltypes add columns (`???` int)", "Invalid column/field name: ???"); - AnalysisError("alter table functional.alltypes replace columns (`???` int)", - "Invalid column/field name: ???"); - // Replace should not throw an error if the column already exists - AnalyzesOk("alter table functional.alltypes replace columns (int_col int)"); - // It is not possible to replace a partition column - AnalysisError("alter table functional.alltypes replace columns (Year int)", - "Column name conflicts with existing partition column: year"); - - // Duplicate column names + // Duplicate column names. AnalysisError("alter table functional.alltypes add columns (c1 int, c1 int)", "Duplicate column name: c1"); - AnalysisError("alter table functional.alltypes replace columns (c1 int, C1 int)", + AnalysisError("alter table functional.alltypes add columns (c1 int, C1 int)", "Duplicate column name: c1"); + AnalysisError("alter table functional.alltypes add if not exists columns " + + "(c1 int, c1 int)", "Duplicate column name: c1"); + AnalysisError("alter table functional.alltypes add if not exists columns " + + "(c1 int, C1 int)", "Duplicate column name: c1"); - // Table/Db does not exist + // Table/Db does not exist. AnalysisError("alter table db_does_not_exist.alltypes add columns (i int)", "Could not resolve table reference: 'db_does_not_exist.alltypes'"); AnalysisError("alter table functional.table_does_not_exist add columns (i int)", @@ -374,9 +439,85 @@ public class AnalyzeDDLTest extends FrontendTestBase { "ALTER TABLE not allowed on a table produced by a data source: " + "functional.alltypes_datasource"); - // Cannot ALTER TABLE ADD/REPLACE COLUMNS on an HBase table. + // Cannot ALTER TABLE ADD COLUMNS on an HBase table. AnalysisError("alter table functional_hbase.alltypes add columns (i int)", - "ALTER TABLE ADD|REPLACE COLUMNS not currently supported on HBase tables."); + "ALTER TABLE ADD COLUMNS not currently supported on HBase tables."); + + // Cannot ALTER ADD COLUMNS primary key on Kudu table. + AnalysisError("alter table functional_kudu.alltypes add columns " + + "(new_col int primary key)", + "Cannot add a primary key using an ALTER TABLE ADD COLUMNS statement: " + + "new_col INT PRIMARY KEY"); + + // A non-null column must have a default on Kudu table. + AnalysisError("alter table functional_kudu.alltypes add columns" + + "(new_col int not null)", + "A new non-null column must have a default value: new_col INT NOT NULL"); + + // Cannot ALTER ADD COLUMN complex type on Kudu table. + AnalysisError("alter table functional_kudu.alltypes add columns (c struct)", + "Kudu tables do not support complex types: c STRUCT"); + + // A not null is a Kudu only option.. + AnalysisError("alter table functional.alltypes add columns(new_col int not null)", + "The specified column options are only supported in Kudu tables: " + + "new_col INT NOT NULL"); + } + + @Test + public void TestAlterTableReplaceColumns() { + AnalyzesOk("alter table functional.alltypes replace columns " + + "(c1 int comment 'c', c2 int)"); + AnalyzesOk("alter table functional.alltypes replace columns " + + "(C1 int comment 'c', C2 int)"); + AnalyzesOk("alter table functional.alltypes replace columns (c array)"); + // Invalid column name. + AnalysisError("alter table functional.alltypes replace columns (`???` int)", + "Invalid column/field name: ???"); + + // Replace should not throw an error if the column already exists. + AnalyzesOk("alter table functional.alltypes replace columns (int_col int)"); + AnalyzesOk("alter table functional.alltypes replace columns (INT_COL int)"); + // It is not possible to replace a partition column. + AnalysisError("alter table functional.alltypes replace columns (year int)", + "Column name conflicts with existing partition column: year"); + AnalysisError("alter table functional.alltypes replace columns (Year int)", + "Column name conflicts with existing partition column: year"); + + // Duplicate column names. + AnalysisError("alter table functional.alltypes replace columns (c1 int, c1 int)", + "Duplicate column name: c1"); + AnalysisError("alter table functional.alltypes replace columns (c1 int, C1 int)", + "Duplicate column name: c1"); + + // Table/Db does not exist + AnalysisError("alter table db_does_not_exist.alltypes replace columns (i int)", + "Could not resolve table reference: 'db_does_not_exist.alltypes'"); + AnalysisError("alter table functional.table_does_not_exist replace columns (i int)", + "Could not resolve table reference: 'functional.table_does_not_exist'"); + + // Cannot ALTER TABLE a view. + AnalysisError("alter table functional.alltypes_view " + + "replace columns (c1 string comment 'hi')", + "ALTER TABLE not allowed on a view: functional.alltypes_view"); + // Cannot ALTER TABLE a nested collection. + AnalysisError("alter table allcomplextypes.int_array_col " + + "replace columns (c1 string comment 'hi')", + createAnalysisCtx("functional"), + "ALTER TABLE not allowed on a nested collection: allcomplextypes.int_array_col"); + // Cannot ALTER TABLE produced by a data source. + AnalysisError("alter table functional.alltypes_datasource " + + "replace columns (c1 string comment 'hi')", + "ALTER TABLE not allowed on a table produced by a data source: " + + "functional.alltypes_datasource"); + + // Cannot ALTER TABLE REPLACE COLUMNS on an HBase table. + AnalysisError("alter table functional_hbase.alltypes replace columns (i int)", + "ALTER TABLE REPLACE COLUMNS not currently supported on HBase tables."); + + // Cannot ALTER TABLE REPLACE COLUMNS on an Kudu table. + AnalysisError("alter table functional_kudu.alltypes replace columns (i int)", + "ALTER TABLE REPLACE COLUMNS is not supported on Kudu tables."); } @Test diff --git a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java index 2d7ada8cb..24086cca6 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java @@ -1939,6 +1939,7 @@ public class AuthorizationStmtTest extends FrontendTestBase { @Test public void testAlterTable() throws ImpalaException { for (AuthzTest test: new AuthzTest[]{ + authorize("alter table functional.alltypes add column c1 int"), authorize("alter table functional.alltypes add columns(c1 int)"), authorize("alter table functional.alltypes replace columns(c1 int)"), authorize("alter table functional.alltypes change int_col c1 int"), diff --git a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java index 07041883d..2810890be 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java @@ -2122,9 +2122,24 @@ public class ParserTest extends FrontendTestBase { ParsesOk("CREATE AGGREGATE FUNCTION Foo(INT...) RETURNS INT LOCATION 'f.jar'"); } + @Test + public void TestAlterTableAddColumn() { + for (String keyword: new String[]{"", "IF NOT EXISTS"}) { + ParsesOk(String.format("ALTER TABLE Foo ADD COLUMN %s i int", keyword)); + ParsesOk(String.format("ALTER TABLE TestDb.Foo ADD COLUMN %s i int", keyword)); + + ParserError(String.format("ALTER TestDb.Foo ADD COLUMN %s", keyword)); + ParserError(String.format("ALTER Foo ADD COLUMN %s", keyword)); + ParserError(String.format("ALTER TABLE TestDb.Foo ADD COLUMN %s (i int)", keyword)); + ParserError(String.format("ALTER TABLE Foo ADD COLUMN %s (i int)", keyword)); + ParserError(String.format("ALTER Foo %s ADD COLUMN i int", keyword)); + ParserError(String.format("ALTER TestDb.Foo %s ADD COLUMN i int", keyword)); + } + } + @Test public void TestAlterTableAddReplaceColumns() { - String[] addReplaceKw = {"ADD", "REPLACE"}; + String[] addReplaceKw = {"ADD", "ADD IF NOT EXISTS", "REPLACE"}; for (String addReplace: addReplaceKw) { ParsesOk(String.format( "ALTER TABLE Foo %s COLUMNS (i int, s string)", addReplace)); @@ -2151,9 +2166,6 @@ public class ParserTest extends FrontendTestBase { ParserError(String.format("ALTER TestDb.Foo %s COLUMNS ()", addReplace)); ParserError(String.format("ALTER Foo %s COLUMNS (i int, s string)", addReplace)); ParserError(String.format("ALTER TABLE %s COLUMNS (i int, s string)", addReplace)); - // Don't yet support ALTER TABLE ADD COLUMN syntax - ParserError(String.format("ALTER TABLE Foo %s COLUMN i int", addReplace)); - ParserError(String.format("ALTER TABLE Foo %s COLUMN (i int)", addReplace)); } } diff --git a/testdata/workloads/functional-query/queries/QueryTest/alter-table.test b/testdata/workloads/functional-query/queries/QueryTest/alter-table.test index e6332d6ba..5d9e6cfef 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/alter-table.test +++ b/testdata/workloads/functional-query/queries/QueryTest/alter-table.test @@ -22,11 +22,65 @@ alter table t1 add columns (t tinyint, s string comment 'Str Col') string ==== ---- QUERY +# Add columns that already exist with "if not exists" clause. +alter table t1 add if not exists columns (t tinyint, s string comment 'Str Col') +---- RESULTS +'No new column(s) have been added to the table.' +---- TYPES +string +==== +---- QUERY +# Add columns that do not exist with "if not exists" clause. +alter table t1 add if not exists columns (t2 tinyint, s2 string comment 'Str Col') +---- RESULTS +'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 t1 add if not exists columns (t3 tinyint, s2 string comment 'Str Col') +---- RESULTS +'New column(s) have been added to the table.' +---- TYPES +string +==== +---- QUERY +# Add a new column that does not exist. +alter table t1 add column t4 tinyint +---- RESULTS +'New column(s) have been added to the table.' +---- TYPES +string +==== +---- QUERY +# Add a new column that does not exist with "if not exists" clause. +alter table t1 add column if not exists t5 tinyint +---- RESULTS +'New column(s) have been added to the table.' +---- TYPES +string +==== +---- QUERY +# Add a new column that already exists with "if not exists" clause. +alter table t1 add column if not exists t5 tinyint +---- RESULTS +'No new column(s) have been added to the table.' +---- TYPES +string +==== +---- QUERY describe t1 ---- RESULTS 'i','int','' 't','tinyint','' 's','string','Str Col' +'t2','tinyint','' +'s2','string','Str Col' +'t3','tinyint','' +'t4','tinyint','' +'t5','tinyint','' ---- TYPES string,string,string ==== @@ -78,6 +132,11 @@ describe t2 'i','int','' 't','tinyint','' 's','string','Str Col' +'t2','tinyint','' +'s2','string','Str Col' +'t3','tinyint','' +'t4','tinyint','' +'t5','tinyint','' ---- TYPES string,string,string ==== @@ -92,6 +151,11 @@ describe t2 ---- RESULTS 'i','int','' 's','string','Str Col' +'t2','tinyint','' +'s2','string','Str Col' +'t3','tinyint','' +'t4','tinyint','' +'t5','tinyint','' ---- TYPES string,string,string ==== diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test index a0d95d91a..0bed123ac 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test +++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test @@ -219,7 +219,7 @@ alter table tbl_to_alter add range partition 1 < values <= 20; alter table tbl_to_alter add columns (new_col1 int not null default 10, new_col2 bigint not null default 1000) ---- RESULTS -'Column has been added/replaced.' +'Column(s) have been added.' ==== ---- QUERY # Verify partition layout @@ -272,7 +272,7 @@ INT,STRING,BIGINT,INT,BIGINT # Add nullable columns: with and without a default alter table tbl_to_alter add columns (new_col3 string null, new_col4 int null default -1) ---- RESULTS -'Column has been added/replaced.' +'Column(s) have been added.' ==== ---- QUERY # Add a row diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test index f38b3c590..981a734b8 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test +++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_insert.test @@ -363,7 +363,7 @@ INT,INT,INT,INT,INT,INT,STRING,BOOLEAN,DECIMAL ---- QUERY alter table tbl_with_defaults add columns (j int null, k int not null default 10000) ---- RESULTS -'Column has been added/replaced.' +'Column(s) have been added.' ==== ---- QUERY select * from tbl_with_defaults