Alex Behm 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 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 nullif() signature to the builtins db. I think there's some general thinking to be done about these kind of converted exprs, e.g. how can we make them appear in "show functions". But not for this patch. Can you file a JIRA for this improvement? 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 Line 58: if (expr != ret) { single line Line 128: // All specified rules must have fired at least once. Explain why. Line 312: RewritesOk("nullif(NULL, NULL)", rule, "NULL"); Did you check whether these new tests are not already addressed by expr-test.cc? For constant folding, it's generally preferable to add new tests in expr-test.cc to keep everything in one place. The tests here usually deal with FE<->BE specific issues for constant folding, or with particularly tricky folding cases. Line 579: public void TestDistinctFromSimplificationRule() throws AnalysisException { TestSimplifyDistinctFromRule() Line 603: * it can be further simplified via DistinctFromSimplificatinoRule. SimplifyDistinctFromRule Line 619: // highlights that nullif gets transformed; the test infrastructure I don't get the part about the test infrastructure. -- 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 <phi...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-HasComments: Yes
