impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
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:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/6955/4/fe/src/main/java/org/apache/impala/analysis/AlterTableChangeColStmt.java
File fe/src/main/java/org/apache/impala/analysis/AlterTableChangeColStmt.java:

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
" +
ALTER TABLE CHANGE/ALTER COLUMN


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


http://gerrit.cloudera.org:8080/#/c/6955/4/fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/KuduCatalogOpExecutor.java:

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)
alterColumn()


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.


http://gerrit.cloudera.org:8080/#/c/6955/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java:

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

Does it make sense to merge this into the existing tests for CHANGE COLUMN?


http://gerrit.cloudera.org:8080/#/c/6955/4/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
File fe/src/test/java/org/apache/impala/analysis/ParserTest.java:

Line 2358:   public void TestAlterColumn() {
TestAlterTableAlterColumn


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.


http://gerrit.cloudera.org:8080/#/c/6955/4/testdata/workloads/functional-query/queries/QueryTest/kudu_alter.test
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?


http://gerrit.cloudera.org:8080/#/c/6955/4/testdata/workloads/functional-query/queries/QueryTest/kudu_describe.test
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 http://gerrit.cloudera.org:8080/6955
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id2e8bd65342b79644a0fdcd925e6f17797e89ad6
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message