Philip Zeyliger has posted comments on this change. Change subject: IMPALA-5211: Simplifying nullif conditional. ......................................................................
Patch Set 6: (17 comments) Thanks for the review. I've addressed all of the comments except moving around TestIfNull: it fundamentally relies on multiple rules to work, so I don't like moving it into one rule or another, but I'm happy to be over-written. I ended up down a small rabbit hole asserting that all the rules passed into RewriteOk() are really used. This led me to small fixes in the tests. I think it's not intrusive (and found some issues and would have helped me), but if you think otherwise, I can get rid of it. 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, Done Line 18: The error messages are slightly altered by this change: > Not just error messages, might be easier to just state that the original if Done Line 20: Before: > Not sure we need to flesh this out so much. Like you said below, this is wh Ok. Done (removed). 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! Done 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 A Done Line 86: throws AnalysisException { > This isn't right - createExpr() is and should only be called from the parse Good idea; done. Line 108: NullLiteral.create(Type.NULL) // NULL > Use new NullLiteral(). The create() returns an *analyzed* NullLiteral which Done 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 Done Line 155: // nullif and nvl2 are re-written into an if. > re-written -> converted Done Line 204: * Note that FunctionalCallExpr.createExpr() re-writes "nvl2" into "if", > re-writes -> converts Done 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 I think I cleaned these up. FoldConstantsRule is the only one that still throws anything, and SimplifyConditionalsRule uses a constant folder, so it permeates. 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 Ok. I had to move some decode tests around too, to satisfy this. And then I went down the small rabbit hole of actually making sure that all the rules specified are applied at least once, which led me to wrap the ExprRewriteRule. With the wrapper, I'm reasonably convinced that all the tests here exercise the rules they're labelled to exercise. This caused me to make minor modifications to some of the tests (that suffered from being solved via constant folding rather than the intention). Line 424: // nullif: handled by folding > move to constant folding test Moved the ones that are simplified there. I removed lines 431-435 entirely, since it's really a test that DISTINCT FROM is being rewritten correctly, and we already have coverage for that. Line 583: // nullif: handled by rewrite to if and simplification > converted to if and simplified Done. Line 584: RewritesOk("nullif(bool_col, bool_col)", rules, "NULL"); > Move into SimplifyConditionalsTest I've not done this, yet. These expressions rely on the conversion and then on both SimplifyConditioanlsRule and SimplifyDistinctFromRule. As such, I think it's confusing to put them in one of the tests that's specific to those rules. (As I was working on this, every time there was "rules" vs "rule" in the test, it took a little bit of deciphering to figure out what was going on.) All that said, happy to move this whole test function into SimplifyConditionalsTest if you think it's better. 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. Done PS6, Line 1238: // wrong number of arguments : ParserAnalysisError("select nullif()", "nullif() must be called with two arguments."); : ParserAnalysisError("select nullif(col, col2, col3)", : "nullif() must be called with two arguments."); I removed these; they're now tested elsewhere. -- 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
