Alex Behm has posted comments on this change. ( http://gerrit.cloudera.org:8080/8660 )
Change subject: IMPALA-5929: Remove redundant explicit casts to string ...................................................................... Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java File fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java: http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@33 PS2, Line 33: * casted to the non-const expression type, that is, check if the following is true: casted -> cast http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@42 PS2, Line 42: * cast(timestamp_col as string) = '2009-01-01 00:01:00' Nice! http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@46 PS2, Line 46: * is not the same if '0123456' is casted to int casted -> cast http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@56 PS2, Line 56: expr.getChild(0).getType() == Type.STRING && what about other string types like CHAR/VARVHAR? http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@62 PS2, Line 62: if (isPotentiallyRedundantCast) { Invert condition to avoid indentation. This is generally our style which some people find more readable (including me): if (!isPotentiallyRedundant) return expr; http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@68 PS2, Line 68: LiteralExpr resultOfCast = LiteralExpr.create(castConstExprToNonConstExprType, You current have 3 BE evals which are generally expensive. Why not club everything into one big expr like you have in the comment in L34? http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@70 PS2, Line 70: if(resultOfCast.isNullLiteral()) return expr; formatting: space after "if" http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@73 PS2, Line 73: LiteralExpr resultOfReverseCast = LiteralExpr.create(castResultBackToConstExprType, need to check for null return value of LiteralExpr.create() http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@88 PS2, Line 88: private boolean isDefinatelyConstant(Expr expr, Analyzer analyzer) I think this function is subtly buggy and we don't need this function. Explanation: * Function is buggy because analyze() may fail if the expr is non-constant and contains a select-list alias to be substituted. Take a look at the TODO in FoldConstantsRule. Let me know if you need more pointers. * In L56 we check the types of the exprs which means they should be analyzed (except literals). So it seems like we should analyze the non-constant expr before checking the type. * We should rely on constant folding and check whether child 0 is a literal. No need to check isConstant() at all. (nit: spelling is "definitely" and not "definately") http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@611 PS2, Line 611: // Works with complex expressions on both sides. Should not work without having the constant folding rule in the rule set. http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@625 PS2, Line 625: RewritesOk("cast(smallint_col as string) = '1000000000'", rule, null); Mention in the class comment of the new rule that cases like this one are not rewritten. -- To view, visit http://gerrit.cloudera.org:8080/8660 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I91b7c6452d0693115f9b9ed9ba09f3ffe0f36b2b Gerrit-Change-Number: 8660 Gerrit-PatchSet: 2 Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Mon, 11 Dec 2017 19:58:13 +0000 Gerrit-HasComments: Yes
