Alex Behm has posted comments on this change. Change subject: IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, and try to do static partition pruning with coalesce() function. ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/6974/1//COMMIT_MSG Commit Message: Line 7: IMPALA-5016 rewrite coalesce() in SimplifyConditionalsRule.java, We typically follow this format for commit msgs: IMPALA-ABCD: Single-line summary of change. <blank line> More detailed description of change. <blank line> Testing: Describe what tests you added, what testing you did. So how about something like this: IMPALA-5016: Simplify coalesce() in SimplifyConditionalsRule. http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/analysis/Expr.java File fe/src/main/java/org/apache/impala/analysis/Expr.java: Line 189: public final static com.google.common.base.Predicate<Expr> Don't think this is necessary. Please inline into the RewriteRule. http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 242: public boolean isCoalesceBuiltinFn() { Don't think this is necessary. Please inline into the RewriteRule. http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java File fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java: Line 129: } else if(conjunct.contains(Expr.IS_COALESCE_BUILTIN_FN_PREDICATE)) { I think this should be handled in the rewrite rule and not in this code. Do you have a case in mind that we would not be able to handle in the rewrite rule? I'm thinking the transformation would be like this: coalesce(a, b, c) -> a if 'a' is a SlotRef to a non-nullable column or if 'a' is a SlotRef to a partition column and that partition column does not have the NULL value In particular, I'm concerned about the added complexity to this code for handling a very narrow case. http://gerrit.cloudera.org:8080/#/c/6974/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 85: Preconditions.checkState(expr.getChildren().size() == 3); please move these cases into their own functions, i.e., simplifyIf() simplifyCoalesce() Line 100: for(int i = 0; i < childrenSize; ++ i) { Please try to follow the existing style: space after "for", no space after "++" Line 103: if(!childExpr.isConstant()) { I believe the rewrite is subtly wrong because: coalesce(1 + NULL, 10) -> coalesce(1 + NULL) One fix is to ignore non-literal constant exprs, and rely on constant folding to correctly simplify those cases. Line 104: List<Expr> newChildren = Lists.newArrayList(expr.getChildren().subList(i, childrenSize)); 1. We should try very hard not to generate objects when no rewrite is performed. 2. When no rewrite is performed, we should return the original unmodified expr. Line 110: return expr; Please look at the comment on ExprRewriteRule.apply(). Returning expr here communicates to the caller that no rewrite was performed, when in fact, you did modify the expr. The general rule is that when a rewrite was performed you need to return a new expression. Line 113: if(!childExpr.isNullLiteral()) { style: space after "if" and single line if possible -- To view, visit http://gerrit.cloudera.org:8080/6974 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I00aac2497ba51432037a6f9312805a63933ace87 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
