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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
