Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/8660 )
Change subject: IMPALA-5929: Remove redundant explicit casts to string ...................................................................... Patch Set 3: (10 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 Done 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 Done http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@56 PS2, Line 56: if ((expr.getChild(0) instanceof CastExpr) && > what about other string types like CHAR/VARVHAR? Done http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@62 PS2, Line 62: Expr nonConstExpr = expr.getChild(0).getChild(0); > Invert condition to avoid indentation. This is generally our style which so Done http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@68 PS2, Line 68: analyzer.getQueryCtx()); > You current have 3 BE evals which are generally expensive. I did this because clubbing implicit casts would convert into one single cast (https://github.com/cloudera/Impala/blob/5c4ec81e6e3348d7677840b437047707b523d5e2/fe/src/main/java/org/apache/impala/analysis/CastExpr.java#L57) that only casts to the outermost cast type. So for a case like this: cast(int_col as string) = '0100' where it would try to evaluate a cast expr like this: cast(cast('0100' as int) as string) would instead convert to : cast('0100' as string) and return '0100' and not the expected '100'. This would eventually result in a wrong conversion of cast(int_col as string) = '0100' => int_col = 100 An alternative to my current approach would be to make it an explicit cast then do that extra conversion later when creating the BinaryPredicate at Line78. Would you recommend I go with the explicit cast approach? http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@70 PS2, Line 70: Expr castResultBackToConstExprType = new CastExpr(constantExpr.getType(), > formatting: space after "if" Done http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@73 PS2, Line 73: analyzer.getQueryCtx()); > need to check for null return value of LiteralExpr.create() Done http://gerrit.cloudera.org:8080/#/c/8660/2/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@88 PS2, Line 88: throws AnalysisException { > I think this function is subtly buggy and we don't need this function. Expl Removed this and added dependency on FoldConstantsRule instead. 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. Done 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 n I mentioned a few in the examples with a short explanation -- 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: 3 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: Tue, 12 Dec 2017 21:42:45 +0000 Gerrit-HasComments: Yes
