You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/03/03 02:01:10 UTC
[2/4] incubator-impala git commit: IMPALA-4616: Add missing Kudu
column options
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 <ma...@cloudera.com>
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 <mj...@cloudera.com>
Authored: Wed Mar 1 15:58:49 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
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