[Impala-ASF-CR] IMPALA-7604: part 2: fixes for AggregationNode cardinality

["Bharath Vissapragada (Code Review)" <ge...@cloudera.org>]

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 <ta...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bh...@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bi...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Comment-Date: Tue, 27 Aug 2019 20:52:56 +0000
Gerrit-HasComments: Yes