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 <fan...@gmail.com> Gerrit-Reviewer: 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: Fri, 01 Nov 2019 02:32:52 +0000 Gerrit-HasComments: Yes
