Matthew Jacobs has posted comments on this change. Change subject: IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal. ......................................................................
Patch Set 4: (2 comments) > > Matt: Might be nice to have some tests like those added in this > patch which make sure the rewritten queries work with nullable > tuples (which are a weird wart, arguably): > > The bug there was, if I read it right, that coalesce(non-nullable-column, > x) was getting simplified to x, but the column could have been > coming from an outer join. Since this change deals only in literals > (and not columnns), I decided that adding a query test wasn't that > valuable. If you disagree, I'd be happy to add one. You're correct, I agree. > > I also wanted to see if I could tell from the explain output > whether or not the optimization was happening. It looks like I can, > if I put the relevant expression inside of an aggregation, since > "output: max:merge(id)" shows up. This could also power a test. Do > you think it's worth it? Seems like a good idea. > > > [...:21000] > explain select max(nvl(null, id)) from > functional.alltypes limit 3; > Query: explain select max(nvl(null, id)) from functional.alltypes > limit 3 > +----------------------------------------------+ > | Explain String | > +----------------------------------------------+ > | Max Per-Host Resource Reservation: Memory=0B | > | Per-Host Resource Estimates: Memory=180.00MB | > | Codegen disabled by planner | > | | > | PLAN-ROOT SINK | > | | | > | 03:AGGREGATE [FINALIZE] | > | | output: max:merge(id) | > | | limit: 3 | > | | | > | 02:EXCHANGE [UNPARTITIONED] | > | | | > | 01:AGGREGATE | > | | output: max(id) | > | | | > | 00:SCAN HDFS [functional.alltypes] | > | partitions=24/24 files=24 size=478.45KB | > +----------------------------------------------+ > Fetched 17 row(s) in 0.02s http://gerrit.cloudera.org:8080/#/c/7781/4/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java: Line 298: RewritesOk("nvl(null, id)", rule, "id"); how about ifnull(null, null), though clearly the test framework handles that in a bit of an ambiguous manner. PS4, Line 296: RewritesOk("ifnull(null, id)", rule, "id"); : RewritesOk("isnull(null, id)", rule, "id"); : RewritesOk("nvl(null, id)", rule, "id"); : RewritesOk("ifnull(id, id + 1)", rule, null); : : RewritesOk("ifnull(1, 2)", rule, "1"); : RewritesOk("ifnull(0, id)", rule, "0"); wrap this in a for loop and iterate over all 3 fns? -- To view, visit http://gerrit.cloudera.org:8080/7781 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9e4b8bf6fedd595f9ea54ffb30fc71a058c7f16c Gerrit-PatchSet: 4 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-HasComments: Yes
