impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4622: Add ALTER COLUMN statement.
Date Thu, 15 Jun 2017 23:18:22 GMT
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 <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