impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1861: Simplify conditionals with constant conditions
Date Tue, 10 Jan 2017 02:42:08 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-1861: Simplify conditionals with constant conditions
......................................................................


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5585/4/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java:

Line 50:     if (!(expr.getChild(0) instanceof BoolLiteral)
why is isLiteral() not enough? (what other literal could it be?)


http://gerrit.cloudera.org:8080/#/c/5585/3/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 65:   /**
> There will be a number of other functions in the future, eg. 'ifnull', 'isf
then spell this out in a todo


Line 132:    * returned.
> It occurs to me - we tried to do lazy creation here to minimize object crea
i don't think it'll have any measurable impact on runtime, and the code won't get much simpler
(plus you need some additional code for the check), so maybe not. but try it out if you want
and see what comes out.


http://gerrit.cloudera.org:8080/#/c/5585/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 91:    * NormalizeExprsRule ensures will be the left child,  according to the following
rules:
spell out rule dependencies in the class comment (and also in analysisctx where you put the
rules together in a particular order, otherwise it's easy to break inadvertently)


Line 143:     // True if this CASE has a FALSE or NULL condition that can be simplified.
'simplified' -> 'removed' (even more specific)


Line 148:       Expr child = expr.getChild(i);
why don't you do

if (child instanceof nullliteral) {
  foundFalseExpr = true;
  continue;
}

here and then you don't have to deal with it anymore, such as in l175


-- 
To view, visit http://gerrit.cloudera.org:8080/5585
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message