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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-HasComments: Yes
