impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
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:

(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 <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Mostafa Mokhtar <mmokhtar@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message