impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <>
Subject [Impala-CR](cdh5-trunk) IMPALA-2805: Order filters based on selectivity and cost
Date Fri, 01 Apr 2016 16:36:02 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-2805: Order filters based on selectivity and cost

Patch Set 3:


almost there. nice!
File fe/src/main/java/com/cloudera/impala/analysis/

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.
File fe/src/main/java/com/cloudera/impala/analysis/

Line 330:     evalCost_ = CASE_COST;
do we need that (on top of the costs of the branches)?
File fe/src/main/java/com/cloudera/impala/analysis/

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?
File fe/src/main/java/com/cloudera/impala/analysis/

Line 185:         IN_COST;
is in_cost needed? (isn't that captured by the binary_predicate_cost?)

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I02279a26fbc6308ac5eb819d78345fc010469034
Gerrit-PatchSet: 3
Gerrit-Project: Impala
Gerrit-Branch: cdh5-trunk
Gerrit-Owner: Thomas Tauber-Marshall <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Marcel Kornacker <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Mostafa Mokhtar <>
Gerrit-Reviewer: Thomas Tauber-Marshall <>
Gerrit-HasComments: Yes

View raw message