Philip Zeyliger has posted comments on this change. Change subject: IMPALA-5211: Simplifying nullif conditional. ......................................................................
Patch Set 7: (9 comments) http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java: Line 49: * x is [not] distinct from x -> false [true] > this is handled in your new rule Done http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java: Line 1642: AnalysisError("select nullif(1,2,3)", "default.nullif() unknown"); > Weird. We should consider fixing this later by perhaps adding a dummy nulli Filed https://issues.apache.org/jira/browse/IMPALA-5938. 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: static class CountingRewriteRuleWrapper implements ExprRewriteRule { > static not needed I took it out. I put it in because this prevents the inner class from referencing ExprRewriteRulesTest.this. Is your consideration here that we should just be as sparse as possible for test code? Line 58: if (expr != ret) { > single line Done Line 128: // All specified rules must have fired at least once. > Explain why. Done Line 312: RewritesOk("nullif(NULL, NULL)", rule, "NULL"); > Did you check whether these new tests are not already addressed by expr-tes nullif, decode, and nvl2 all have some coverage in expr-test. I've removed this. Line 579: public void TestDistinctFromSimplificationRule() throws AnalysisException { > TestSimplifyDistinctFromRule() Done Line 603: * it can be further simplified via DistinctFromSimplificatinoRule. > SimplifyDistinctFromRule Done Line 619: // highlights that nullif gets transformed; the test infrastructure > I don't get the part about the test infrastructure. updated comment. -- 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: 7 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
