Vincent Tran has posted comments on this change. Change subject: IMPALA-5030: Adds support for NVL2() function ......................................................................
Patch Set 2: (16 comments) http://gerrit.cloudera.org:8080/#/c/6962/1//COMMIT_MSG Commit Message: Line 17 > Clean up commit msg Done http://gerrit.cloudera.org:8080/#/c/6962/2//COMMIT_MSG Commit Message: Line 11: expr3. > Briefly mention that NVL2() is converted to IF before analysis. Done 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: // Binary predicates must be rewritten to a canonical form for both Kudu predicate > Please look the places that reference BetweenToCompoundRule.INSTANCE. There Abandoned this approach in favor of IF FunctionCallExpr 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 tran Done http://gerrit.cloudera.org:8080/#/c/6962/2/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java File fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java: Line 94: if (fnName.getFnNamePath().get(0).equalsIgnoreCase("nvl2")) { > Please follow the same checks as above for "decode". You could have: Done Line 95: Preconditions.checkArgument(params.size()==3, "NVL2() " > Preconditions checks are like asserts. They are used to enforce code invari Done Line 97: Preconditions.checkArgument( > Same here, inappropriate use of Preconditions check. In addition, the match Done 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 > Provide class comment with examples like in the other ExprRewriteRules Done Line 39 > negate this condition and inline the Nvl2RewriteFunction for brevity Done Line 53 > Remove tabs. Done Line 55 > Why not IF? That seems shorter and more appropriate. Take a look at TupleIs Done http://gerrit.cloudera.org:8080/#/c/6962/2/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: Line 1605: > remove whitespace Done Line 1606: // NVL2() > mention that NVL2() is converted to IF before analysis Done Line 1607: AnalyzesOk("select nvl2(1, 'not null', 'null')"); > Add negative cases to show what error message will be shown, for example: Done 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: > Remove tabs Done Line 85: RewritesOk("int_col not between float_col and double_col", rule, > Add additional tests: Done -- 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: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Vincent Tran <vtt...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Vincent Tran <vtt...@cloudera.com> Gerrit-HasComments: Yes
