Mailing-List: contact reviews-help@impala.incubator.apache.org; run by ezmlm
Precedence: bulk
Message-Id: <201701050700.v0570gGQ002900@ip-10-146-233-104.ec2.internal>
Date: Thu, 5 Jan 2017 07:00:42 +0000
From: "Alex Behm (Code Review)" <gerrit@cloudera.org>
To: Thomas Tauber-Marshall <tmarshall@cloudera.com>, impala-cr@cloudera.com,
        reviews@impala.incubator.apache.org
Reply-To: alex.behm@cloudera.com
Subject: =?UTF-8?Q?=5BImpala-ASF-CR=5D_IMPALA-1861=3A_Simplify_constant_conditionals=0A?=
In-Reply-To: <gerrit.1482967038000.Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b@gerrit.cloudera.org>
References: <gerrit.1482967038000.Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b@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.2
archived-at: Thu, 05 Jan 2017 07:00:54 -0000

Alex Behm has posted comments on this change.

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


Patch Set 2:

(23 comments)

Nice!

http://gerrit.cloudera.org:8080/#/c/5585/2//COMMIT_MSG
Commit Message:

Line 7: IMPALA-1861: Simplify constant conditionals
Simplify conditionals with constant conditions.


Line 13: 
What testing did you do? In particular, I'm curious if you ran the BE expr-test which will also exercise your new rule


http://gerrit.cloudera.org:8080/#/c/5585/2/fe/src/main/java/org/apache/impala/analysis/CaseWhenClause.java
File fe/src/main/java/org/apache/impala/analysis/CaseWhenClause.java:

Line 31:     this.whenExpr_ = whenExpr;
remove 'this' while here


Line 35:   public Expr getWhenExpr() {
single lines while you are here


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

Line 37:  * This rule simplifies exprs with constant valued predicates. It relies on
This rule simplifies conditional functions with constant conditions.


Line 38:  * FoldConstantsRule to reduce the predicates to BoolLiteral or NullLiteral first.
.. to replace the constants condition with a BoolLiteral or NullLiteral first...


Line 42:  * id = 0 || false -> id = 0
add an example AND

let's use AND and OR instead of && and || to make it very clear  that these are SQL expressions and to be consistent with the ExtractCommonConjunctsRule


Line 45: public class ConstantPredicateRule implements ExprRewriteRule {
SimplifyConditionalsRule?

The name should describe what the rule does, and I'm not sure I understand what action a ConstantPredicateRule performs.


Line 50:     if (expr instanceof FunctionCallExpr) {
As a sanity check, check expr.isAnalyzed() and bail if it not analyzed. See the TODO in FoldConstantsRule.

Here's a query you can try to see what happens when this rule is called on an unanalyzed expression.

select bool_col a from functional.alltypes group by if(a, bigint_col, int_col)

The problematic group-by expr has a select-list alias and that causes issues with our rewrite framework.


Line 51:       FunctionName fnName = ((FunctionCallExpr) expr).getFnName();
let's put these into separate functions, i.e. simplifyFunctionCallExpr(), simplifyCompoundPredicate(), simplifyCaseExpr(), etc.


Line 56:             return expr.getChild(1).castTo(expr.getType());
These casts should not be necessary if the expr is analyzed


Line 61:         } else if (expr.getChild(0) instanceof NullLiteral) {
Might be useful to add helper functions LiteralExpr.isTrue() and LiteralExpr.isFalse(). Inside isFalse() you can check for a false bool literal or a null literal. That way we don't have to check both paths everywhere.


Line 68:       if (pred.getOp() == CompoundPredicate.Operator.AND) {
This would become simpler if we normalized binary predicates to always have constants/literals on one side. Another rule to consider if you are up for it :). Even just simple normalization could help a lot.


Line 71:           if (child instanceof BoolLiteral) {
What if child is a NullLiteral? Also add a test for this if you haven't already.


Line 73:               // TRUE && foo, so return foo.
let's say 'expr' instead of foo


Line 98:       List<CaseWhenClause> newWhenClauses = new ArrayList<CaseWhenClause>();
Avoid creating objects when no rewrite takes place, e.g., you can leave this null and create it lazily on demand.

As we expand our rule set, these rewrite rules may be executed many many times.


Line 105:         if (((CaseExpr) expr).hasCaseExpr()) {
little easier to read if you use a "CaseExpr caseExpr = (CaseExpr) expr" at the beginning


Line 106:           BinaryPredicate pred = new BinaryPredicate(
Avoid object creation if caseExpr or expr.getChild(i) are not literals and this branch can't be simplified, e.g., you can set whenExpr to null which will fail any subsequent instanceof check


Line 133:           if (elseExpr == null) return new NullLiteral();
use NullLiteral.create(expr.getType());


Line 137:         CaseExpr result = new CaseExpr(caseExpr, newWhenClauses, elseExpr);
return new CaseExpr(...)


http://gerrit.cloudera.org:8080/#/c/5585/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 43:     // Create a rewriter with only a single rule.
remove comment and simplify code below to:

return RewritesOk(expr, Lists.newArrayList(rule), expectedStr);


Line 253:     RewritesOk("true && id = 0", rule, "id = 0");
add tests with NULL


Line 260:     RewritesOk("case 2 when 0 then id when 1 then id * 2 else 0 end", rule, "0");
also test the implicit ELSE NULL when hasCaseExpr()


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes