impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alex Behm (Code Review)" <>
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:

File fe/src/main/java/org/apache/impala/rewrite/

Line 49:  * x is [not] distinct from x -> false [true]
this is handled in your new rule
File fe/src/test/java/org/apache/impala/analysis/

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?
File fe/src/test/java/org/apache/impala/analysis/

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 For constant
folding, it's generally preferable to add new tests in 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 {

Line 603:    * it can be further simplified via DistinctFromSimplificatinoRule.

Line 619:     // highlights that nullif gets transformed; the test infrastructure
I don't get the part about the test infrastructure.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-Reviewer: Matthew Jacobs <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-HasComments: Yes

View raw message