Aman Sinha has posted comments on this change. ( http://gerrit.cloudera.org:8080/16140 )
Change subject: IMPALA-9917: grouping() and grouping_id() support ...................................................................... Patch Set 7: (7 comments) Mostly minor comments. 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 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 ? 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' 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() function can occur with just a simple GROUP BY also without grouping sets. Hive actually supports it. All grouping() values show up as 0 in that case. 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. 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' 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. -- 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 07:02:02 +0000 Gerrit-HasComments: Yes
