Paul Rogers has posted comments on this change. ( http://gerrit.cloudera.org:8080/11887 )
Change subject: IMPALA-7818: Standardize use of Expr predicates ...................................................................... Patch Set 5: (5 comments) Thanks Vuk for the review. Addressed your comments. http://gerrit.cloudera.org:8080/#/c/11887/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11887/4//COMMIT_MSG@10 PS4, Line 10: isMumble( > sp? Done http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@1169 PS4, Line 1169: IS_NU > drop here, and other places where its not needed. Thanks for catching this. http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@432 PS4, Line 432: IS_NU > can drop Done http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java File fe/src/main/java/org/apache/impala/analysis/LikePredicate.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java@132 PS4, Line 132: IS_NON_NULL_LITERAL > double-checking if this translation preserves the previous expression? isNu I agree, the original is confusing with "isNullLiteral()" meaning "null literal or cast." Let's work this through. The first term in the original matches only literal nodes. The second term matched Null literal or a cast of a null. But, since the first term excluded the cast, we only care about the null literal check. The new predicate allows all literals except the null literal. So, unless I'm missing something, they seem identical. http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java File fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java: http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java@52 PS4, Line 52: IS_LITERAL.apply > IS_LITERAL? Yes. The change shown here is needed, but is better presented in a separate patch that includes new test cases. -- To view, visit http://gerrit.cloudera.org:8080/11887 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a Gerrit-Change-Number: 11887 Gerrit-PatchSet: 5 Gerrit-Owner: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Vuk Ercegovac <vercego...@cloudera.com> Gerrit-Comment-Date: Wed, 07 Nov 2018 21:17:04 +0000 Gerrit-HasComments: Yes
