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

(18 comments)

Nice work! This approach looks much better. Still some work to do, but this patch is very
promising :)

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

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

simplifyIfFunction()

or

simplifyIfFunctionCallExpr()


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

simplifyCoalesceFunction()

or

simplifyCoalesceFunctionCallExpr()


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);

etc.


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


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

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 http://gerrit.cloudera.org:8080/7015
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0325a9e437261b15313bbdf2f22d83376a84b575
Gerrit-PatchSet: 1
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