[Impala-ASF-CR] IMPALA-8021: Add estimated cardinality to EXPLAIN output

["Tim Armstrong (Code Review)" <ge...@cloudera.org>]

Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12136 )

Change subject: IMPALA-8021: Add estimated cardinality to EXPLAIN output
......................................................................


Patch Set 4:

(5 comments)

Thanks for doing this, this needed a bit of love

http://gerrit.cloudera.org:8080/#/c/12136/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java
File fe/src/main/java/org/apache/impala/planner/PlanNode.java:

http://gerrit.cloudera.org:8080/#/c/12136/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@412
PS4, Line 412: explainCardinality
I'd prefer appendExplainCardinality() or appendCardinality() - usually the verb makes it clear what it's mutating.


http://gerrit.cloudera.org:8080/#/c/12136/4/fe/src/main/java/org/apache/impala/planner/PlanNode.java@413
PS4, Line 413:     expBuilder.append("row-size=")
I sort-of feel that row-size isn't necessary for STANDARD explain level - it's relatively low level. I don't feel particularly strongly though.


http://gerrit.cloudera.org:8080/#/c/12136/4/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/4/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@157
PS4, Line 157:      * @return {@link VerifyResult#SKIP} if field not one covered by this rule,
Something slightly non-obvious is that if all rules SKIP, then it's treated as a MATCH by the validation code.


http://gerrit.cloudera.org:8080/#/c/12136/4/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@229
PS4, Line 229: delta
name seems misleading since it's the ratio, not the difference


http://gerrit.cloudera.org:8080/#/c/12136/4/fe/src/test/java/org/apache/impala/testutil/TestUtils.java@230
PS4, Line 230: 095
shouldn't this be 0.95?



-- 
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: 4
Gerrit-Owner: Paul Rogers <pr...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Jan 2019 00:59:04 +0000
Gerrit-HasComments: Yes