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