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 nullif conditional.
Date Fri, 15 Sep 2017 15:49:21 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 48:   class CountingRewriteRuleWrapper implements ExprRewriteRule {
> I took it out.
I was thinking along the lines that nobody outside of ExprRewriteRulesTest needs to create
an instance of this, but your argument for static makes sense, too. private static then?


http://gerrit.cloudera.org:8080/#/c/7829/8/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 606:     RewritesOk("if(true, nullif(tinyint_col, int_col), null)",
I don't understand what coverage this adds. The ParserTest already checks the nullif() to
if() conversion. It's confusing to me to test conversion in ExprRewriteRulesTest because the
conversion does not happen through a rewrite rule. Also you need to wrap the nullif() in this
test with an if() to workaround the new counting check you added... Quite hard to comprehend
what's going on here. Please remove unless you insist this adds coverage.


-- 
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message