Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/14132 )
Change subject: IMPALA-7604: part 2: fixes for AggregationNode cardinality ...................................................................... Patch Set 6: Code-Review+1 (4 comments) Patch and the resulting test file changes make sense to me. I'll let Bikram do another pass if he has any comments. http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/main/java/org/apache/impala/analysis/Expr.java@789 PS6, Line 789: nit: Uses? http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java File fe/src/main/java/org/apache/impala/planner/AggregationNode.java: http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@210 PS6, Line 210: This is prone to overflow, nit: update? http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@259 PS6, Line 259: // Note that this will still be -1 if the child's cardinality is unknown. nit: Do we need some trace logging, just in case? http://gerrit.cloudera.org:8080/#/c/14132/6/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test File testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test: http://gerrit.cloudera.org:8080/#/c/14132/6/testdata/workloads/functional-planner/queries/PlannerTest/insert-sort-by.test@14 PS6, Line 14: HDFS Just curious why did this change everywhere? Someone forgot to update the test files in an unrelated change? -- To view, visit http://gerrit.cloudera.org:8080/14132 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ieed41d60c0e0dfeca64035e919cb8c28a054a9ab Gerrit-Change-Number: 14132 Gerrit-PatchSet: 6 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Comment-Date: Tue, 27 Aug 2019 20:52:56 +0000 Gerrit-HasComments: Yes
