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 Thu, 14 Sep 2017 00:42:11 GMT
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 <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