Marcel Kornacker has posted comments on this change. Change subject: IMPALA-1861: Simplify conditionals with constant conditions ......................................................................
Patch Set 3: (5 comments) 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 133: boolean foundFalseExpr = false; briefly explain why this is needed (indicates that case can be simplified) Line 140: if (caseExpr.isLiteral() && expr.getChild(i).isLiteral()) { > it looks like expr.hasCaseExpr() && !caseExpr.isLiteral() means you'll neve one more thing: for 'case [<expr>]' you can remove all 'when null' branches. this applies even to non-literal case exprs (which won't trigger l159). Line 144: whenExpr = analyzer.getConstantFolder().rewrite(pred, analyzer); > this is a roundabout way to get a boolean value. use FeSupport.evalPredicat never mind, you need this for the non-const case. http://gerrit.cloudera.org:8080/#/c/5585/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 239: public void TestSimplifyConditionalsRule() throws AnalysisException { for each conditional, also add a test case that doesn't allow any rewrites. Line 262: RewritesOk("case 3 when 0 then id when 1 then id + 1 end", rule, "NULL"); > add 'case null' also add statement with multiple matches. -- 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: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
