impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal.
Date Thu, 24 Aug 2017 20:50:31 GMT
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 <philip@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message