Alex Behm has posted comments on this change. Change subject: IMPALA-5016: Try to rewrite coalesce() function in SimplifyConditionalsRule. ......................................................................
Patch Set 1: (18 comments) Nice work! This approach looks much better. Still some work to do, but this patch is very promising :) http://gerrit.cloudera.org:8080/#/c/7015/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 49: * case when false then 0 when true then 1 end -> 1 update to have an example with coalesce Line 83: private Expr simplifyIfFunctionExpr(FunctionCallExpr expr) { please rename to simplifyIfFunction() or simplifyIfFunctionCallExpr() Line 101: * Simplifies COALESCE by returning the corresponding child if function has Clearer to understand if you spell out the cases in bullet points, something like: Simplifies COALESCE by skipping leading zeros and applying the following transformations: <list your examples here> TODO(ALEX): Refine comment. Line 104: private Expr simplifyCoalesceFunctionExpr(FunctionCallExpr expr) { please rename to simplifyCoalesceFunction() or simplifyCoalesceFunctionCallExpr() Line 105: int childrenSize = expr.getChildren().size(); nit for consistency: we usually use 'numChilderen' Line 107: Expr result = new NullLiteral(); Preserve the type as follows: Expr result = NullLiteral.create(expr.getType()); Line 112: continue; single line if+continue, remove else block, i.e. if (childExpr.isNullLiteral()) continue; ... Line 114: // COALESCE(1, a, b) => 1 Can you move these into the function comment please? You can express the conditions like this: COALESCE(<literal>, a, b) -> <literal> // Rely on constant folding. COALESCE(<non-literal constant>, a, b) -> COALESCE(<non-literal constant>, a, b); etc. Line 125: // COALESCE(null, a, b) => COALESCE(a, b) Why not skip leading NULLs in a separate step at the beginning? This solution is expensive (N^2) if there are many leading NULLs. Line 136: if (child.isConstant() && !child.contains(NullLiteral.class)) { Use child.isLiteral() && !child.isNullLiteral(). It's simpler to rely on constant folding than trying to figure out which constant expression can return a NULL value. The condition here is subtly wrong because a constant expression can return NULL even if it does not contain a NullLiteral, e.g.: case when 1 > 10 then 10 end There are many more examples like that one. It is impractical to try and list them all. Relying on constant folding is much simpler. Line 139: if (child instanceof SlotRef) { Use Expr.unwrapExpr(false) here. This optimization is ok if the SlotRef has an implicit or even an explicit cast. Add tests for the implicit and explicit cast scenarios, e.g.: // test implicitly cast partition col coalesce(year, bigint_col) -> year // test explicitly cast partition col coalesce(cast(year as string), string_col) -> year Line 141: // Can not assume column stats is correctly and newest Partition columns do not have stored stats, so this comment is misleading. Remove. Line 143: // check all case in case of NullPointerException. We should also allow the simplification if !slotRef.getIsNullable() Add a test for this case. Line 144: if (slot.getDesc() != null && slot.getDesc().getParent() != null && You should be able to omit these first two checks. An analyzed SlotRef must always have a SlotDescriptor, and a SlotDescriptor must always have a parent TupleDescriptor. Line 149: // return true if this partition column has no-null value. no NULL value http://gerrit.cloudera.org:8080/#/c/7015/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 337: // IMPALA-5016: constants COALESCE function should be rewritten. IMPALA-5016: Simplify COALESCE function Line 341: // first parameter is constants, rewrite to the the constant expr. grammar and typos Line 344: // special cast, first paarameter contains null, DO NOT REWRITE. grammar and typos (case -> case?), paarameter, please refrain from using all caps here -- To view, visit http://gerrit.cloudera.org:8080/7015 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0325a9e437261b15313bbdf2f22d83376a84b575 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: yu feng <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
