Marcel Kornacker has posted comments on this change. Change subject: IMPALA-2805: Order filters based on selectivity and cost ......................................................................
Patch Set 3: (6 comments) almost there. nice! http://gerrit.cloudera.org:8080/#/c/2598/3/fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java File fe/src/main/java/com/cloudera/impala/analysis/AnalyticExpr.java: Line 543: // This is not currently used as an AnalyticExpr cannot be a child of a conjunct. i would group that with the type assignment, because it's a standard component that has to be computed for every expr. easier to find that way. http://gerrit.cloudera.org:8080/#/c/2598/3/fe/src/main/java/com/cloudera/impala/analysis/CaseExpr.java File fe/src/main/java/com/cloudera/impala/analysis/CaseExpr.java: Line 330: evalCost_ = CASE_COST; do we need that (on top of the costs of the branches)? http://gerrit.cloudera.org:8080/#/c/2598/3/fe/src/main/java/com/cloudera/impala/analysis/Expr.java File fe/src/main/java/com/cloudera/impala/analysis/Expr.java: Line 191: // its children. evalCost_ will be -1 if it has not been calculated yet. evalCost_ you should point out when this gets set (see comment for selectivity_). also, rather than saying '-1 means invalid', you should say 'valid only after analysis'. we use -1 elsewhere to mean unknown elsewhere, which isn't quite the same (because in getCost() you assert it's != -1). Line 235: Preconditions.checkState(evalCost_ >= 0); check for isAnalyzed_ instead Line 1249: public static <T extends Expr> List<T> orderConjuncts(List<T> conjuncts) { too generic a name; orderConjunctsByCost? http://gerrit.cloudera.org:8080/#/c/2598/3/fe/src/main/java/com/cloudera/impala/analysis/InPredicate.java File fe/src/main/java/com/cloudera/impala/analysis/InPredicate.java: Line 185: IN_COST; is in_cost needed? (isn't that captured by the binary_predicate_cost?) -- To view, visit http://gerrit.cloudera.org:8080/2598 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I02279a26fbc6308ac5eb819d78345fc010469034 Gerrit-PatchSet: 3 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
