yu feng has posted comments on this change. Change subject: IMPALA-5016: Try to rewrite coalesce() function in SimplifyConditionalsRule. ......................................................................
Patch Set 4: (35 comments) responded to comments by alex. 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 Done Line 83: */ > please rename to Done Line 101: /** > Clearer to understand if you spell out the cases in bullet points, somethin modify the comments for the function Line 104: * COALESCE(<literal>, a, b) -> <literal>, when literal is not NullLiteral; > please rename to Done Line 105: * COALESCE(<partition-slotref>, a, b) -> <partition-slotref>, > nit for consistency: we usually use 'numChilderen' Done Line 107: */ > Preserve the type as follows: Done Line 112: Expr childExpr = expr.getChildren().get(i); > single line if+continue, remove else block, i.e. Done Line 114: if (childExpr.isNullLiteral()) continue; > Can you move these into the function comment please? move those comments to the beginning of the function. Line 125: return result; > Why not skip leading NULLs in a separate step at the beginning? This soluti I always break when meets the first non-NULL parameter, so it is O(N) for the function. Line 136: > Use child.isLiteral() && !child.isNullLiteral(). Done Line 139: if (!slotRef.getDesc().getIsNullable()) return true; > Use Expr.unwrapExpr(false) here. This optimization is ok if the SlotRef has Add some test case for cast function. Line 141: if (slotRef.getDesc().getParent().getTable() instanceof HdfsTable && > Partition columns do not have stored stats, so this comment is misleading. Done Line 143: slotRef.getDesc().getParent().getTable().isClusteringColumn(slotRef.getDesc().getColumn())) { > We should also allow the simplification if !slotRef.getIsNullable() check slot with !slotRef.getIsNullable(), But I find isNullable_ is set to false when aggregation and union statement, Can not construct test case in ExprRewriteRulesTest.java Line 144: HdfsTable table = (HdfsTable) slotRef.getDesc().getParent().getTable(); > You should be able to omit these first two checks. An analyzed SlotRef must Done Line 149: } > no NULL value Done http://gerrit.cloudera.org:8080/#/c/7015/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 105: * COALESCE(<partition-slotref>, a, b) -> <partition-slotref>, > change <partition-column> to <partition-slotref> Done Line 113: // Skip leading nulls. > Nice! Done Line 128: /** > Flesh out the comment a little more. When does this function return true an Done Line 131: * child is a possibly cast SlotRef against a non-nullable slot; > single-line if Done Line 134: private boolean canSimplifyCoalesceUsingChild(Expr child) { > use unwrapSlotRef() Done PS2, Line 144: HdfsTable table = (HdfsTable) slotRef.getDesc().getParent().getTable(); change the single-line if http://gerrit.cloudera.org:8080/#/c/7015/3/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 105: * COALESCE(<partition-slotref>, a, b) -> <partition-slotref>, > long line Done Line 128: /** > Checks if the given child expr is nullable. Done 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: Simplify COALESCE function Done Line 341: RewritesOk("if(false, max(id), min(id))", rule, "min(id)"); > grammar and typos Done Line 344: RewritesOk( > grammar and typos (case -> case?), paarameter, please refrain from using al Done http://gerrit.cloudera.org:8080/#/c/7015/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 338: // IMPALA-5125: Exprs containing aggregates should not be rewritten if the rewrite > // Test skipping leading nulls Done Line 340: RewritesOk("if(true, 0, sum(id))", rule, null); > add one more leading null Done Line 341: RewritesOk("if(false, max(id), min(id))", rule, "min(id)"); > If the leading parameter is a non-NULL constant, rewrite to that constant. Done Line 345: "case when true then count(id) when false then sum(id) end", rule, "count(id)"); > If all parameters are NULL, rewrite to NULL Done Line 347: // IMPALA-5016: Simplify COALESCE function > // Do not rewrite non-literal constant exprs (rely on constant folding). Done Line 355: RewritesOk("coalesce(id)", rule, "id"); > also add coalesce(year, bigint_col) to test an implicit cast around year Done, add the testcase. Line 358: // Do not rewrite non-literal constant exprs, rely on constant folding. > We should add tests for the following cases. They will need more work becau Done, I add some more testcases depend on other tables: 'functional.alltypesagg' contains a partition column 'day', which have NULL value. 'functional_kudu.alltypes' is a kudu table, it's primary_key is not nullable. http://gerrit.cloudera.org:8080/#/c/7015/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 58: public Expr RewritesOk(String tableName, String expr, List<ExprRewriteRule> rules, > long line Done Line 376: RewritesOk("functional_kudu.alltypessmall", "coalesce(cast(id as string), string_col)", rule, > long line Done -- 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: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: yu feng <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: yu feng <[email protected]> Gerrit-HasComments: Yes
