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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
