Matthew Jacobs 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 So now there are basically 2 things we have to do: a) fold the children (which now removes the unnecessary casts) b) re-order the predicate so it matches: slot op literal The advantage of doing (a) first is that we can directly check if one of the children is a SlotRef, otherwise we have to assume there could be a cast in there and use unwrapSlotRef(). That's not a big deal but it's just 1 less thing to think about. How would we be able to remove the analyze call which happens after the foldConstantChildren()? That calls reset(). Or were you thinking that foldConstantChildren() should also re-analyze? I'm not sure how that impacts the other caller in HdfsPartitionPruner.canEvalUsingPartitionMd() 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 Works for me, though there is another caller: HdfsPartitionPruner.canEvalUsingPartitionMd(). I'm not really sure what's going on with that code, i.e. why it expects an unanalyzed expr and I don't see it re-analyzing the expr after calling this. Does that look suspicious to you? Line 1213: reset(); > Let's only do this if there was actually folding. Done -- 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
