impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <>
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:

File fe/src/main/java/org/apache/impala/rewrite/

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

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

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/  I looked and I could obviously write four functions
to parallel that implementation, but it seems no better.

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 understan
Updated the comment.

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 n

PS2, Line 300: Rewrites a = b
> Doesn't indicate that this creates Expr 'a = b'. How about:

PS2, Line 303: literal
> this fn doesn't appear to require literal exprs. A more precise name might 

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...

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-HasComments: Yes

View raw message