impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4622: Add ALTER COLUMN statement.
Date Tue, 06 Jun 2017 16:51:43 GMT
Matthew Jacobs has posted comments on this change.

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


Patch Set 3:

(4 comments)

Nice patch, though I still have a feeling the AlterColumnStmt/AlterTableChangeColStmt thing
could be made a bit more clear.

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

It feels like there should be more shared with AlterTableChangeColStmt, e.g. maybe making
this a subclass or removing this and creating more static constructors with more explicit
behaviors in AlterTableChangeColStmt. My main issue isn't about code reuse but rather overlapping
concepts with unrelated classes.


PS3, Line 36: Sent to the catalog as a CHANGE
            :  * column operation where the column name and type are unchanged.
rather than put this implementation detail here, it'd be better to say what this statement
is used for, i.e. sets properties on an existing column.


PS3, Line 43: createDropDefault
comment; maybe createDropDefaultStmt to be a bit more clear


PS3, Line 75: String.format(
not necessary


-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message