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-5016: Try to rewrite coalesce() function in SimplifyConditionalsRule.
Date Wed, 07 Jun 2017 22:08:29 GMT
Alex Behm has posted comments on this change.

Change subject: IMPALA-5016: Try to rewrite coalesce() function in SimplifyConditionalsRule.
......................................................................


Patch Set 2:

(13 comments)

The rewrite code looks very clean and concise!

http://gerrit.cloudera.org:8080/#/c/7015/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 105:    * COALESCE(<partition-column>, a, b) -> <partition-column>, when
partition column do not contains null.
change <partition-column> to <partition-slotref>


change "when partition column do not contains null" to "when the partition column does not
contain NULL" (also long line)


Line 113:       if (childExpr.isNullLiteral()) continue;
Nice!


Line 128:    * Check child expr is nullable, assume child has been rewritten by constants
folding rule.
Flesh out the comment a little more. When does this function return true and when false?

For example,

Returns true if one of the following holds:
child is a non-NULL literal
child is a possibly cast SlotRef against a non-nullable slot
child is a possible case SlotRef against a partition column that does not contain NULL


Line 131:     if (child.isLiteral() && !child.isNullLiteral()) {
single-line if


Line 134:     child = child.unwrapExpr(false);
use unwrapSlotRef()

also to avoid deep indentation you can exit early like this:

SlotRef slotRef = child.unwrapSlotRef(false);
if (slotRef == null) return false;
...


Line 143:         // Return true if this partition column do not has NULL value.
partition column does not have a NULL value


http://gerrit.cloudera.org:8080/#/c/7015/2/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 338:     // If the leading parameters is null, rewrite COALESCE function with last parameters.
// Test skipping leading nulls


Line 340:     RewritesOk("coalesce(null, 1, id)", rule, "1");
add one more leading null


Line 341:     // If the leading parameter is constants, rewrite to constant expr.
If the leading parameter is a non-NULL constant, rewrite to that constant.


Line 345:     // If all parameters is null, rewrite to NULL.
If all parameters are NULL, rewrite to NULL


Line 347:     // If the leading parameter contains null-literal expr, DO NOT rewrite.
// Do not rewrite non-literal constant exprs (rely on constant folding).


Line 355:     RewritesOk("coalesce(year, id)", rule, "year");
also add coalesce(year, bigint_col) to test an implicit cast around year


Line 358:     RewritesOk("coalesce(null, year, id)", rule, "year");
We should add tests for the following cases. They will need more work because RewritesOk()
assumes a specific table, but we need to check other tables.

1. Check that no rewrite is performed for a partition-column that contains a NULL
2. Check that a rewrite is performed for a SlotRef against a non-nullable Slot


-- 
To view, visit http://gerrit.cloudera.org:8080/7015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0325a9e437261b15313bbdf2f22d83376a84b575
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <hzfengyu@corp.netease.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message