Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-1861: Simplify conditionals with constant conditions ......................................................................
Patch Set 3: (21 comments) http://gerrit.cloudera.org:8080/#/c/5585/2//COMMIT_MSG Commit Message: Line 7: IMPALA-1861: Simplify conditionals with constant conditions > Simplify conditionals with constant conditions. Done 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: whenExpr_ = whenExpr; > remove 'this' while here Done Line 35: public Expr getWhenExpr() { return whenExpr_; } > single lines while you are here Done 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 conditional functions with constant conditions. Done Line 38 > .. to replace the constants condition with a BoolLiteral or NullLiteral fir Done Line 42 > add an example AND Done Line 45 > SimplifyConditionalsRule? Done Line 50 > As a sanity check, check expr.isAnalyzed() and bail if it not analyzed. See I did this, but it causes problems with repeated application of rules, since a newly created expr resulting from a rewrite won't get reanalyzed until after rewriting is finished. For example, the addition of NormalizeExprsRule reverted a change to the empty.test PlannerTest. Line 51 > let's put these into separate functions, i.e. simplifyFunctionCallExpr(), s Done Line 56 > These casts should not be necessary if the expr is analyzed Done Line 61 > Might be useful to add helper functions LiteralExpr.isTrue() and LiteralExp I think this would be misleading, since NULL is not always equal to FALSE, eg NULL && true != false && true. Line 71 > What if child is a NullLiteral? Also add a test for this if you haven't alr For CompoundPredicates, we can't simplify NullLiterals unless the other child is a BoolLiteral in which case these cases will cover it. I added a comment explaining this. Line 73 > let's say 'expr' instead of foo Done Line 98 > Avoid creating objects when no rewrite takes place, e.g., you can leave thi Done Line 105 > little easier to read if you use a "CaseExpr caseExpr = (CaseExpr) expr" at Done Line 106 > Avoid object creation if caseExpr or expr.getChild(i) are not literals and Done Line 133 > use NullLiteral.create(expr.getType()); Done Line 137 > return new CaseExpr(...) Done 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: throws AnalysisException { > remove comment and simplify code below to: Done Line 253: RewritesOk("null && id = 0", rule, null); > add tests with NULL Done Line 260: "CASE 0 WHEN id THEN id + 1 END"); > also test the implicit ELSE NULL when hasCaseExpr() Done -- 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: 3 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
