You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jo...@apache.org on 2019/05/03 21:26:14 UTC
[impala] 04/04: IMPALA-5351: Support storing column comment of kudu
table
This is an automated email from the ASF dual-hosted git repository.
joemcdonnell pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git
commit 3fb36570ae0c7329cdbe3640515bc3e0cb066c81
Author: helifu <hz...@corp.netease.com>
AuthorDate: Tue Apr 9 21:22:14 2019 +0800
IMPALA-5351: Support storing column comment of kudu table
This patch intends to support storing column comment of kudu table
on impala side.
Belows tests passed:
1) creata kudu-table with column comment;
2) alter kudu-table with (add/alter[delete] column comment);
3) show create kudu table;
4) describe kudu-table;
5) invalidate metadata;
6) comment on column is { '' | null | 'comment' }
Change-Id: Ifb3b37eed364f12bdb3c1d7ef5be128f1475936c
Reviewed-on: http://gerrit.cloudera.org:8080/12977
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
.../impala/analysis/AlterTableAlterColStmt.java | 4 ---
.../java/org/apache/impala/catalog/KuduColumn.java | 7 ++--
.../apache/impala/service/CatalogOpExecutor.java | 22 ++++++++----
.../impala/service/KuduCatalogOpExecutor.java | 12 +++++--
.../org/apache/impala/analysis/AnalyzeDDLTest.java | 15 ++++----
.../queries/QueryTest/kudu_describe.test | 2 +-
tests/metadata/test_ddl.py | 42 ++++++++++++++++++++++
tests/metadata/test_ddl_base.py | 2 +-
tests/query_test/test_kudu.py | 15 ++++++--
9 files changed, 96 insertions(+), 25 deletions(-)
diff --git a/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java b/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
index 6b43bfd..5e6b5cd 100644
--- a/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
+++ b/fe/src/main/java/org/apache/impala/analysis/AlterTableAlterColStmt.java
@@ -165,10 +165,6 @@ public class AlterTableAlterColStmt extends AlterTableStmt {
"Cannot %s default value for primary key column '%s'",
isDropDefault_ ? "drop" : "set", colName_));
}
- if (newColDef_.getComment() != null) {
- // IMPALA-5351
- throw new AnalysisException("Kudu does not support column comments.");
- }
if (newColDef_.isPrimaryKey()) {
throw new AnalysisException(
"Altering a column to be a primary key is not supported.");
diff --git a/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java b/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java
index a4b8e5f..246b0e3 100644
--- a/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java
+++ b/fe/src/main/java/org/apache/impala/catalog/KuduColumn.java
@@ -85,10 +85,11 @@ public class KuduColumn extends Column {
}
Preconditions.checkNotNull(defaultValueExpr);
}
+ String comment = !colSchema.getComment().isEmpty() ? colSchema.getComment() : null;
return new KuduColumn(colSchema.getName(), type, colSchema.isKey(),
colSchema.isNullable(), colSchema.getEncoding(),
colSchema.getCompressionAlgorithm(), defaultValueExpr,
- colSchema.getDesiredBlockSize(), null, position);
+ colSchema.getDesiredBlockSize(), comment, position);
}
public static KuduColumn fromThrift(TColumn column, int position)
@@ -111,8 +112,10 @@ public class KuduColumn extends Column {
}
int blockSize = 0;
if (column.isSetBlock_size()) blockSize = column.getBlock_size();
+ String comment = (column.isSetComment() && !column.getComment().isEmpty()) ?
+ column.getComment() : null;
return new KuduColumn(column.getKudu_column_name(), columnType, column.isIs_key(),
- column.isIs_nullable(), encoding, compression, defaultValue, blockSize, null,
+ column.isIs_nullable(), encoding, compression, defaultValue, blockSize, comment,
position);
}
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 3bf2434..80859fa 100644
--- a/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
@@ -4021,15 +4021,23 @@ public class CatalogOpExecutor {
try {
long newCatalogVersion = catalog_.incrementAndGetCatalogVersion();
catalog_.getLock().writeLock().unlock();
- org.apache.hadoop.hive.metastore.api.Table msTbl =
- tbl.getMetaStoreTable().deepCopy();
- if (!updateColumnComment(msTbl.getSd().getColsIterator(), columnName, comment)) {
- if (!updateColumnComment(msTbl.getPartitionKeysIterator(), columnName, comment)) {
- throw new ColumnNotFoundException(String.format(
- "Column name %s not found in table %s.", columnName, tbl.getFullName()));
+ if (tbl instanceof KuduTable) {
+ TColumn new_col = new TColumn(columnName,
+ tbl.getColumn(columnName).getType().toThrift());
+ new_col.setComment(comment != null ? comment : "");
+ KuduCatalogOpExecutor.alterColumn((KuduTable) tbl, columnName, new_col);
+ } else {
+ org.apache.hadoop.hive.metastore.api.Table msTbl =
+ tbl.getMetaStoreTable().deepCopy();
+ if (!updateColumnComment(msTbl.getSd().getColsIterator(), columnName, comment)) {
+ if (!updateColumnComment(msTbl.getPartitionKeysIterator(), columnName,
+ comment)) {
+ throw new ColumnNotFoundException(String.format(
+ "Column name %s not found in table %s.", columnName, tbl.getFullName()));
+ }
}
+ applyAlterTable(msTbl, true);
}
- applyAlterTable(msTbl, true);
loadTableMetadata(tbl, newCatalogVersion, false, true, null);
addTableToCatalogUpdate(tbl, response.result);
addSummary(response, "Column has been altered.");
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 a7fb142..f4477b4 100644
--- a/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
+++ b/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
@@ -131,6 +131,9 @@ public class KuduCatalogOpExecutor {
csb.typeAttributes(
DecimalUtil.typeAttributes(type.getPrecision(), type.getDecimalDigits()));
}
+ if (column.isSetComment() && !column.getComment().isEmpty()) {
+ csb.comment(column.getComment());
+ }
return csb.build();
}
@@ -292,7 +295,10 @@ public class KuduCatalogOpExecutor {
}
Type type =
KuduUtil.toImpalaType(colSchema.getType(), colSchema.getTypeAttributes());
- cols.add(new FieldSchema(colSchema.getName(), type.toSql().toLowerCase(), null));
+ String comment =
+ !colSchema.getComment().isEmpty() ? colSchema.getComment() : null;
+ cols.add(new FieldSchema(colSchema.getName(), type.toSql().toLowerCase(),
+ comment));
}
} catch (Exception e) {
throw new ImpalaRuntimeException(String.format("Error loading schema of table " +
@@ -459,7 +465,6 @@ public class KuduCatalogOpExecutor {
throws ImpalaRuntimeException {
Preconditions.checkState(!Strings.isNullOrEmpty(colName));
Preconditions.checkNotNull(newCol);
- Preconditions.checkState(!newCol.isSetComment());
Preconditions.checkState(!newCol.isIs_key());
Preconditions.checkState(!newCol.isSetIs_nullable());
if (LOG.isTraceEnabled()) {
@@ -495,6 +500,9 @@ public class KuduCatalogOpExecutor {
if (!newColName.toLowerCase().equals(colName.toLowerCase())) {
alterTableOptions.renameColumn(kuduColName, newColName);
}
+ if (newCol.isSetComment()) {
+ alterTableOptions.changeComment(kuduColName, newCol.getComment());
+ }
String errMsg = String.format(
"Error altering column %s in Kudu table %s", colName, tbl.getName());
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 64c7a99..02d0b84 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
@@ -1479,6 +1479,7 @@ public class AnalyzeDDLTest extends FrontendTestBase {
"compression LZ4 encoding RLE");
AnalyzesOk("alter table functional.alltypes alter int_col set comment 'a'");
AnalyzesOk("alter table functional_kudu.alltypes alter int_col drop default");
+ AnalyzesOk("alter table functional_kudu.alltypes alter int_col set comment 'a'");
AnalysisError("alter table functional_kudu.alltypes alter id set default 0",
"Cannot set default value for primary key column 'id'");
@@ -1493,8 +1494,6 @@ public class AnalyzeDDLTest extends FrontendTestBase {
"Altering a column to be a primary key is not supported.");
AnalysisError("alter table functional_kudu.alltypes alter int_col set not null",
"Altering the nullability of a column is not supported.");
- AnalysisError("alter table functional_kudu.alltypes alter int_col set comment 'a'",
- "Kudu does not support column comments.");
AnalysisError("alter table functional.alltypes alter int_col set compression lz4",
"Unsupported column options for non-Kudu table: 'int_col INT COMPRESSION LZ4'");
AnalysisError("alter table functional.alltypes alter int_col drop default",
@@ -2604,14 +2603,13 @@ public class AnalyzeDDLTest extends FrontendTestBase {
// ALTER TABLE CHANGE COLUMN on Kudu tables
AnalyzesOk("alter table functional_kudu.testtbl change column name new_name string");
+ AnalyzesOk("alter table functional_kudu.testtbl change column zip " +
+ "zip int comment 'comment'");
// Unsupported column options
AnalysisError("alter table functional_kudu.testtbl change column zip zip_code int " +
"encoding rle compression lz4 default 90000", "Unsupported column options in " +
"ALTER TABLE CHANGE COLUMN statement: 'zip_code INT ENCODING RLE COMPRESSION " +
"LZ4 DEFAULT 90000'. Use ALTER TABLE ALTER COLUMN instead.");
- AnalysisError(
- "alter table functional_kudu.testtbl change column zip zip int comment 'comment'",
- "Kudu does not support column comments.");
// Changing the column type is not supported for Kudu tables
AnalysisError("alter table functional_kudu.testtbl change column zip zip bigint",
"Cannot change the type of a Kudu column using an ALTER TABLE CHANGE COLUMN " +
@@ -3019,6 +3017,10 @@ public class AnalyzeDDLTest extends FrontendTestBase {
"ts timestamp not null default '2009-1 foo') " +
"partition by hash(id) partitions 3 stored as kudu",
"String '2009-1 foo' cannot be cast to a TIMESTAMP literal.");
+
+ // Test column comments.
+ AnalyzesOk("create table tab (x int comment 'x', y int comment 'y', " +
+ "primary key (x, y)) stored as kudu");
}
@Test
@@ -4325,7 +4327,8 @@ public class AnalyzeDDLTest extends FrontendTestBase {
new Pair<>("functional.alltypes.id", createAnalysisCtx()),
new Pair<>("alltypes.id", createAnalysisCtx("functional")),
new Pair<>("functional.alltypes_view.id", createAnalysisCtx()),
- new Pair<>("alltypes_view.id", createAnalysisCtx("functional"))}) {
+ new Pair<>("alltypes_view.id", createAnalysisCtx("functional")),
+ new Pair<>("functional_kudu.alltypes.id", createAnalysisCtx())}) {
AnalyzesOk(String.format("comment on column %s is 'comment'", pair.first),
pair.second);
AnalyzesOk(String.format("comment on column %s is ''", pair.first), pair.second);
diff --git a/testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test b/testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test
index daaa18f..8b8e80f 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test
@@ -39,7 +39,7 @@ NAME,TYPE,COMMENT,PRIMARY_KEY,NULLABLE,DEFAULT_VALUE,ENCODING,COMPRESSION,BLOCK_
'pk1','int','','true','false','','AUTO_ENCODING','DEFAULT_COMPRESSION','0'
'pk2','int','','true','false','','AUTO_ENCODING','DEFAULT_COMPRESSION','0'
'pk3','string','','true','false','','AUTO_ENCODING','DEFAULT_COMPRESSION','0'
-'c1','string','','false','true','abc','AUTO_ENCODING','DEFAULT_COMPRESSION','0'
+'c1','string','testing','false','true','abc','AUTO_ENCODING','DEFAULT_COMPRESSION','0'
'c2','int','','false','false','100','PLAIN_ENCODING','SNAPPY','0'
'c3','int','','false','true','','AUTO_ENCODING','DEFAULT_COMPRESSION','8388608'
---- TYPES
diff --git a/tests/metadata/test_ddl.py b/tests/metadata/test_ddl.py
index c8871c5..05d8c01 100644
--- a/tests/metadata/test_ddl.py
+++ b/tests/metadata/test_ddl.py
@@ -728,6 +728,48 @@ class TestDdlStatements(TestDdlBase):
self.client, "show create table {0}".format(orc_table))
assert any("ORC" in x for x in result.data)
+ def test_kudu_column_comment(self, vector, unique_database):
+ table = "{0}.kudu_table0".format(unique_database)
+ self.client.execute("create table {0}(x int comment 'x' primary key) \
+ stored as kudu".format(table))
+ comment = self._get_column_comment(table, 'x')
+ assert "x" == comment
+
+ table = "{0}.kudu_table".format(unique_database)
+ self.client.execute("create table {0}(i int primary key) stored as kudu"
+ .format(table))
+ comment = self._get_column_comment(table, 'i')
+ assert "" == comment
+
+ self.client.execute("comment on column {0}.i is 'comment1'".format(table))
+ comment = self._get_column_comment(table, 'i')
+ assert "comment1" == comment
+
+ self.client.execute("comment on column {0}.i is ''".format(table))
+ comment = self._get_column_comment(table, 'i')
+ assert "" == comment
+
+ self.client.execute("comment on column {0}.i is 'comment2'".format(table))
+ comment = self._get_column_comment(table, 'i')
+ assert "comment2" == comment
+
+ self.client.execute("comment on column {0}.i is null".format(table))
+ comment = self._get_column_comment(table, 'i')
+ assert "" == comment
+
+ self.client.execute("alter table {0} alter column i set comment 'comment3'"
+ .format(table))
+ comment = self._get_column_comment(table, 'i')
+ assert "comment3" == comment
+
+ self.client.execute("alter table {0} alter column i set comment ''".format(table))
+ comment = self._get_column_comment(table, 'i')
+ assert "" == comment
+
+ self.client.execute("alter table {0} add columns (j int comment 'comment4')"
+ .format(table))
+ comment = self._get_column_comment(table, 'j')
+ assert "comment4" == comment
# IMPALA-2002: Tests repeated adding/dropping of .jar and .so in the lib cache.
class TestLibCache(TestDdlBase):
diff --git a/tests/metadata/test_ddl_base.py b/tests/metadata/test_ddl_base.py
index a27aa1c..83399b0 100644
--- a/tests/metadata/test_ddl_base.py
+++ b/tests/metadata/test_ddl_base.py
@@ -119,7 +119,7 @@ class TestDdlBase(ImpalaTestSuite):
comments = dict()
for row in result.data:
cols = row.split('\t')
- if len(cols) == 3:
+ if len(cols) <= 9:
comments[cols[0].rstrip()] = cols[2].rstrip()
return comments.get(col_name)
diff --git a/tests/query_test/test_kudu.py b/tests/query_test/test_kudu.py
index 67ba8e2..776486c 100644
--- a/tests/query_test/test_kudu.py
+++ b/tests/query_test/test_kudu.py
@@ -776,6 +776,7 @@ class TestCreateExternalTable(KuduTestSuite):
kudu_client.delete_table(table_name)
class TestShowCreateTable(KuduTestSuite):
+ column_properties = "ENCODING AUTO_ENCODING COMPRESSION DEFAULT_COMPRESSION"
def assert_show_create_equals(self, cursor, create_sql, show_create_sql):
"""Executes 'create_sql' to create a table, then runs "SHOW CREATE TABLE" and checks
@@ -790,7 +791,6 @@ class TestShowCreateTable(KuduTestSuite):
textwrap.dedent(show_create_sql.format(**format_args)).strip()
def test_primary_key_and_distribution(self, cursor):
- # TODO: Add test cases with column comments once KUDU-1711 is fixed.
# TODO: Add case with BLOCK_SIZE
self.assert_show_create_equals(cursor,
"""
@@ -877,7 +877,18 @@ class TestShowCreateTable(KuduTestSuite):
STORED AS KUDU
TBLPROPERTIES ('kudu.master_addresses'='{kudu_addr}')""".format(
db=cursor.conn.db_name, kudu_addr=KUDU_MASTER_HOSTS))
-
+ self.assert_show_create_equals(cursor,
+ """
+ CREATE TABLE {table} (c INT COMMENT 'Ab 1@' PRIMARY KEY) STORED AS KUDU""",
+ """
+ CREATE TABLE {db}.{{table}} (
+ c INT NOT NULL {p} COMMENT 'Ab 1@',
+ PRIMARY KEY (c)
+ )
+ STORED AS KUDU
+ TBLPROPERTIES ('kudu.master_addresses'='{kudu_addr}')""".format(
+ db=cursor.conn.db_name, p=self.column_properties,
+ kudu_addr=KUDU_MASTER_HOSTS))
def test_timestamp_default_value(self, cursor):
create_sql_fmt = """