Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16112 )
Change subject: IMPALA-9897: GROUPING SETS/CUBE/ROLLUP parsing and analysis ...................................................................... Patch Set 13: (11 comments) http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java File fe/src/main/java/org/apache/impala/analysis/GroupByClause.java: http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@66 PS13, Line 66: > nit: 4 spaces identation Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@94 PS13, Line 94: e.clone() > Could you leave a comment for doing clone here? Oh this shouldn't be needed. http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@125 PS13, Line 125: if (groupingIDs_.size() >= GROUPING_SET_LIMIT) { : throw new AnalysisException("Limit of " + GROUPING_SET_LIMIT + : " grouping sets exceeded"); : } > Could you add a test for this? Done - in AnalyzeStmtsTest http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@173 PS13, Line 173: analyzer > nit: unused parameter. The SelectStmt parameter is also not used. Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@174 PS13, Line 174: > nit: 4 spaces indentation Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@175 PS13, Line 175: throws AnalysisException { > nit: this can be merged to the above line Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@218 PS13, Line 218: if (groupingSetsType_ == GroupingSetsType.ROLLUP) { > nit: could you merge this to the above if-statement as an else-if clause? Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@233 PS13, Line 233: : if (groupingSetsType_ == GroupingSetsType.SETS) { > nit: could you merge this to the above if-statement as an else-if clause? Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/main/java/org/apache/impala/analysis/GroupByClause.java@235 PS13, Line 235: for (int setNum = 0; setNum < groupingSetsList_.size(); setNum++) { : List<Integer> set = groupingSetsList_.get(setNum); > nit: can be simlified to Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java: http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2459 PS13, Line 2459: > nit: blank line Done http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java: http://gerrit.cloudera.org:8080/#/c/16112/13/fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java@75 PS13, Line 75: > We'd better add an @AfterClass cleanUp method to reset the env to avoid fla I think this was done indirectly by AbstractFrontendTest and the FrontendFixture, but what you're proposing would make this more obviously correct without having to read a bunch of other code. So I made the change. -- To view, visit http://gerrit.cloudera.org:8080/16112 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I178e45de94d736630c97ae1ec4a92423cd74621f Gerrit-Change-Number: 16112 Gerrit-PatchSet: 13 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Aman Sinha <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Shant Hovsepian <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 06 Jul 2020 17:19:36 +0000 Gerrit-HasComments: Yes
