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 Tue, 30 May 2017 21:48:39 GMT
Alex Behm has posted comments on this change.

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

Patch Set 1:


Nice work! This approach looks much better. Still some work to do, but this patch is very
promising :)
File fe/src/main/java/org/apache/impala/rewrite/

Line 49:  * case when false then 0 when true then 1 end -> 1
update to have an example with coalesce

Line 83:   private Expr simplifyIfFunctionExpr(FunctionCallExpr expr) {
please rename to




Line 101:    * Simplifies COALESCE by returning the corresponding child if function has
Clearer to understand if you spell out the cases in bullet points, something like:

Simplifies COALESCE by skipping leading zeros and applying the following transformations:
<list your examples here>

TODO(ALEX): Refine comment.

Line 104:   private Expr simplifyCoalesceFunctionExpr(FunctionCallExpr expr) {
please rename to




Line 105:     int childrenSize = expr.getChildren().size();
nit for consistency: we usually use 'numChilderen'

Line 107:     Expr result = new NullLiteral();
Preserve the type as follows:

Expr result = NullLiteral.create(expr.getType());

Line 112:         continue;
single line if+continue, remove else block, i.e.

if (childExpr.isNullLiteral()) continue;

Line 114:         // COALESCE(1, a, b) => 1
Can you move these into the function comment please?

You can express the conditions like this:

COALESCE(<literal>, a, b) -> <literal>
// Rely on constant folding.
COALESCE(<non-literal constant>, a, b) -> COALESCE(<non-literal constant>,
a, b);


Line 125:           // COALESCE(null, a, b) => COALESCE(a, b)
Why not skip leading NULLs in a separate step at the beginning? This solution is expensive
(N^2) if there are many leading NULLs.

Line 136:     if (child.isConstant() && !child.contains(NullLiteral.class)) {
Use child.isLiteral() && !child.isNullLiteral().

It's simpler to rely on constant folding than trying to figure out which constant expression
can return a NULL value. The condition here is subtly wrong because a constant expression
can return NULL even if it does not contain a NullLiteral, e.g.:

case when 1 > 10 then 10 end

There are many more examples like that one. It is impractical to try and list them all. Relying
on constant folding is much simpler.

Line 139:     if (child instanceof SlotRef) {
Use Expr.unwrapExpr(false) here. This optimization is ok if the SlotRef has an implicit or
even an explicit cast.

Add tests for the implicit and explicit cast scenarios, e.g.:

// test implicitly cast partition col
coalesce(year, bigint_col) -> year
// test explicitly cast partition col
coalesce(cast(year as string), string_col) -> year

Line 141:       // Can not assume column stats is correctly and newest
Partition columns do not have stored stats, so this comment is misleading. Remove.

Line 143:       // check all case in case of NullPointerException.
We should also allow the simplification if !slotRef.getIsNullable()

Add a test for this case.

Line 144:       if (slot.getDesc() != null && slot.getDesc().getParent() != null &&
You should be able to omit these first two checks. An analyzed SlotRef must always have a
SlotDescriptor, and a SlotDescriptor must always have a parent TupleDescriptor.

Line 149:         // return true if this partition column has no-null value.
no NULL value
File fe/src/test/java/org/apache/impala/analysis/

Line 337:     // IMPALA-5016: constants COALESCE function should be rewritten.
IMPALA-5016: Simplify COALESCE function

Line 341:     // first parameter is constants, rewrite to the the constant expr.
grammar and typos

Line 344:     // special cast, first paarameter contains null, DO NOT REWRITE.
grammar and typos (case -> case?), paarameter, please refrain from using all caps here

To view, visit
To unsubscribe, visit

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

View raw message