Alex Behm has posted comments on this change. Change subject: IMPALA-3369: Add ALTER TABLE SET COLUMN STATS statement. ......................................................................
Patch Set 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/3189/2//COMMIT_MSG Commit Message: Line 12: ALTER TABLE <tbl_name> SET COLUMN STATS <col_name> > [<db_name>.]<table_name> just to make it clearer. Done http://gerrit.cloudera.org:8080/#/c/3189/2/fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetColumnStats.java File fe/src/main/java/com/cloudera/impala/analysis/AlterTableSetColumnStats.java: Line 21: import org.kududb.client.shaded.com.google.common.collect.Maps; > Shouldn't this be imported directly from Guava rather than from shaded kudu Yes. Done. Line 43: public class AlterTableSetColumnStats extends AlterTableSetStmt { > - I think this should subclass AlterTableStmt directly as we don't seem to Done Line 102: setStatsValue(entry.getKey(), entry.getValue(), col, colStats_); > This is already seems to be a class variable, no need to pass to the method Correct, it is not necessary, but imo it is easier to reason about self-contained functions without relying on side-effects. Line 102: setStatsValue(entry.getKey(), entry.getValue(), col, colStats_); > Also should this be made a class variable, set in analyze() ? Don't think so. Less state is better. Line 118: if (col.getType().isFixedLengthType()) { > I think we can probably merge these two 'if', unless you wrote it this way Done Line 165: } > I think its cleaner to move this exists check after L101 to something like Wfm, but it's more code. Done. http://gerrit.cloudera.org:8080/#/c/3189/2/fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java File fe/src/test/java/com/cloudera/impala/analysis/AnalyzeDDLTest.java: Line 33: import org.kududb.client.shaded.com.google.common.base.Joiner; > Direct guava import Done -- To view, visit http://gerrit.cloudera.org:8080/3189 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I45cd8aa7241ea962788ba9ca7d0bbfd864c4304f Gerrit-PatchSet: 2 Gerrit-Project: Impala Gerrit-Branch: cdh5-2.6.0_5.8.0 Gerrit-Owner: Alex Behm <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-HasComments: Yes
