Bharath Vissapragada has posted comments on this change. ( http://gerrit.cloudera.org:8080/12136 )
Change subject: IMPALA-8021: Add estimated cardinality to EXPLAIN output ...................................................................... Patch Set 12: (4 comments) lgtm, just a few nits. Also there are some warnings from clang-tidy bot that you might want to fix. http://gerrit.cloudera.org:8080/#/c/12136/12/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java: http://gerrit.cloudera.org:8080/#/c/12136/12/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1230 PS12, Line 1230: .append(String.format("partition predicates: %s\n", Looks like Greg had a different opinion on the jira when I mentioned separating the two sets of conjuncts. Appears he is not strong though. http://gerrit.cloudera.org:8080/#/c/12136/12/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java: http://gerrit.cloudera.org:8080/#/c/12136/12/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@1331 PS12, Line 1331: scanNode.setPartitionConjuncts(pair.second); nit: Can we force this in the c'tor above? Setting it separately seems weird. http://gerrit.cloudera.org:8080/#/c/12136/12/fe/src/test/java/org/apache/impala/testutil/TestUtils.java File fe/src/test/java/org/apache/impala/testutil/TestUtils.java: http://gerrit.cloudera.org:8080/#/c/12136/12/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@207 PS12, Line 207: If found, then parse the values and compare them. Report a match : * if the values are within 5%, else report a mismatch. Is this because of the loss of information during pretty printing and then decoding it back? If so, probably worth mentioning it. http://gerrit.cloudera.org:8080/#/c/12136/12/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@226 PS12, Line 226: SKIP you mean DIFF? (same below) -- To view, visit http://gerrit.cloudera.org:8080/12136 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie9aa2d715b04cbb279aaffec8c5692686562d986 Gerrit-Change-Number: 12136 Gerrit-PatchSet: 12 Gerrit-Owner: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <prog...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Sat, 05 Jan 2019 00:35:13 +0000 Gerrit-HasComments: Yes
