Mailing-List: contact dev-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@impala.incubator.apache.org
Message-Id: <201604011636.u31Ga2bM021893@ip-10-146-233-104.ec2.internal>
Date: Fri, 1 Apr 2016 16:36:02 +0000
From: "Marcel Kornacker (Code Review)" <gerrit@cloudera.org>
To: Thomas Tauber-Marshall <tmarshall@cloudera.com>, impala-cr@cloudera.com,
        dev@impala.incubator.apache.org
CC: Matthew Jacobs <mj@cloudera.com>, Alex Behm <alex.behm@cloudera.com>,
        Mostafa Mokhtar <mmokhtar@cloudera.com>
Reply-To: marcel@cloudera.com
Subject: 
 =?UTF-8?Q?[Impala-CR](cdh5-trunk)_IMPALA-2805:_Order_filters_based_on_selectivity_and_cost=0A?=
In-Reply-To: 
 <gerrit.1458604925000.I02279a26fbc6308ac5eb819d78345fc010469034@gerrit.cloudera.org>
References: 
 <gerrit.1458604925000.I02279a26fbc6308ac5eb819d78345fc010469034@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.10-rc0

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