Philip Zeyliger has posted comments on this change. Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.) ......................................................................
Patch Set 4: -Code-Review (2 comments) Thanks for the review, Alex! I'll update nullif() to be simpler and use equals() for the literal case as well for the next iteration. http://gerrit.cloudera.org:8080/#/c/7829/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 160: if (expr.getChildren().size() == 1) { > Sorry, but I think these simplifications are not needed because they are al Yep, you're right. I'll remove this code, update comments to indicate that these methods exist, but are handled elsewhere, and I'll probably keep the tests but move them into wherever the folding rule is tested. Line 220: * Simplifies nullif(a, b) when a and b are identical > Why is equals() not sufficient on its own? Two identical literals should al I'm not sure what you mean by your suggestion "...deal with NullLiteral case specially in this rule". There are two cases for simplification: x.equals(y): return NULL [x/y don't have to be literals!] x.isLiteral() && y.isLiteral() && !x.equals(y): return x (Note that the comment on line 223-224 is poor; I'll fix.) Expressing it the way those two cases are written is simpler than what I have now, so I'll update that. I was initially worried that x.equals(y) might return false if the types of x and y are different, but that can't happen here since the types are promoted to be compatible by the time we get here. I thought about converting nullif(x,y) to if(x=y, null, x). The problem with that is that it won't capture the x.equals(y) case where x/y aren't literals. We don't simplify if(x=x, null, x) -> NULL. -- To view, visit http://gerrit.cloudera.org:8080/7829 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5 Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-HasComments: Yes
