Bharath Vissapragada 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: <tbl_name> [<db_name>.]<table_name> just to make it clearer. 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 jar? Eclipse error? Line 43: AlterTableSetStmt - I think this should subclass AlterTableStmt directly as we don't seem to be using any specific fields (partitionSpec_) of AlterTableSetStmt and also not calling super.analyze(). - Once we sub-class AlterTableStmt, we can also call super.analyze() and clean up the anlayze() method here. What do you think? Line 102: colStats_ This is already seems to be a class variable, no need to pass to the method? Line 102: col Also should this be made a class variable, set in analyze() ? Line 116: if (statsKey.equalsIgnoreCase(AVG_SIZE_KEY) || : statsKey.equalsIgnoreCase(MAX_SIZE_KEY)) { : if (col.getType().isFixedLengthType()) { I think we can probably merge these two 'if', unless you wrote it this way so that the code is easy to read and reflects the description above. Line 161: else { : throw new AnalysisException( : String.format("Invalid column stats key: %s\nValid keys are: %s", : statsKey, Joiner.on(',').join(ALL_STATS_KEYS))); : } I think its cleaner to move this exists check after L101 to something like for (Map.Entry<String, String> entry: statsMap_.entrySet()) { if (!ALL_STATS_KEYS.contains(entry.getKey()) { throw new AnalysisException... } setStatsValue(entry.getKey(), entry.getValue()); } 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 -- 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: Bharath Vissapragada <[email protected]> Gerrit-HasComments: Yes
