Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14132 )
Change subject: IMPALA-7604: part 2: fixes for AggregationNode cardinality ...................................................................... Patch Set 7: (6 comments) PS8 is a rebase onto the latest part 1 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: s > nit: Uses? Done http://gerrit.cloudera.org:8080/#/c/14132/3/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/3/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@254 PS3, Line 254: } Need to fix references to cardinality_ here. 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: > nit: update? Done http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/main/java/org/apache/impala/planner/AggregationNode.java@259 PS6, Line 259: LOG.trace("Node " + id_ + " numGroups= " + numGroups + " aggInputCardinality=" + > nit: Do we need some trace logging, just in case? Done http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java File fe/src/test/java/org/apache/impala/planner/CardinalityTest.java: http://gerrit.cloudera.org:8080/#/c/14132/6/fe/src/test/java/org/apache/impala/planner/CardinalityTest.java@369 PS6, Line 369: 12760 > I am not really familiar with the file size estimation logic, but I have no The approx bit is meant to guard against that. Although I'm realising now that I should be checking exact cardinality for the tests with stats. 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 t It was deliberate - this prefix is ignored by the test validator. Otherwise adding the prefix would've required updating all of the planner test files and generating a bunch of noise. -- 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: 7 Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Tue, 27 Aug 2019 23:54:48 +0000 Gerrit-HasComments: Yes
