Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.) ......................................................................
Patch Set 2: (16 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 really understand what it was doing. I don't have a better suggestion yet. PS2, Line 153: Boolean is there a reason you use Boolean over boolean? Given getValue() returns the primitive type, I'd prefer the primitive type so that readers don't have to reason about this being null. PS2, Line 154: val also, I don't see a reason not to getValue() inline here PS2, Line 165: : /** : * Helper for simplifying unary functions like isnull(x). : */ : private Expr unaryHelper2(Expr expr, LiteralExpr child, boolean onNull, : boolean onOther) same Line 179: private Expr simplifyFunctionCallExpr(FunctionCallExpr expr, Analyzer analyzer) throws AnalysisException { nit: long line, please wrap at 90 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 don't feel strongly though, and in this case it does seem to be helpful. That said, I think we should try to find a way to make the function names/parameters a bit easier to understand on their own. PS2, Line 235: x y 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 understand why the cast case can't be simplified, but not the case involving argument ordering. Line 243: private Expr simplifyNullIfFunctionCallExpr(Expr expr, Analyzer analyzer) throws AnalysisException { nit long line PS2, Line 248: literalExprsEqual can you explain why this does constant folding but other optimizations do not? PS2, Line 300: Rewrites a = b Doesn't indicate that this creates Expr 'a = b'. How about: Returns a new Expr 'a = b', after folding constants. PS2, Line 303: literal this fn doesn't appear to require literal exprs. A more precise name might be something like: foldedExprsEqualExpr PS2, Line 306: Expr rewritten = analyzer.getConstantFolder().rewrite(pred, analyzer); : return rewritten; remove local var and return result PS2, Line 315: return rewritten instanceof BoolLiteral && : ((BoolLiteral) rewritten).getValue(); nit 1line? It looks less than 90chars... http://gerrit.cloudera.org:8080/#/c/7829/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: PS2, Line 424: // Exhaustive test for istrue and friends on boolean literals. It'd be helpful to have a few more cases where you have constant expressions, e.g. including some cases that don't get rewritten (istrue(true and true)) PS2, Line 426: // $for f in istrue isnottrue isfalse isnotfalse; do : // for y in true false null; do : // echo 'RewritesOk("'"$f($y)"'", rule, "'$(impala-shell.sh -B --quiet --query "select $f($y)" 2> /dev/null | tr '[a-z]' '[A-Z]')'");' : // done : // done fancy -- 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 <phi...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-HasComments: Yes
