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-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:


The rewrite code looks very clean and concise!
File fe/src/main/java/org/apache/impala/rewrite/

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;

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

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
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I0325a9e437261b15313bbdf2f22d83376a84b575
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: yu feng <>
Gerrit-Reviewer: Alex Behm <>
Gerrit-HasComments: Yes

View raw message