You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2017/01/06 19:40:19 UTC
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Thomas Tauber-Marshall has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5631
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
IMPALA-4716: Expr rewrite causes IllegalStateException
THe problem here is that the DECODE constructor in CaseExpr uses the
same decodeExpr object when building the BinaryPredicates that compare
the decodeExpr to each 'when' of the DECODE. This results in the
decodeExpr being cast for each BinaryPredicate, potentially making its
type incompatible with an earlier BinaryPredicate when it's cast for
a later BinaryPredicate, and leading to a Precondition check failure
in BinaryPredicate.toThrift.
This wasn't a problem prior to expr rewrites being introduced because
toThift was never called on the original BinaryPredicate objects that
shared the same child, only on exprs that had been cloned and reanalyzed
and so no longer shared the same child object. This is no longer the
case with expr rewrites turned on, as FoldConstantRule calls toThrift
to send the expr to the backend for evaluation.
The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.
Testing:
- Added a regression test to RewriteExprRulesTest.
Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
2 files changed, 9 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/5631/1
--
To view, visit http://gerrit.cloudera.org:8080/5631
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#3).
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
IMPALA-4716: Expr rewrite causes IllegalStateException
The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.
The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.
Testing:
- Added a regression test to RewriteExprRulesTest.
Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
2 files changed, 11 insertions(+), 3 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/5631/3
--
To view, visit http://gerrit.cloudera.org:8080/5631
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 5: Code-Review+2
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 3: Code-Review+1
LGTM, will let someone else approve
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
IMPALA-4716: Expr rewrite causes IllegalStateException
The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.
The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.
Testing:
- Added a regression test to exprs.test
Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Reviewed-on: http://gerrit.cloudera.org:8080/5631
Reviewed-by: Marcel Kornacker <ma...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
3 files changed, 35 insertions(+), 4 deletions(-)
Approvals:
Marcel Kornacker: Looks good to me, approved
Impala Public Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5631
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
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 <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 1:
(1 comment)
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
> Good catch, yes this 'encoded' should be cloned too.
It seems safer to me to clone it, although I'm not as familiar with this code as some others.
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5631/3/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:
Line 133: BinaryPredicate.Operator.EQ, encoded.clone(), candidate));
explain somewhere why this is needed
http://gerrit.cloudera.org:8080/#/c/5631/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 234: // correctly when FoldConstantRule is used.
this needs to be a test case in exprs.test (ie, a functional test), you don't want to test just for a specific rule. (what if some other rule gets added later that reintroduces the bug?)
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 1:
(1 comment)
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?
What about 'candidate'?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5631
to look at the new patch set (#4).
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
IMPALA-4716: Expr rewrite causes IllegalStateException
The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.
The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.
Testing:
- Added a regression test to exprs.test
Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
2 files changed, 24 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/5631/4
--
To view, visit http://gerrit.cloudera.org:8080/5631
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 1:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5631/1//COMMIT_MSG
Commit Message:
PS1, Line 9: THe problem here is that
remove
PS1, Line 13: earlier
avoid ambiguous terms earlier and later
Line 15: in BinaryPredicate.toThrift.
It looks to me like the issue that the 'folded' constant may just have a different type than the type in the BinaryPredicate: e.g. TINYINT and INT respectively decode(0, 1, 1, id, 1)
Is that correct? If so, that may be a more clear way to describe it. Also giving an example helps.
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?
--
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 3:
(1 comment)
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
> It seems safer to me to clone it, although I'm not as familiar with this co
Done
--
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5631/4/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:
Line 133: children_.add(encodedIsNull.clone());
another one that was missed.
it looks like this is easy to break (in the future). please add a verification step at the end of this c'tor that checks that the expr tree rooted at 'this' doesn't contain encoded or encodedIsNull. extend TreeNode if you don't find a good functional match.
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5631
to look at the new patch set (#5).
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
IMPALA-4716: Expr rewrite causes IllegalStateException
The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.
The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.
Testing:
- Added a regression test to exprs.test
Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
3 files changed, 35 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/5631/5
--
To view, visit http://gerrit.cloudera.org:8080/5631
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#2).
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
IMPALA-4716: Expr rewrite causes IllegalStateException
The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.
The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.
Testing:
- Added a regression test to RewriteExprRulesTest.
Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
2 files changed, 10 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/31/5631/2
--
To view, visit http://gerrit.cloudera.org:8080/5631
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5631/4/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:
Line 133: BinaryPredicate.Operator.EQ, encoded.clone(), candidate));
> another one that was missed.
Done
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 5:
Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/187/
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 5: Verified+1
--
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException
Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.
Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5631/3/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:
Line 133: children_.add(encodedIsNull.clone());
> explain somewhere why this is needed
Done
http://gerrit.cloudera.org:8080/#/c/5631/3/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
Line 234
> this needs to be a test case in exprs.test (ie, a functional test), you don
Done
--
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes