Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/11760 )
Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE ...................................................................... Patch Set 4: (8 comments) http://gerrit.cloudera.org:8080/#/c/11760/4//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11760/4//COMMIT_MSG@20 PS4, Line 20: k nit: please wrap at 72 chars http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@144 PS4, Line 144: instanceof NullLiteral) I would prefer .isNullLiteral() as other functions use that. http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@183 PS4, Line 183: revised.size() - 1; Can you add a comment about not adding the last child here? It took me some time until I understood that it only added in CaseExpr's default branch. http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@328 PS4, Line 328: isNull typo: ifNull http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@335 PS4, Line 335: * <li><code>nullif(x, null)</code> → <code>NULL</code></li> Is this really the way it should behave? Currently select nullif(1,NULL) returns 1. http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@345 PS4, Line 345: tail I think that we should return head in this case (if head is not null, then head and tail are distinct, so head should be returned). http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java File fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java: http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java@252 PS4, Line 252: // If non-leading parameter is a non-NULL constant, drop other terms nit: missing . http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java@291 PS4, Line 291: NULL This should be 'id', see my comment at SimplifyConditionalsRule.java:335 expr-test should catch this - be/src/exprs/expr-test.cc contains: TestValue("nullif(TRUE, NULL)", TYPE_BOOLEAN, true); Can you verify that it indeed catches this issue? -- To view, visit http://gerrit.cloudera.org:8080/11760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163 Gerrit-Change-Number: 11760 Gerrit-PatchSet: 4 Gerrit-Owner: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Csaba Ringhofer <csringho...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Paul Rogers <par0...@yahoo.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Wed, 24 Oct 2018 13:27:42 +0000 Gerrit-HasComments: Yes
