[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE
Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/11760 ) Change subject: IMPALA-7655: Rewrite if, isnull to use CASE .. Abandoned Will revisit after cleaning up blocking issues. -- 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: abandon Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163 Gerrit-Change-Number: 11760 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE
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: (4 comments) http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG@42 PS11, Line 42: While the desire was to replace the BE implementation of if and isnull, This section summarizes limitations. They are: (1) no guarantee that the fns are removed (list dependent bugs, disabled rewrites), (2) missed simplifications (list dependent bugs). I think this can be made more concise. http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG@45 PS11, Line 45: ORDER BY clause, and 2) if the user disables rewrites. prior, you mentioned a potential performance regression-- I think I saw an example of this in the current tests, pls confirm. http://gerrit.cloudera.org:8080/#/c/11760/11//COMMIT_MSG@50 PS11, Line 50: Coalesce() is implemented, but disabled. The old rewrites are retained : until b as mentioned in the source files, I'd remove this. http://gerrit.cloudera.org:8080/#/c/11760/11/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test File testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test: http://gerrit.cloudera.org:8080/#/c/11760/11/testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test@2238 PS11, Line 2238: int_col did you want to mention the repeated sub-expression evaluation for examples like this (int_col could be an expression)? -- 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 Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 31 Oct 2018 23:53:24 + Gerrit-HasComments: Yes
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE
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
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE
Impala Public Jenkins 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: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/1236/ : 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/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 Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac Gerrit-Comment-Date: Wed, 31 Oct 2018 22:12:44 + Gerrit-HasComments: No
[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE
Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, Impala Public Jenkins, Vuk Ercegovac, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11760 to look at the new patch set (#11). Change subject: IMPALA-7655: Rewrite if, isnull to use CASE .. IMPALA-7655: Rewrite if, isnull to use CASE See IMPALA-7655 for backgound. Tim found that the current interpreted forms of if, isnull and coalesce are slow compared to the code-generated CASE statement. This patch rewrites the if and isnull functions into the equivalent CASE structure. Coalesce is omitted due to limitations within the code. Caveats: * IMPALA-7753: Conditionals in the top-level ORDER BY clause are not rewritten, but those one or more levels down are. The result is that PlannerTest files still show uses of if and isnull despite this fix. * IMPALA-7785: Similar, but says that the GROUP BY clause is not analyzed prior to rewrites, so the code again forces analysis. There are some odd "analyze()" calls in the code to work around this limitation snd the next two. * IMPALA-7769: Some expressions involving NULL are not simplified. As a result, some of the rewrites don't result in the most optimal result. * IMPALA-7754: The rewrite engine ignores unanalyzed expressions, yet the rewrite engine does not, in general, re-analyze the expressions it produces, causing simplifications to be skipped. * IMPALA-4356: Even after this change, code gen won't occur for SELECT expressions executed in teh root fragment (the most common case in simple tests.) Because of the above, some if and isnull calls are not rewritten, and others are not rewritten to the simplest forms. The tests contain commented-out tests, and comments with bug numbers, to record these known issues. Still, the current fix is useful enough by itself to proceed despite the above. While the desire was to replace the BE implementation of if and isnull, the caveats prevent doing so at present, so the BE retains the original interpreted forms. They are used when: 1) top-level conditions in the ORDER BY clause, and 2) if the user disables rewrites. Conditional function rewrites are moved into a new class, RewriteConditionalsRule in order to keep things simple. Coalesce() is implemented, but disabled. The old rewrites are retained until blocking issues are fixed. Testing: * Tests for conditional functions are pulled out into a new test class. * A new base class holds code common to the new class and the existing ExprRewriterTest. * A new FullRewriteTest class tests rewrites that require multiple passes thorugh teh rewrite engine. * Planner Test expected results were updated based on the rewritten expressions. Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163 --- M be/src/exprs/conditional-functions.h M fe/src/main/java/org/apache/impala/analysis/Analyzer.java M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test M testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test M testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test 13 files changed, 939 insertions(+), 256 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/11760/11 -- 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: newpatchset Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163 Gerrit-Change-Number: 11760 Gerrit-PatchSet: 11 Gerrit-Owner: Paul Rogers Gerrit-Reviewer: Bharath Vissapragada Gerrit-Reviewer: Csaba Ringhofer Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Paul Rogers Gerrit-Reviewer: Philip Zeyliger Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vuk Ercegovac
