Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/16140 )
Change subject: IMPALA-9917: grouping() and grouping_id() support ...................................................................... Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/16140/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16140/7//COMMIT_MSG@28 PS7, Line 28: 27 and 36 sho > https://github.com/cwida/tpcds-result-reproduction/blob/master/answer_sets_ Added the rollup variants of those two with the same parameters as the references results and compared to those reference results. They were the same up to rounding and formatting differences. http://gerrit.cloudera.org:8080/#/c/16140/7/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/16140/7/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@2 PS7, Line 2: > nit: remove newline Done http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java File fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java: http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@352 PS7, Line 352: groupingIdExprs.add(aggExpr); > The groupingIdExprs list does not get used ? Done http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@394 PS7, Line 394: // special-casingthe degenerate case of these functions being used with a single > nit: add space between 'casing', 'the' Done http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@398 PS7, Line 398: throw new AnalysisException("Aggregate function '" + > It's probably ok to reject the query but strictly speaking, grouping() func I looked at this again and it's not too hard to implement by adding smap entries, so I just went and did it - might as well be complete. http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@586 PS7, Line 586: String name = aggExpr.getFnName().getFunction(); > 'name' variable does not get used. Done http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java@667 PS7, Line 667: * Return the return value for grouping() or grouping_id() for a particular aggregation > nit: remove the second 'return' Done http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java File fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java: http://gerrit.cloudera.org:8080/#/c/16140/7/fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java@150 PS7, Line 150: * analysis. > Mention in the comment that it's used for implementing grouping() as an exa Done http://gerrit.cloudera.org:8080/#/c/16140/7/testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test File testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test: http://gerrit.cloudera.org:8080/#/c/16140/7/testdata/workloads/functional-planner/queries/PlannerTest/grouping-sets.test@1169 PS7, Line 1169: # introduce NULLs. > nit: not clear what 'introduce nulls' means here. I think this was a fat finger error. Removed it. -- To view, visit http://gerrit.cloudera.org:8080/16140 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If0b1640d606256c0fe9204d2a21a8f6d06abcdb6 Gerrit-Change-Number: 16140 Gerrit-PatchSet: 7 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Aman Sinha <amsi...@cloudera.com> Gerrit-Reviewer: David Rorke <dro...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Shant Hovsepian <sh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Tue, 07 Jul 2020 18:44:58 +0000 Gerrit-HasComments: Yes
