Marcel Kornacker has posted comments on this change. Change subject: IMPALA-1861: Simplify conditionals with constant conditions ......................................................................
Patch Set 3: (14 comments) http://gerrit.cloudera.org:8080/#/c/5585/3/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java File fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java: Line 80: rules.add(NormalizeExprsRule.INSTANCE); why not do this normalization right after folding constants? http://gerrit.cloudera.org:8080/#/c/5585/3/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java File fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java: Line 40: * Normalizes CompoundPredicates by ensuring that if either child of AND or OR is a make this the class comment 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: private Expr simplifyFunctionCallExpr(FunctionCallExpr expr) { are there any other functions that are subject to this? otherwise call this simplifyIf() or something like that? Line 67: if (fnName.getFunction().equals("if")) { if (!..equals("if")) return expr; to avoid one level of indentation Line 95: * 'expr' is false if 'expr' is false but null if 'expr' is true. is that standard sql? i thought 'null and <expr>' is always null. Line 99: private Expr simplifyCompoundPredicate(CompoundPredicate expr) { if (!(getchild(0) instanceof BoolLiteral)) return expr; and then simplify the rest Line 101: // NormalizeExprsRule ensures that if either child is a literal, the first child state this dependency in the class comment, not here Line 129: private Expr simplifyCaseExpr(CaseExpr expr, Analyzer analyzer) explanatory comment missing Line 132: List<CaseWhenClause> newWhenClauses = null; explanatory comment Line 140: if (caseExpr.isLiteral() && expr.getChild(i).isLiteral()) { it looks like expr.hasCaseExpr() && !caseExpr.isLiteral() means you'll never do anything. check that at the beginning and then exit. Line 144: whenExpr = analyzer.getConstantFolder().rewrite(pred, analyzer); this is a roundabout way to get a boolean value. use FeSupport.evalPredicate() instead and return immediately when appropriate. 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 254: RewritesOk("null || id = 0", rule, null); isn't this logically 'id = 0'? Line 269: RewritesOk("case when false then 0 end", rule, "NULL"); add 'case when true' add 'case' with multiple logical 'when true' (such as 'when 1= 1', 'when 2 * 2 = 3 + 1', etc.) Line 290: RewritesOk("true and id = 0", rule, null); ? -- 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
