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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-HasComments: Yes
