Dimitris Tsirogiannis has posted comments on this change. ( http://gerrit.cloudera.org:8080/9777 )
Change subject: IMPALA-4025: Part 1: Add percentile_disc aggregation function ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/AggregateInfo.java@99 PS4, Line 99: private ExprSubstitutionMap outputGroupingExprSmap_ = new ExprSubstitutionMap(); This seems like a subset of outputTupleSmap_. If it's just for checking whether an expr is bounded by grouping exprs, why can't we use outputTupleSmap_? http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/Analyzer.java@353 PS4, Line 353: boolean topStmtContainsPercentile() { return containsPercentile; } I haven't seen yet where this is used but I feel this is not correct. A stmt may have a hierarchy of analyzers each one corresponding to a select block and each one with or without a percentile function. So "topStmtContainsPercentile" in the context of single analyzer seems kind of off. http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@468 PS4, Line 468: mergeAggInputFn_.analyze(analyzer); Are you sure this is correct? Won't this call Expr.analyzer() which then subsequently calls FunctionCallExpr.analyzeImpl()? http://gerrit.cloudera.org:8080/#/c/9777/4/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/9777/4/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@63 PS4, Line 63: // slotRefs directly refer to table(s) in the from clause. Can you plz expand the comment a bit to describe why this is needed? http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java: http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1140 PS4, Line 1140: Those slotRefs Unfinished sentence http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1143 PS4, Line 1143: slotRef mention what this slotRef represents http://gerrit.cloudera.org:8080/#/c/9777/4/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@1163 PS4, Line 1163: + nit: add a space before '+' http://gerrit.cloudera.org:8080/#/c/9777/4/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/9777/4/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java@2124 PS4, Line 2124: "from functional.alltypesagg group by tinyint_col"); Can you also add a case with a WITH clause? -- To view, visit http://gerrit.cloudera.org:8080/9777 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacef7b3fcd74c4c73d88400ce27307c3baa0121e Gerrit-Change-Number: 9777 Gerrit-PatchSet: 4 Gerrit-Owner: Tianyi Wang <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Tianyi Wang <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Thu, 19 Apr 2018 21:07:05 +0000 Gerrit-HasComments: Yes
