impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Tauber-Marshall (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Date Sat, 07 Jan 2017 00:35:06 GMT
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................


Patch Set 2:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/5631/1//COMMIT_MSG
Commit Message:

PS1, Line 9: The DECODE constructor in
> remove
Done


PS1, Line 13: Precond
> avoid ambiguous terms earlier and later
Done


Line 15: The solution is to clone the decodeExpr in the DECODE constructor in
> It looks to me like the issue that the 'folded' constant may just have a di
As discussed, this isn't correct, but I think the confusion is that I'm trying to explain
too much, so I've simplified the commit message to focus on describing the actual bug, not
the subtle way it happened to manifest in this case.


http://gerrit.cloudera.org:8080/#/c/5631/1/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:

PS1, Line 133: encoded
> Does this 'encoded' need to be cloned too?
Good catch, yes this 'encoded' should be cloned too.

'candidate' is not reused (declared inside the loop) so it doesn't need to be cloned.

One case I'm not sure about is 'encodedIsNull'. It is reused, so it could potentially cause
weird issues, but it couldn't actually cause the particular issue in this jira since its type
is BOOLEAN and its the child of CompoundPredicates so it shouldn't have any casting issues.

Maybe better to just clone it anyways, since the small performance hit is worth minimizing
the risk of future bugs?


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

Line 238:     RewritesOk("decode(0, 1, 0, id, 1)", rule, "decode(0, 1, 0, id, 1)");
> to be clear, this threw the IllegalStateEx before your change?
Yes.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message