impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4620: Refactor evalcost computation in query analysis
Date Wed, 06 Sep 2017 05:16:49 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-4620: Refactor evalcost computation in query analysis
......................................................................


Patch Set 1:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java:

Line 556:   protected float computeEvalCost() {
single line (and elsewhere)


http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:

Line 352:     if (hasChildCosts()) {
Prefer this to avoid code indentation and fewer lines:

if (!hasChildCosts()) return UNKNOWN_COST;


http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/CastExpr.java
File fe/src/main/java/org/apache/impala/analysis/CastExpr.java:

Line 68:       evalCost_ = computeEvalCost();
move after computeNumDistinctValues() for consistency


http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

Line 247:   protected float evalCost_;
We should be able to make this private now.


Line 369:    * computed. Should be called bottom-up whenever the the structure of subtree
is
double 'the'


http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/InPredicate.java
File fe/src/main/java/org/apache/impala/analysis/InPredicate.java:

Line 191:     if (hasChildCosts()) {
if (!hasChildCosts()) return UNKNOWN_COST;


http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java
File fe/src/main/java/org/apache/impala/analysis/IsNotEmptyPredicate.java:

Line 55:     if (getChild(0).hasCost()) {
if (!getChild(0).hasCost()) return UNKNOWN_COST;


http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
File fe/src/main/java/org/apache/impala/analysis/LikePredicate.java:

Line 150:     if (!hasChildCosts()) {
single line and make the else if a regular if


http://gerrit.cloudera.org:8080/#/c/7948/1/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
File fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java:

Line 93
Thanks for cleaning this up! Pretty confusing in how many places the eval cost was set...
who knows if it was necessary


http://gerrit.cloudera.org:8080/#/c/7948/1/testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test
File testdata/workloads/functional-planner/queries/PlannerTest/inline-view.test:

Line 218: |     predicates: functional.alltypessmall.string_col = '15', functional.alltypessmall.id
+ 15 = 27
The string predicate should be more expensive. Can you confirm the costs of these predicates?

I looked at this briefly and the string predicate should be something like:

SLOT_REF_COST + BINARY_PREDICTAE_COST + LITERAL_COST + AVG(sizeof(string_col)) + AVG(sizeof('15'))

that seems much higher than the other binary predicate


-- 
To view, visit http://gerrit.cloudera.org:8080/7948
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibec3d648532c185d4318476796b1ba432b0fe59e
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message