Alex Behm has posted comments on this change. Change subject: IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal. ......................................................................
Patch Set 4: (6 comments) I vote for keeping the rewrite logic simple like it is in this patch. I think we made some mistakes with our complex coalesce() rewrite that also checks partition slots and slot nullability (and I'm not even sure what we have today is correct, will investigate). http://gerrit.cloudera.org:8080/#/c/7781/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 106: * NULL value or if the child is a literal. NULL is also a literal, so just saying literal is enough Line 111: if (child0 instanceof NullLiteral) { single line Line 115: Preconditions.checkState(!(child0 instanceof NullLiteral)); remove Preconditions, and single line (the Precondition is trivially true here) http://gerrit.cloudera.org:8080/#/c/7781/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 296: RewritesOk("ifnull(null, id)", rule, "id"); unfortunately, we don't cover this case for some of the other functions, but we should make sure that the rule does do anything if the condition has a non-literal constant, e.g.: RewritesOk("isnull(1 + 1, id)", rule, "ISNULL(1 + 1, id)"); RewritesOk("isnull(NULL + 1, id)", rule, "ISNULL(NULL + 1, id)"); PS4, Line 296: RewritesOk("ifnull(null, id)", rule, "id"); : RewritesOk("isnull(null, id)", rule, "id"); : RewritesOk("nvl(null, id)", rule, "id"); : RewritesOk("ifnull(id, id + 1)", rule, null); : : RewritesOk("ifnull(1, 2)", rule, "1"); : RewritesOk("ifnull(0, id)", rule, "0"); > wrap this in a for loop and iterate over all 3 fns? +1 Line 389: RewritesOk( please also add a similar case with ifnull here -- To view, visit http://gerrit.cloudera.org:8080/7781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e4b8bf6fedd595f9ea54ffb30fc71a058c7f16c Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-HasComments: Yes
