Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-4622: Add ALTER COLUMN statement. ......................................................................
Patch Set 5: (18 comments) > We should add an e2e test that exhaustively tests all combinations > of column type, encoding and compression. In each case, we can do > something very simple like insert one row and then do a select. Done 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 > We should move in the direction of making ALTER TABLE ALTER COLUMN the defa Done Line 56 > Why is it ok to use a NullLiteral for this purpose? Isn't NULL a legitimate The behavior is the same either way - if the column is nullable, it gets the value NULL when no value is specified either if its default is NULL or if it doesn't have a default. It the column is not nullable, there's an error when no value is specified either way. Kudu actually disallows setting a default value of NULL and requires that you use 'dropDefault' to achieve the effect, though we're allowing both and then translating 'set default null' to 'dropDefault' in KuduCatalogOpExecutor. It does seem a little weird to allow you to set a default value of NULL on a non-nullable column, but I'm following Postgres's example here and above. Line 88 > Remove this TODO, seems useless Done Line 91 > ALTER TABLE CHANGE/ALTER COLUMN Done Line 97 > So weird. Can you clean this up while you are here? Feels easier to do thes Done Line 126 > Somewhat redundant with the checks in L97. Can you clean it up? Done Line 146 > Mention that the new stmt should be used Done 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 and options specified in > I thought it was not possible to change the type? Done Line 402: * > alterColumn() Done Line 440: alterTableOptions.changeEncoding( > We should display the existing column name, not the new name. Done Line 441: newColName, KuduUtil.fromThrift(newCol.getEncoding())); > Does this alteration apply to new data being added to Kudu, or does this op It doesn't rewrite column data as part of the alter operation. The new encoding/compression/block size is applied to new rowsets when they are flushed to disk, and possibly eventually to old rowsets if they get compacted into new rowsets depending on cost based decisions that Kudu makes. I've noted this in the method comment, and added some trace logging as we do elsewhere in this file. 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 TestAlterTableAlterColumn() throws AnalysisException { > TestAlterTableAlterColumn() for consistency Done 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 TestAlterTableAlterColumn() { > TestAlterTableAlterColumn Done Line 2359: for (String column : Lists.newArrayList("", "COLUMN")) { > By the way, the Postgres ALTER TABLE ALTER COLUMN allows changing multiple I filed: IMPALA-5521 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 - I've disallowed defaults for pk columns. This is covered in AnalyzeDDLTest now. - You can set a default value of null for non-nullable columns (its equivalent to dropping the default). As mentioned elsewhere, this seems weird but its what Postgres does. Tested here now. - Yes, tested here now. 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? Done 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 > seems more appropriate to have this in kudu_alter.test Done Line 51 > Make sure that an insert works and that the values can be scanned. Done -- 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: 5 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
