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 4: (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@68 PS2, Line 68: expr.getChild(1).isLiteral() && > I did this because clubbing implicit casts would convert into one single ca Yes, explicit cast is preferable. Implicit casts are generally only added automatically when there is no "information loss" from the cast. In this case we want to execute the cast no matter what (we actually want to see if some information is lost), so semantically an explicit cast seems more precise even though the BE is code executed is exactly the same. http://gerrit.cloudera.org:8080/#/c/8660/4/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/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@31 PS4, Line 31: * "cast(<non-const expr> to <string type>) <eq/ne op> <string constant>" string literal (not string constant) http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@35 PS4, Line 35: * if the following is true: cast(cast(<string constant> as typeOf(<non-const string literal (not string constant) http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@36 PS4, Line 36: * expr>)) as typeOf(<original cast expr>)) == <string constant> string literal (not string constant) http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@39 PS4, Line 39: * <constant> is always on the right hand side and all constant Exprs have been <string-literal> (not <constant>) http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@48 PS4, Line 48: * Few cases that are are not rewritten as the redundancy test fails: double "are" Let's distinguish between cases that are not rewritten because the rewrite would be wrong and limitations of the current implementation. In particular, removing the cast in 3 would be correct, but we do not rewrite due limitations in this rule. http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@67 PS4, Line 67: !expr.getChild(0).isLiteral() && not needed, we already require that child(0) is a CastExpr http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@69 PS4, Line 69: (op == BinaryPredicate.Operator.EQ || op == BinaryPredicate.Operator.NE); this is the cheapest check, might want to to the front of the conjunction or do it separately in L63 http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java@75 PS4, Line 75: LiteralExpr constantExpr = (LiteralExpr) expr.getChild(1); literalExpr http://gerrit.cloudera.org:8080/#/c/8660/4/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/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@619 PS4, Line 619: // Works for other string types // Works for VARCHAR/CHAR http://gerrit.cloudera.org:8080/#/c/8660/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@641 PS4, Line 641: remove extra blank line -- 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: 4 Gerrit-Owner: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 15 Dec 2017 19:37:35 +0000 Gerrit-HasComments: Yes
