Quanlong Huang 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 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? 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? 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. 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 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 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? 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? 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 for (List<Integer> set : groupingSetsList_) 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 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 flakiness like IMPALA-9743 when running all FE tests in a JVM. Some tests (e.g. CatalogTest) require isTestEnv = False. Ideally all tests should set their required flags (IMPALA-9780). Currently, we'd better reset these flags at the end. -- 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 09:07:29 +0000 Gerrit-HasComments: Yes
