Alex Behm has posted comments on this change. Change subject: IMPALA-5030: Adds support for NVL2() function ......................................................................
Patch Set 2: (7 comments) 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. 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: SELECT NVL2() ... or SELECT _impala_builtins.NVL2() ... Line 95: Preconditions.checkArgument(params.size()==3, "NVL2() " Preconditions checks are like asserts. They are used to enforce code invariants, i.e., if one of them is hit it indicates a bug in the code. I suggest removing this Preconditions check. The analysis of the returned IF FunctionCallExpr will handle checking the number of arguments. Line 97: Preconditions.checkArgument( Same here, inappropriate use of Preconditions check. In addition, the matchesType() is too restrictive. We typically add implicit casts when possible to match function signatures. I think this should simply be removed. The analysis of the IF FunctionCallExpr will deal with type compatibility and add casts if necessary. 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 Line 1606: // NVL2() mention that NVL2() is converted to IF before analysis Line 1607: AnalyzesOk("select nvl2(1, 'not null', 'null')"); Add negative cases to show what error message will be shown, for example: select nvl2(1, 's', 10) select nvl2(1, 10); -- 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-HasComments: Yes
