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 <fan...@gmail.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 24 Oct 2019 17:55:42 +0000 Gerrit-HasComments: Yes
