impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-4622: Add ALTER COLUMN statement.
Date Wed, 14 Jun 2017 21:33:09 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4622: Add ALTER COLUMN statement.

Patch Set 4:

File fe/src/main/java/org/apache/impala/analysis/

Line 44: public class AlterTableChangeColStmt extends AlterTableStmt {
We should move in the direction of making ALTER TABLE ALTER COLUMN the default. In that sense,
I think we should rename this class to AlterTableAlterColumn and make corresponding renames
everywhere else. It's fine if you prefer to do the renames in a follow-on patch.

The new AlterTableAlterColumn should have a static createChangeColStmt() to make it clear
we are creating one of those.

Line 56:     option.put(ColumnDef.Option.DEFAULT, new NullLiteral());
Why is it ok to use a NullLiteral for this purpose? Isn't NULL a legitimate default value
that a user might want to set via ALTER TABLE ALTER COLUMN SET?

Line 88:     // TODO: Support column-level DDL on HBase tables. Requires updating the column
Remove this TODO, seems useless

Line 91:       throw new AnalysisException("ALTER TABLE CHANGE COLUMN not currently supported
" +

Line 97:     for (FieldSchema fs: t.getMetaStoreTable().getPartitionKeys()) {
So weird. Can you clean this up while you are here? Feels easier to do these checks using
Impala's Table/Column classes.

Line 126:     if (!colName_.toLowerCase().equals(newColDef_.getColName().toLowerCase()) &&
Somewhat redundant with the checks in L97. Can you clean it up?

Line 146:             "COLUMN statement: " + newColDef_.toString());
Mention that the new stmt should be used
File fe/src/main/java/org/apache/impala/service/

Line 399:    * Updates the column matching 'colName' to have the name, type, and options specified
I thought it was not possible to change the type?

Line 402:   public static void changeColumn(KuduTable tbl, String colName, TColumn newCol)

Line 440:         newCol.getColumnName(), tbl.getName());
We should display the existing column name, not the new name.

Line 441:     alterKuduTable(tbl, alterTableOptions, errMsg);
Does this alteration apply to new data being added to Kudu, or does this operation rewrite
all the column data in the new encoding/compression? We need to be careful with expensive
operations, maybe we can avoid holding the table lock, we should add logging for debugging
purposes, etc.
File fe/src/test/java/org/apache/impala/analysis/

Line 1133:   public void TestAlterColumn() throws AnalysisException {
TestAlterTableAlterColumn() for consistency

Does it make sense to merge this into the existing tests for CHANGE COLUMN?
File fe/src/test/java/org/apache/impala/analysis/

Line 2358:   public void TestAlterColumn() {

Line 2359:     for (String column : Lists.newArrayList("", "COLUMN")) {
By the way, the Postgres ALTER TABLE ALTER COLUMN allows changing multiple columns in the
same stmt. No need to add that now, just wanted to make you aware.
File testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test:

Line 459: # add a default to a row that didn't already have one
* What happens when adding/dropping a default value to a PK column altering (seems like we
should catch that in analysis)
* Can you alter a non-nullable column to have a default value of NULL?
* Can you change the compression/encoding of PK columns?

Line 462: alter table kudu_tbl_to_alter alter column new_col1 set default 10 + 5;
can you set the default back to NULL later?
File testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test:

Line 49: alter table describe_test alter column c3
seems more appropriate to have this in kudu_alter.test

Line 51: describe describe_test;
Make sure that an insert works and that the values can be scanned.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2e8bd65342b79644a0fdcd925e6f17797e89ad6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-HasComments: Yes

View raw message