You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Alice Fan (Code Review)" <ge...@cloudera.org> on 2019/10/24 16:08:22 UTC
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Alice Fan has uploaded this change for review. ( http://gerrit.cloudera.org:8080/14540
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
For 'case' function, if left WHEN condition is true, SimplifyConditionalsRule
will return THEN result-expression. However, the result-expression should not
be casted to original expression's type. This will cause result-expression
not re-analyzed and throwing IllegalStateException.
For example:
Query: select * from functional.alltypessmall where case when true then id < 50 END
ERROR: IllegalStateException: null
Testing:
- Added e2e test to exprs.test
- Added unit test to ExprRewriteRulesTest
Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
3 files changed, 21 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/14540/1
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 1
Gerrit-Owner: Alice Fan <fa...@gmail.com>
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Alice Fan (Code Review)" <ge...@cloudera.org>.
Alice Fan has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@253
PS1, Line 253: return expr.getChild(i + 1);
> I don't think this is necessarily the right fix since the rewrite should no
As we discussed offline. It should be safe to remove cast here. As the SimplifyConditionalsRule only applies to analyzed expression , so the expr has ready been casted before it reaches rewrite step. Also, for some reason, the cast function will make a part of conjunction become not analyzed result in llegalStateException.
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@502
PS1, Line 502: + "END", rule, "id > 30");
> Can you add some expression rewriter test where the two branches have diffe
I added some test cases in ExprRewriterTest. But I think we knew that the change won't fix the issue of casting to compatible type for rewrite. That will require a separate jira to address the re-analyze issue. Really appreciated your examples and explanation!
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 1
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 02:32:52 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 2
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 22:47:29 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Alice Fan (Code Review)" <ge...@cloudera.org>.
Alice Fan has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 4:
My functional.alltypes data is messed up, so one of the newly added query tests will return incorrect result. I have fixed it. Sorry about that.
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 4
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 03:13:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 5
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 19:33:46 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
For 'case' function, if left WHEN condition is true, SimplifyConditionalsRule
will cast the THEN result-expression to original expression's type before return
result. In this Jira, we would like to remove the cast function for two reasons:
1. SimplifyConditionalsRule only applys to analyzed expression, which means
expression has already been casted to compatible type before it reaches the
expression rewrite step.
2. The cast function will cause IllegalStateException when 'CASE WHEN TRUE'
appearing in the where conjunction. For example:
Query: select * from functional.alltypessmall where case when true then id < 50 END
ERROR: IllegalStateException: null
Testing:
- Added e2e test to exprs.test
- Added unit test to ExprRewriteRulesTest
- Added unit test to ExprRewriterTest
Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Reviewed-on: http://gerrit.cloudera.org:8080/14540
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 54 insertions(+), 2 deletions(-)
Approvals:
Impala Public Jenkins: Looks good to me, approved; Verified
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 6
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 1:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4871/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 1
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 16:56:14 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 5: Verified+1
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 5
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 23:56:05 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 1:
(2 comments)
I had looked a bit at implicit casts recently and there's some pre-existing bugs in this area. I think the reason this fix works is because there's a different bug interacting with it. In theory the inserted cast should prevent expr rewrites changing the type, but it doesn't work when the expr is reanalysed. I included an example of a query where this problem occurs.
I think this fix is OK, but we should file a bug for that and add some more tests to this patch that show the buggy behaviour (and make sure they don't throw IllegalStateException or similar).
I think there's a bug here where the expression rewrite can change
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@253
PS1, Line 253: return expr.getChild(i + 1);
I don't think this is necessarily the right fix since the rewrite should not be able to change the type of the expression - inserting the cast is necessary in that case.
I don't think I understand why the cast is a problem, since it should be a pre-analyzed cast.
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:
http://gerrit.cloudera.org:8080/#/c/14540/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@502
PS1, Line 502: + "END", rule, "id > 30");
Can you add some expression rewriter test where the two branches have different types, e.g. "case when 1 = 2 then string_col else timestamp_col end"
Also some tests that verify the type produced in various cases with expression rewrites enabled. E.g. I can reproduce a bug where the rewrites change the type (i.e. the cast doesn't actually work):
[localhost:21000] default> set enable_expr_rewrites=false; select typeof(case when 1 = 1 then string_col else timestamp_col end) from functional.alltypes limit 1;
ENABLE_EXPR_REWRITES set to false
Query: select typeof(case when 1 = 1 then string_col else timestamp_col end) from functional.alltypes limit 1
Query submitted at: 2019-10-24 10:53:01 (Coordinator: http://tarmstrong-box:25000)
Query progress can be monitored at: http://tarmstrong-box:25000/query_plan?query_id=d7480cd9aab4b9fb:1df2edfc00000000
+----------------------------------------------------------------+
| typeof(case when 1 = 1 then string_col else timestamp_col end) |
+----------------------------------------------------------------+
| TIMESTAMP |
+----------------------------------------------------------------+
Fetched 1 row(s) in 0.11s
[localhost:21000] default> set enable_expr_rewrites=true; select typeof(case when 1 = 1 then string_col else timestamp_col end) from functional.alltypes limit 1;
ENABLE_EXPR_REWRITES set to true
Query: select typeof(case when 1 = 1 then string_col else timestamp_col end) from functional.alltypes limit 1
Query submitted at: 2019-10-24 10:53:10 (Coordinator: http://tarmstrong-box:25000)
Query progress can be monitored at: http://tarmstrong-box:25000/query_plan?query_id=ac4f5efb1e2c6402:8b81ef1800000000
+----------------------------------------------------------------+
| typeof(case when 1 = 1 then string_col else timestamp_col end) |
+----------------------------------------------------------------+
| STRING |
+----------------------------------------------------------------+
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 1
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 24 Oct 2019 17:55:42 +0000
Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 5:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5191/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 5
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 19:33:47 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 2:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4925/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 2
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 03:03:42 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 3:
Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/5167/ DRY_RUN=false
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 3
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 22:47:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 3: Verified-1
Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/5167/
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 3
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Sat, 02 Nov 2019 03:12:43 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Alice Fan (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14540
to look at the new patch set (#4).
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
For 'case' function, if left WHEN condition is true, SimplifyConditionalsRule
will cast the THEN result-expression to original expression's type before return
result. In this Jira, we would like to remove the cast function for two reasons:
1. SimplifyConditionalsRule only applys to analyzed expression, which means
expression has already been casted to compatible type before it reaches the
expression rewrite step.
2. The cast function will cause IllegalStateException when 'CASE WHEN TRUE'
appearing in the where conjunction. For example:
Query: select * from functional.alltypessmall where case when true then id < 50 END
ERROR: IllegalStateException: null
Testing:
- Added e2e test to exprs.test
- Added unit test to ExprRewriteRulesTest
- Added unit test to ExprRewriterTest
Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 54 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/14540/4
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 4
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 4:
Build Successful
https://jenkins.impala.io/job/gerrit-code-review-checks/4957/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 4
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Nov 2019 03:54:48 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 4
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Nov 2019 19:33:35 +0000
Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Alice Fan (Code Review)" <ge...@cloudera.org>.
Hello Thomas Tauber-Marshall, Tim Armstrong, Impala Public Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/14540
to look at the new patch set (#2).
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
For 'case' function, if left WHEN condition is true, SimplifyConditionalsRule
will cast the THEN result-expression to original expression's type before return
result. In this Jira, we would like to remove the cast function for two reasons:
1. SimplifyConditionalsRule only applys to analyzed expression, which means
expression has already been casted to compatible type before it reaches the
expression rewrite step.
2. The cast function will cause IllegalStateException when 'CASE WHEN TRUE'
appearing in the where conjunction. For example:
Query: select * from functional.alltypessmall where case when true then id < 50 END
ERROR: IllegalStateException: null
Testing:
- Added e2e test to exprs.test
- Added unit test to ExprRewriteRulesTest
- Added unit test to ExprRewriterTest
Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
---
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 54 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/40/14540/2
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 2
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
[Impala-ASF-CR] IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/14540 )
Change subject: IMPALA-9023: Fix IllegalStateException in SimplifyConditionalsRule
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/14540
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I640d577200e76121c72685e4aaba1ef312a2d8b4
Gerrit-Change-Number: 14540
Gerrit-PatchSet: 3
Gerrit-Owner: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Alice Fan <fa...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Nov 2019 22:47:47 +0000
Gerrit-HasComments: No