Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201708282225.v7SMP3HC030160@ip-10-146-233-104.ec2.internal>
Date: Mon, 28 Aug 2017 22:25:03 +0000
From: "Matthew Jacobs (Code Review)" <gerrit@cloudera.org>
To: Philip Zeyliger <philip@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: mj@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-5211=3A_Simplifying_conditionals_=28istrue=2C_nullif=2C_etc=2E=29=0A?=
In-Reply-To: <gerrit.1503686704000.Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5@gerrit.cloudera.org>
References: <gerrit.1503686704000.Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5@gerrit.cloudera.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Content-Disposition: inline
User-Agent: Gerrit/2.12.7
archived-at: Mon, 28 Aug 2017 22:25:10 -0000

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 <philip@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes