impala-reviews mailing list archives

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

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


Patch Set 2:

(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() { return UNKNOWN_COST; }
> single line (and elsewhere)
Done


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

Line 240:           (float) (getAvgStringLength(getChild(0)) + getAvgStringLength(getChild(1)))
*
The original code here (with different parenthesis position) doesn't seem correct.


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()) return UNKNOWN_COST;
> Prefer this to avoid code indentation and fewer lines:
Done


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:       computeNumDistinctValues();
> move after computeNumDistinctValues() for consistency
Done


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.
Not really. We need to initialize it in SlotRef, CastExpr and LiteralExpr.


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


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()) return UNKNOWN_COST;
> if (!hasChildCosts()) return UNKNOWN_COST;
Done


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()) return UNKNOWN_COST;
> if (!getChild(0).hasCost()) return UNKNOWN_COST;
Done


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()) return UNKNOWN_COST;
> single line and make the else if a regular if
Done


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
string_col = '15':
SLOT_REF_COST + LITERAL_COST + (AVGLEN(string_col)  + LEN('15')) * BINARY_PREDICATE_COST =
1 + 1 + (1 + 2) * 1 = 5

Note AVGLEN(string_col) = 1 in functional.alltypessmall

id + 15 = 27:
SLOT_REF_COST + CAST_COST + 2 * LITERAL_COST + ARITHMETIC_OP_COST + BINARY_PREDICATE_COST
= 1 + 1 + 2 * 1 + 1 + 1 = 6

id is cast from int to bigint before adding

Without this patch the CAST_COST was not updated into its parent because the CastExpr was
created somewhere in analyzeImpl(). The cost computed will be 5 instead of 6.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message