Alex Behm has posted comments on this change. Change subject: IMPALA-5211: Simplifying ifnull conditional. ......................................................................
Patch Set 6: (16 comments) http://gerrit.cloudera.org:8080/#/c/7829/6//COMMIT_MSG Commit Message: Line 9: Re-writes ifnull(x, y) into if(x IS DISTINCT FROM y, x, NULL), Comment somewhat hard to read, maybe create bullet points foe each change, should be clear which one is a "conversion" and which one adds a new rewrite rule I prefer "convert" instead of re-write because the latter has a very specific meaning to us (done via ExprRewriteRule) Line 18: The error messages are slightly altered by this change: Not just error messages, might be easier to just state that the original ifnull() is printed as the converted if() everywhere including error messages, explain plans, column labels, etc. Line 20: Before: Not sure we need to flesh this out so much. Like you said below, this is what you already get with similar conversions, e.g., NVL2() or decode() http://gerrit.cloudera.org:8080/#/c/7829/6/be/src/exprs/conditional-functions-ir.cc File be/src/exprs/conditional-functions-ir.cc: Line 46 I like red! http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 85: public static Expr createExpr(FunctionName fnName, FunctionParams params) I think we should add tests along the lines of what we have for NVL2() in AnalyzeExprsTest#TestFunctionCallExpr Line 86: throws AnalysisException { This isn't right - createExpr() is and should only be called from the parser from which we should not throw an AnalysisException. We should strive to keep parsing and analysis distinct phases. I think the right way to fix the dilemma below is to only transform the NULLIF() into IF() if there are exactly two arguments - otherwise we can just create a FunctionCallExpr with the original NULLIF function name which will later fail gracefully during analysis. Line 108: NullLiteral.create(Type.NULL) // NULL Use new NullLiteral(). The create() returns an *analyzed* NullLiteral which is not what you want because we have not started analysis yet (still in parsing). create() is typically used for creating an analyzed NullLiteral of a specific (non-NULL) type http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 152: // Unary functions like isfalse, isnotfalse, istrue, isnottrue, nullvalue, Move to class comment Line 155: // nullif and nvl2 are re-written into an if. re-written -> converted Line 204: * Note that FunctionalCallExpr.createExpr() re-writes "nvl2" into "if", re-writes -> converts http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java: Line 39: public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException { Doesn't throw. I just noticed we declare throws unnecessarily in a bunch of other rules. Mind cleaning those up, too? http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 387: RewritesOk("nvl2(null, 1, 2)", rules, "2"); Let's try not to mix these tests. Constant folding should be tested in the constant folding test. The fact that this is a conditional function is secondary. The tests are grouped by rewrite rule - not by type of expr. Line 424: // nullif: handled by folding move to constant folding test Line 583: // nullif: handled by rewrite to if and simplification converted to if and simplified Line 584: RewritesOk("nullif(bool_col, bool_col)", rules, "NULL"); Move into SimplifyConditionalsTest http://gerrit.cloudera.org:8080/#/c/7829/6/fe/src/test/java/org/apache/impala/analysis/ParserTest.java File fe/src/test/java/org/apache/impala/analysis/ParserTest.java: Line 90: public void ParserAnalysisError(String stmt, String expectedErrorString) { Parser does analyze. Also I don't see this used. -- To view, visit http://gerrit.cloudera.org:8080/7829 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5 Gerrit-PatchSet: 6 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-HasComments: Yes
