Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11883 )
Change subject: IMPALA-7808: Refactor Analyzer for easier debugging ...................................................................... Patch Set 2: (12 comments) Thanks for the reviews. Addressed the comments. If you are ready, I'll go ahead and shift the indent of the methods in the inner class, which will mark a zillion likes as changed. In its current form, you can more easily see the actual changes. http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG@37 PS1, Line 37: Reran a > pls move this testing section above the Change-Id Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461 PS1, Line 461: r > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461 PS1, Line 461: eturn; > We generally do not use braces in one line "if (cond) stmt;". Thanks. Every project has its quirks; I 'm still learning Impala's preferences. Fixed. http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java: http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@176 PS1, Line 176: private final Analyzer analyzer_; > these members should be suffixed with "_" Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@186 PS1, Line 186: Note tem > note, you can always include such review-level comments as comments in gerr Agreed. The comment is here to rather force the need to come back and shift indentation before the final +2. At that point, the comment will be removed. http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@219 PS1, Line 219: if (multiAggInfo_ != null && multiAggInfo_.hasAggregateExprs()) { > here and at many other functions: in Impala we generally do not add an empt Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@389 PS1, Line 389: if (t > did some other class need this protected? Tried to minimize extra changes. It works to make this private, so went ahead and did so. http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@526 PS1, Line 526: c > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@617 PS1, Line 617: su > why public? Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@659 PS1, Line 659: > why public? Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@757 PS1, Line 757: } > why public? Done http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@894 PS1, Line 894: > just to remind myelf... end of inner class. Yes. When you are satisfied that nothing changed other than the inner class, I'll go ahead and indent the whole mess. -- To view, visit http://gerrit.cloudera.org:8080/11883 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52 Gerrit-Change-Number: 11883 Gerrit-PatchSet: 2 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Tue, 06 Nov 2018 20:44:33 +0000 Gerrit-HasComments: Yes
