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 <tmarsh...@cloudera.com> Gerrit-Reviewer: Matthew Jacobs <m...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
