IMPALA-4616: Add missing Kudu column options Adds support for missing Kudu column options in ALTER TABLE (it was there in CREATE TABLE already): * encoding * compression * block_size
Also adds support for adding nullable columns with default values. All of the above was not originally implemented due to limitations in the Kudu client, but have since been fixed: KUDU-1746, KUDU-1747 Testing: Updates and adds relevant test cases. Change-Id: I96a0fce7f6bc0c086b259d3119daa72c94b0af7b Reviewed-on: http://gerrit.cloudera.org:8080/6220 Reviewed-by: Marcel Kornacker <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/3a2a380c Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/3a2a380c Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/3a2a380c Branch: refs/heads/master Commit: 3a2a380cf73c55e55f4025c008c50accda056f85 Parents: 642b8f1 Author: Matthew Jacobs <[email protected]> Authored: Wed Mar 1 15:58:49 2017 -0800 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Mar 3 01:29:14 2017 +0000 ---------------------------------------------------------------------- .../analysis/AlterTableAddReplaceColsStmt.java | 9 +-- .../org/apache/impala/analysis/ColumnDef.java | 10 ++- .../org/apache/impala/analysis/TableDef.java | 2 +- .../impala/service/KuduCatalogOpExecutor.java | 84 ++++++++------------ .../apache/impala/analysis/AnalyzeDDLTest.java | 17 ++-- .../queries/QueryTest/kudu_alter.test | 56 ++++++------- 6 files changed, 78 insertions(+), 100 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java index 9b2d7c0..598b47f 100644 --- a/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java +++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableAddReplaceColsStmt.java @@ -120,12 +120,9 @@ public class AlterTableAddReplaceColsStmt extends AlterTableStmt { throw new AnalysisException("Cannot add a primary key using an ALTER TABLE " + "ADD COLUMNS statement: " + c.toString()); } - if (c.hasEncoding() || c.hasCompression() || c.hasBlockSize()) { - // Kudu API doesn't support specifying encoding, compression and desired - // block size on a newly added column (see KUDU-1746). - throw new AnalysisException("ENCODING, COMPRESSION and " + - "BLOCK_SIZE options cannot be specified in an ALTER TABLE ADD COLUMNS " + - "statement: " + c.toString()); + if (c.isExplicitNotNullable() && !c.hasDefaultValue()) { + throw new AnalysisException("A new non-null column must have a default " + + "value: " + c.toString()); } } else if (c.hasKuduOptions()) { throw new AnalysisException("The specified column options are only supported " + http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java b/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java index d7217f7..42304e6 100644 --- a/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java +++ b/fe/src/main/java/org/apache/impala/analysis/ColumnDef.java @@ -162,7 +162,15 @@ public class ColumnDef { public boolean hasCompression() { return compressionVal_ != null; } public boolean hasBlockSize() { return blockSize_ != null; } public boolean isNullabilitySet() { return isNullable_ != null; } - public boolean isNullable() { return isNullabilitySet() && isNullable_; } + + // True if the column was explicitly set to be nullable (may differ from the default + // behavior if not explicitly set). + public boolean isExplicitNullable() { return isNullabilitySet() && isNullable_; } + + // True if the column was explicitly set to be not nullable (may differ from the default + // behavior if not explicitly set). + public boolean isExplicitNotNullable() { return isNullabilitySet() && !isNullable_; } + public boolean hasDefaultValue() { return defaultValue_ != null; } public void analyze(Analyzer analyzer) throws AnalysisException { http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/analysis/TableDef.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/TableDef.java b/fe/src/main/java/org/apache/impala/analysis/TableDef.java index ae314c3..cad45b7 100644 --- a/fe/src/main/java/org/apache/impala/analysis/TableDef.java +++ b/fe/src/main/java/org/apache/impala/analysis/TableDef.java @@ -251,7 +251,7 @@ class TableDef { throw new AnalysisException(String.format( "PRIMARY KEY column '%s' does not exist in the table", colName)); } - if (colDef.isNullable()) { + if (colDef.isExplicitNullable()) { throw new AnalysisException("Primary key columns cannot be nullable: " + colDef.toString()); } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java index 6fc5674..f1a9135 100644 --- a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java +++ b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java @@ -91,6 +91,36 @@ public class KuduCatalogOpExecutor { } } + private static ColumnSchema createColumnSchema(TColumn column, boolean isKey) + throws ImpalaRuntimeException { + Type type = Type.fromThrift(column.getColumnType()); + Preconditions.checkState(type != null); + org.apache.kudu.Type kuduType = KuduUtil.fromImpalaType(type); + + ColumnSchemaBuilder csb = new ColumnSchemaBuilder(column.getColumnName(), kuduType); + csb.key(isKey); + if (column.isSetIs_nullable()) { + Preconditions.checkArgument(!isKey); + csb.nullable(column.isIs_nullable()); + } else { + // Non-key columns are by default nullable unless the user explicitly sets its + // nullability. Key columns are not nullable. + csb.nullable(!isKey); + } + if (column.isSetDefault_value()) { + csb.defaultValue(KuduUtil.getKuduDefaultValue(column.getDefault_value(), kuduType, + column.getColumnName())); + } + if (column.isSetBlock_size()) csb.desiredBlockSize(column.getBlock_size()); + if (column.isSetEncoding()) { + csb.encoding(KuduUtil.fromThrift(column.getEncoding())); + } + if (column.isSetCompression()) { + csb.compressionAlgorithm(KuduUtil.fromThrift(column.getCompression())); + } + return csb.build(); + } + /** * Creates the schema of a new Kudu table. */ @@ -100,33 +130,8 @@ public class KuduCatalogOpExecutor { Preconditions.checkState(!keyColNames.isEmpty()); List<ColumnSchema> colSchemas = new ArrayList<>(params.getColumnsSize()); for (TColumn column: params.getColumns()) { - Type type = Type.fromThrift(column.getColumnType()); - Preconditions.checkState(type != null); - org.apache.kudu.Type kuduType = KuduUtil.fromImpalaType(type); - // Create the actual column and check if the column is a key column - ColumnSchemaBuilder csb = - new ColumnSchemaBuilder(column.getColumnName(), kuduType); boolean isKey = keyColNames.contains(column.getColumnName()); - csb.key(isKey); - if (column.isSetIs_nullable()) { - csb.nullable(column.isIs_nullable()); - } else if (!isKey) { - // Non-key columns are by default nullable unless the user explicitly sets their - // nullability. - csb.nullable(true); - } - if (column.isSetDefault_value()) { - csb.defaultValue(KuduUtil.getKuduDefaultValue(column.getDefault_value(), kuduType, - column.getColumnName())); - } - if (column.isSetBlock_size()) csb.desiredBlockSize(column.getBlock_size()); - if (column.isSetEncoding()) { - csb.encoding(KuduUtil.fromThrift(column.getEncoding())); - } - if (column.isSetCompression()) { - csb.compressionAlgorithm(KuduUtil.fromThrift(column.getCompression())); - } - colSchemas.add(csb.build()); + colSchemas.add(createColumnSchema(column, isKey)); } return new Schema(colSchemas); } @@ -369,32 +374,7 @@ public class KuduCatalogOpExecutor { throws ImpalaRuntimeException { AlterTableOptions alterTableOptions = new AlterTableOptions(); for (TColumn column: columns) { - Type type = Type.fromThrift(column.getColumnType()); - Preconditions.checkState(type != null); - org.apache.kudu.Type kuduType = KuduUtil.fromImpalaType(type); - boolean isNullable = !column.isSetIs_nullable() ? true : column.isIs_nullable(); - if (isNullable) { - if (column.isSetDefault_value()) { - // See KUDU-1747 - throw new ImpalaRuntimeException(String.format("Error adding nullable " + - "column to Kudu table %s. Cannot specify a default value for a nullable " + - "column", tbl.getKuduTableName())); - } - alterTableOptions.addNullableColumn(column.getColumnName(), kuduType); - } else { - Object defaultValue = null; - if (column.isSetDefault_value()) { - defaultValue = KuduUtil.getKuduDefaultValue(column.getDefault_value(), kuduType, - column.getColumnName()); - } - try { - alterTableOptions.addColumn(column.getColumnName(), kuduType, defaultValue); - } catch (IllegalArgumentException e) { - // TODO: Remove this when KUDU-1747 is fixed - throw new ImpalaRuntimeException("Error adding non-nullable column to " + - "Kudu table " + tbl.getKuduTableName(), e); - } - } + alterTableOptions.addColumn(createColumnSchema(column, false)); } String errMsg = "Error adding columns to Kudu table " + tbl.getName(); alterKuduTable(tbl, alterTableOptions, errMsg); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java ---------------------------------------------------------------------- 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 c0703eb..757b857 100644 --- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java @@ -1896,17 +1896,16 @@ public class AnalyzeDDLTest extends FrontendTestBase { AnalysisError("alter table functional_kudu.testtbl add columns (a int primary key)", "Cannot add a primary key using an ALTER TABLE ADD COLUMNS statement: " + "a INT PRIMARY KEY"); - // Non-nullable columns require a default value + // Columns requiring a default value AnalyzesOk("alter table functional_kudu.testtbl add columns (a1 int not null " + "default 10)"); - // Unsupported column options - String[] unsupportedColOptions = {"encoding rle", "compression lz4", "block_size 10"}; - for (String colOption: unsupportedColOptions) { - AnalysisError(String.format("alter table functional_kudu.testtbl add columns " + - "(a1 int %s)", colOption), String.format("ENCODING, COMPRESSION and " + - "BLOCK_SIZE options cannot be specified in an ALTER TABLE ADD COLUMNS " + - "statement: a1 INT %s", colOption.toUpperCase())); - } + AnalyzesOk("alter table functional_kudu.testtbl add columns (a1 int null " + + "default 10)"); + // Other Kudu column options + 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)"); + // REPLACE columns is not supported for Kudu tables AnalysisError("alter table functional_kudu.testtbl replace columns (a int null)", "ALTER TABLE REPLACE COLUMNS is not supported on Kudu tables"); http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/3a2a380c/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test index f959a42..369eecf 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test +++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test @@ -258,56 +258,50 @@ ID, NAME, VALI, NEW_COL1, NEW_COL2 INT,STRING,BIGINT,INT,BIGINT ==== ---- QUERY -# Add a nullable column -alter table tbl_to_alter add columns (new_col3 string null) +# 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 ==== ---- QUERY # Add a row -insert into tbl_to_alter values ((4, 'test', 300, 1, 100, null), - (5, 'test', 400, 2, 200, 'names')) +insert into tbl_to_alter values ((4, 'test', 300, 1, 100, null, null), + (5, 'test', 400, 2, 200, 'names', 1)) ---- RUNTIME_PROFILE NumModifiedRows: 2 NumRowErrors: 0 ---- LABELS -ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3 +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4 ---- DML_RESULTS: tbl_to_alter -2,'test',100,1,100,'NULL' -3,'test',200,10,1000,'NULL' -4,'test',300,1,100,'NULL' -5,'test',400,2,200,'names' +2,'test',100,1,100,'NULL',-1 +3,'test',200,10,1000,'NULL',-1 +4,'test',300,1,100,'NULL',NULL +5,'test',400,2,200,'names',1 ---- TYPES -INT,STRING,BIGINT,INT,BIGINT,STRING +INT,STRING,BIGINT,INT,BIGINT,STRING,INT ==== ---- QUERY -# Add a row that doesn't have a value for the last added column +# Add a row that doesn't have a value for the last added columns insert into tbl_to_alter (id, name, vali, new_col1, new_col2) values (6, 'test', 500, 3, 300) ---- RUNTIME_PROFILE NumModifiedRows: 1 NumRowErrors: 0 ---- LABELS -ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3 +ID, NAME, VALI, NEW_COL1, NEW_COL2, NEW_COL3, NEW_COL4 ---- DML_RESULTS: tbl_to_alter -2,'test',100,1,100,'NULL' -3,'test',200,10,1000,'NULL' -4,'test',300,1,100,'NULL' -5,'test',400,2,200,'names' -6,'test',500,3,300,'NULL' +2,'test',100,1,100,'NULL',-1 +3,'test',200,10,1000,'NULL',-1 +4,'test',300,1,100,'NULL',NULL +5,'test',400,2,200,'names',1 +6,'test',500,3,300,'NULL',-1 ---- TYPES -INT,STRING,BIGINT,INT,BIGINT,STRING -==== ----- QUERY -# Add a nullable column with a default value -alter table tbl_to_alter add columns (invalid_col int null default 10) ----- CATCH -Error adding nullable column to Kudu table +INT,STRING,BIGINT,INT,BIGINT,STRING,INT ==== ---- QUERY # Add a non-nullable column without a default value alter table tbl_to_alter add columns (invalid_col int not null) ---- CATCH -Error adding non-nullable column to Kudu table +A new non-null column must have a default value ==== ---- QUERY # Drop a column @@ -318,13 +312,13 @@ alter table tbl_to_alter drop column vali # Retrieve table rows after column got dropped select * from tbl_to_alter ---- RESULTS -2,'test',1,100,'NULL' -3,'test',10,1000,'NULL' -4,'test',1,100,'NULL' -5,'test',2,200,'names' -6,'test',3,300,'NULL' +2,'test',1,100,'NULL',-1 +3,'test',10,1000,'NULL',-1 +4,'test',1,100,'NULL',NULL +5,'test',2,200,'names',1 +6,'test',3,300,'NULL',-1 ---- TYPES -INT,STRING,INT,BIGINT,STRING +INT,STRING,INT,BIGINT,STRING,INT ==== ---- QUERY # Try to drop a primary key column
