Philip Zeyliger has posted comments on this change. Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.) ......................................................................
Patch Set 2: (14 comments) http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: PS2, Line 145: /** : * Helper for simplifying unary boolean function (like istrue(x)). : */ : private Expr unaryHelper(Expr expr, LiteralExpr child, : boolean onTrue, boolean onFalse, boolean onNull) > I'm not crazy about this interface because I had to read all the code to re I added a few more comments here. I don't have a better name idea. "isBooleanHelper()" seems no better. I moved this below the code that uses it so it reads easier for reviewers. PS2, Line 153: Boolean > is there a reason you use Boolean over boolean? Given getValue() returns th Should have been boolean; fixed by inlining. PS2, Line 154: val > also, I don't see a reason not to getValue() inline here Done PS2, Line 165: : /** : * Helper for simplifying unary functions like isnull(x). : */ : private Expr unaryHelper2(Expr expr, LiteralExpr child, boolean onNull, : boolean onOther) > same I got rid of this method entirely. Line 179: private Expr simplifyFunctionCallExpr(FunctionCallExpr expr, Analyzer analyzer) throws AnalysisException { > nit: long line, please wrap at 90 Done PS2, Line 197: // ontrue onfalse onnull : if (fnName.equals("isfalse")) return unaryHelper(expr, c, false, true, false); > nit: we typically don't use cool whitespace formatting to line things up. I I debated using whitespace here vs. doing the easy thing, and I decided that it was a little bit easier to read when the truth table is right here in front of you. I'm happy to be overridden and just to have Eclipse re-format this, but I do think it's easier to read this way. I also thought about doing crazy bit math and nested ternary operators. I didn't go that route, as I figured I couldn't read it. The implementation for the C++ half of these is in be/src/exprs/conditional-functions-ir.cc. I looked and I could obviously write four functions to parallel that implementation, but it seems no better. PS2, Line 235: x > y Done Line 237: * Note that we currently don't simplify all possible equal expressions > This may be obvious, but not to me. Would you mind elaborating? I understan Updated the comment. Line 243: private Expr simplifyNullIfFunctionCallExpr(Expr expr, Analyzer analyzer) throws AnalysisException { > nit long line Done PS2, Line 248: literalExprsEqual > can you explain why this does constant folding but other optimizations do n Done PS2, Line 300: Rewrites a = b > Doesn't indicate that this creates Expr 'a = b'. How about: Done PS2, Line 303: literal > this fn doesn't appear to require literal exprs. A more precise name might Done PS2, Line 306: Expr rewritten = analyzer.getConstantFolder().rewrite(pred, analyzer); : return rewritten; > remove local var and return result Sure. I think I default to being more verbose because I like to set breakpoints, which leads me to having one thing per line. PS2, Line 315: return rewritten instanceof BoolLiteral && : ((BoolLiteral) rewritten).getValue(); > nit 1line? It looks less than 90chars... Done -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-HasComments: Yes
