Alex Behm has posted comments on this change. Change subject: IMPALA-5030: Add support to NVL2() function ......................................................................
Patch Set 1: (10 comments) I left my CR comments for educational purposes, although most of them will be mute when switching to the FunctionCallExpr.createExpr() approach. Please look at my first comment in FunctionCallExpr first. http://gerrit.cloudera.org:8080/#/c/6962/1//COMMIT_MSG Commit Message: Line 17: IMPALA-5030 - Add NVL2() Function Clean up commit msg http://gerrit.cloudera.org:8080/#/c/6962/1/fe/src/main/java/org/apache/impala/analysis/Analyzer.java File fe/src/main/java/org/apache/impala/analysis/Analyzer.java: Line 329: rules.add(BetweenToCompoundRule.INSTANCE); Please look the places that reference BetweenToCompoundRule.INSTANCE. There are several places where we need to manually rewrite BetweenPredicates before the standard rewriting phase after analysis. We need to do the same rewrite for NVL2(). For example, I believe these queries will break on your patch: select 1 from t where nvl2(1, true, false); alter table t drop partition (partition_col = nvl2(1, 10, 20)); http://gerrit.cloudera.org:8080/#/c/6962/1/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) { The current approach looks too complicated. I think we should directly transform NVL2 into an IF FunctionCallExpr similar to what we do for decode. There are some usability caveats, like not showing NVL2 in error messages or the explain plan, but I think that is an acceptable tradeoff for making this change significantly simpler. Line 455: if (!argTypes[1].matchesType(argTypes[2])) { This does not deal with type promotion. Take a look at what we do in L565-L576. We would need to do something similar for NVL2(), but the existing code would need a good bit of refactoring. http://gerrit.cloudera.org:8080/#/c/6962/1/fe/src/main/java/org/apache/impala/rewrite/Nvl2ToCaseRule.java File fe/src/main/java/org/apache/impala/rewrite/Nvl2ToCaseRule.java: Line 33: public class Nvl2ToCaseRule implements ExprRewriteRule { Provide class comment with examples like in the other ExprRewriteRules Line 39: if (expr instanceof FunctionCallExpr) { negate this condition and inline the Nvl2RewriteFunction for brevity Line 53: whenClause.add(new CaseWhenClause(nullPred, expr.getChild(1))); Remove tabs. Line 55: // CASE WHEN x IS NOT NULL THEN y ELSE z END Why not IF? That seems shorter and more appropriate. Take a look at TupleIsNullPredicate.wrapExpr() for an example of creating an IF. http://gerrit.cloudera.org:8080/#/c/6962/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 81: public void TestNvl2ToCaseRule() throws AnalysisException { Remove tabs Line 85: RewritesOk("NVL2(id, 'not null', 'null')", rule, Add additional tests: 1. NULL literals. 2. Non-constant expressions in the then/else args. 3. Nested nvl2() functions -- To view, visit http://gerrit.cloudera.org:8080/6962 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I32b03e9864f46c9c5e482280d1aa676ff7f02644 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Vincent Tran <vtt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-HasComments: Yes
