Thomas Tauber-Marshall has posted comments on this change. Change subject: IMPALA-1861: Simplify conditionals with constant conditions ......................................................................
Patch Set 4: (15 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(ExtractCommonConjunctRule.INSTANCE); > why not do this normalization right after folding constants? Done 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: if (expr instanceof CompoundPredicate) { > make this the class comment Done 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: /** > are there any other functions that are subject to this? otherwise call this There will be a number of other functions in the future, eg. 'ifnull', 'isfalse', 'isnotfalse', etc., most of which will be quite short to simplify and giving them each their own 'simplify' function would be cluttered, so I would prefer to leave this and your next comment below as is. Line 95: * false OR 'expr' -> 'expr' > is that standard sql? i thought 'null and <expr>' is always null. I believe so. This is how impala behaves currently, and also see for example, Postgres's docs: https://www.postgresql.org/docs/current/static/functions-logical.html Line 99: * 'expr' is false if 'expr' is false but null if 'expr' is true. > if (!(getchild(0) instanceof BoolLiteral)) return expr; Done Line 101: * NOT is covered by FoldConstantRule. > state this dependency in the class comment, not here Done Line 129: * Simpilfies CASE and DECODE. If any of the 'when's has a constant TRUE value, the > explanatory comment missing Done Line 132: * returned. > explanatory comment It occurs to me - we tried to do lazy creation here to minimize object creation since this rule may be invoked many times, but this list is still going to be created and populated with the created CaseWhenClause objects needlessly any time there is a CASE that can't be simplified and has at least one 'when', which I would guess would be the common case. Do you think it would be worth having an initial step where we iterate over the children, check if any cases are constant without constructing 'newWhenClauses' and return immediately if no cases are constant, only going through the more complicated logic if we know that we will be able to simplify something? This would speed up evaluation when we can't simplify anything at the cost of making it slower when we can, eg. because we would need to create and fold some of the BinaryPredicates for the hasCaseExpr case twice. Line 133: */ > briefly explain why this is needed (indicates that case can be simplified) Done Line 140: // Contains all 'when' clauses with non-constant conditions, used to contruct the new > one more thing: for 'case [<expr>]' you can remove all 'when null' branches Done 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. Done Line 254: RewritesOk("null && id = 0", rule, null); > isn't this logically 'id = 0'? No - see https://www.postgresql.org/docs/current/static/functions-logical.html Line 262: RewritesOk("case 1 when 0 then id when 1 then id + 1 when 2 then id + 2 end", rules, > also add statement with multiple matches. Done Line 269: // All FALSE, return implicit NULL ELSE. > add 'case when true' Done Line 290: RewritesOk("case when false then 0 end", rules, "NULL"); > ? These cases are already normalized. -- 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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Marcel Kornacker <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-HasComments: Yes
