Vuk Ercegovac has posted comments on this change. ( http://gerrit.cloudera.org:8080/11760 )
Change subject: IMPALA-7655: Rewrite if, isnull to use CASE ...................................................................... Patch Set 11: (13 comments) http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java File fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java: http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@39 PS11, Line 39: coalesce(v1, v2, ...) pls remove. here's a good place to put one todo for the coalesce work. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@43 PS11, Line 43: nullif where's this handled? http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@65 PS11, Line 65: coalesce stale. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@99 PS11, Line 99: // IMPALA-7793: CASE statement does not handle NULL from UDF overflow : // Thus, CASE cannot be made to act as coalesce() when handling Decimal : // overflow, so can't do the rewrite. : // case "coalesce": : // return rewriteCoalesceFn(expr); lets get rid of this. example changes for coalesce can be revived from this review. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@164 PS11, Line 164: rewriteCoalesceFn pls remove. http://gerrit.cloudera.org:8080/#/c/11760/10/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/11760/10/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@217 PS10, Line 217: // Note: retaining the current simplification because of bug: : // IMPALA-7793: CASE statement does not handle NULL from UDF overflow : // Which makes it impossible to rewrite coalesce() to CASE in : // RewriteConditionalFnsRule without loss of functionality. : // pls remove and optionally put this as a todo with the new rewrites. when coalesce is properly removed, removing this will follow. http://gerrit.cloudera.org:8080/#/c/11760/11/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/11760/11/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@a587 PS11, Line 587: why are these removed? http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@296 PS11, Line 296: // Note: retaining the current simplification because of bug: : // IMPALA-7793: CASE statement does not handle NULL from UDF overflow : // Which makes it impossible to rewrite coalesce() to CASE in : // RewriteConditionalFnsRule without loss of functionality. : // This implementation is temporary until that bug is fixed. : // Once it is, remove these tests and enable those in : // RewriteConditionalFnsRuleTest and FullRewriteTest. pls remove. this is more of a note for the reviewer and not necessary to stick with the code as currently written. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java File fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java: http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@37 PS11, Line 37: r coalesce since its rewrite handles all the : * required simplification. stale http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@119 PS11, Line 119: // v thank you for including tests that should work for bugs that will be fixed later. nit: pls turn these into block comments so that they are not at the start of the line. http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@163 PS11, Line 163: i nit: capitalize http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@223 PS11, Line 223: TestNullif() what's going on with the duplication with the test on L240? http://gerrit.cloudera.org:8080/#/c/11760/11/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@257 PS11, Line 257: Where were you planning to have a standard battery of expressions that you then apply to different parts of the statement? it does not look like uniform coverage currently. -- To view, visit http://gerrit.cloudera.org:8080/11760 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163 Gerrit-Change-Number: 11760 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers <[email protected]> Gerrit-Reviewer: Bharath Vissapragada <[email protected]> Gerrit-Reviewer: Csaba Ringhofer <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Paul Rogers <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Vuk Ercegovac <[email protected]> Gerrit-Comment-Date: Wed, 31 Oct 2018 23:30:47 +0000 Gerrit-HasComments: Yes
