Alex Behm has posted comments on this change. Change subject: IMPALA-4213: Planner not pushing some Kudu predicates ......................................................................
Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java File fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java: Line 124: try { Do we still need these changes? I kind of liked the previous flow better. I think with the changes to foldConstantChildren() should be sufficient (we can remove the analyzeNoThrow() from the original L145 though). http://gerrit.cloudera.org:8080/#/c/4613/2/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: Line 1195: * resulting LiteralExpr. Modifies 'this' in place and resets it. Hence, it is not safe Seems cleaner to reset() and then analyze() in here. That way callers don't need to worry about the weird state the modified Expr could be in. It just works. Line 1213: reset(); Let's only do this if there was actually folding. -- To view, visit http://gerrit.cloudera.org:8080/4613 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
