impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5211: Simplifying conditionals (istrue, nullif, etc.)
Date Tue, 29 Aug 2017 21:12:34 GMT
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 <philip@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message