Repository: incubator-impala Updated Branches: refs/heads/master b524a5ce6 -> 113526198
IMPALA-5211: Simplifying ifnull/isnull/nvl where conditional is a literal. Added code to simplify ifnull(<literal>, <x>) to x if <literal> is NULL or <literal> if it's not NULL. Removed a few unused Java imports. I did the naive thing and hardcoded the list of ifnull aliases. It looks like common/function-registry/impala_functions.py is used to generate ScalarBuiltins.java, and a similar approach could be taken. There are more conditional functions where similar approaches can be taken; IMPALA-5211 is not yet complete with just this patch. Testing: * Added new cases to existing unit test. * Added a simple test case to the PlannerTest infrastructure. * I ran the relevant test with code coverage using "mvn -Dtest=ExprRewriteRulesTest -Djacoco.skip=false test". Coverage of SimplifyConditionalsRule.java is very strong. There's one pre-existing "return expr" in simplifyCompoundPredicate that's not covered, and almost all the rest are preconditions checks. Change-Id: I9e4b8bf6fedd595f9ea54ffb30fc71a058c7f16c Reviewed-on: http://gerrit.cloudera.org:8080/7781 Reviewed-by: Alex Behm <[email protected]> Tested-by: Impala Public Jenkins Project: http://git-wip-us.apache.org/repos/asf/incubator-impala/repo Commit: http://git-wip-us.apache.org/repos/asf/incubator-impala/commit/48539a1b Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/48539a1b Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/48539a1b Branch: refs/heads/master Commit: 48539a1bbcfd2697dcd8e519b47e4e6d55a05e5c Parents: b524a5c Author: Philip Zeyliger <[email protected]> Authored: Tue Aug 22 16:12:59 2017 -0700 Committer: Impala Public Jenkins <[email protected]> Committed: Fri Aug 25 22:30:23 2017 +0000 ---------------------------------------------------------------------- .../rewrite/SimplifyConditionalsRule.java | 28 ++++++++++++++++---- .../impala/analysis/ExprRewriteRulesTest.java | 16 +++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/48539a1b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java index 6d0b38c..770bea0 100644 --- a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java +++ b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java @@ -29,13 +29,12 @@ import org.apache.impala.analysis.CompoundPredicate; import org.apache.impala.analysis.Expr; import org.apache.impala.analysis.FunctionCallExpr; import org.apache.impala.analysis.FunctionName; +import org.apache.impala.analysis.LiteralExpr; import org.apache.impala.analysis.NullLiteral; -import org.apache.impala.analysis.SlotDescriptor; -import org.apache.impala.analysis.SlotRef; -import org.apache.impala.catalog.HdfsTable; import org.apache.impala.common.AnalysisException; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; /*** @@ -53,6 +52,9 @@ import com.google.common.collect.Lists; public class SimplifyConditionalsRule implements ExprRewriteRule { public static ExprRewriteRule INSTANCE = new SimplifyConditionalsRule(); + private static List<String> IFNULL_ALIASES = ImmutableList.of( + "ifnull", "isnull", "nvl"); + @Override public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException { if (!expr.isAnalyzed()) return expr; @@ -100,6 +102,20 @@ public class SimplifyConditionalsRule implements ExprRewriteRule { } /** + * Simplifies IFNULL if the condition is a literal, using the + * following transformations: + * IFNULL(NULL, x) -> x + * IFNULL(a, x) -> a, if a is a non-null literal + */ + private Expr simplifyIfNullFunctionCallExpr(FunctionCallExpr expr) { + Preconditions.checkState(expr.getChildren().size() == 2); + Expr child0 = expr.getChild(0); + if (child0 instanceof NullLiteral) return expr.getChild(1); + if (child0.isLiteral()) return child0; + return expr; + } + + /** * Simplify COALESCE by skipping leading nulls and applying the following transformations: * COALESCE(null, a, b) -> COALESCE(a, b); * COALESCE(<literal>, a, b) -> <literal>, when literal is not NullLiteral; @@ -127,11 +143,13 @@ public class SimplifyConditionalsRule implements ExprRewriteRule { private Expr simplifyFunctionCallExpr(FunctionCallExpr expr) { FunctionName fnName = expr.getFnName(); - // TODO: Add the other conditional functions, eg. ifnull, istrue, etc. + // TODO: Add the other conditional functions, eg. istrue, etc. if (fnName.getFunction().equals("if")) { return simplifyIfFunctionCallExpr(expr); } else if (fnName.getFunction().equals("coalesce")) { return simplifyCoalesceFunctionCallExpr(expr); + } else if (IFNULL_ALIASES.contains(fnName.getFunction())) { + return simplifyIfNullFunctionCallExpr(expr); } return expr; } @@ -247,4 +265,4 @@ public class SimplifyConditionalsRule implements ExprRewriteRule { } return new CaseExpr(caseExpr, newWhenClauses, elseExpr); } -} \ No newline at end of file +} http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/48539a1b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java ---------------------------------------------------------------------- diff --git a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java index 4ff50a3..1401fed 100644 --- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java +++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java @@ -35,6 +35,7 @@ import org.apache.impala.rewrite.SimplifyConditionalsRule; import org.junit.Assert; import org.junit.Test; +import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; /** @@ -292,6 +293,19 @@ public class ExprRewriteRulesTest extends FrontendTestBase { RewritesOk("if(null, id, id+1)", rule, "id + 1"); RewritesOk("if(id = 0, true, false)", rule, null); + // IFNULL and its aliases + for (String f : ImmutableList.of("ifnull", "isnull", "nvl")) { + RewritesOk(f + "(null, id)", rule, "id"); + RewritesOk(f + "(null, null)", rule, "NULL"); + RewritesOk(f + "(id, id + 1)", rule, null); + + RewritesOk(f + "(1, 2)", rule, "1"); + RewritesOk(f + "(0, id)", rule, "0"); + // non literal constants shouldn't be simplified by the rule + RewritesOk(f + "(1 + 1, id)", rule, null); + RewritesOk(f + "(NULL + 1, id)", rule, null); + } + // CompoundPredicate RewritesOk("false || id = 0", rule, "id = 0"); RewritesOk("true || id = 0", rule, "TRUE"); @@ -376,6 +390,8 @@ public class ExprRewriteRulesTest extends FrontendTestBase { RewritesOk("if(true, 0, sum(id))", rule, null); RewritesOk("if(false, max(id), min(id))", rule, "min(id)"); RewritesOk("true || sum(id) = 0", rule, null); + RewritesOk("ifnull(null, max(id))", rule, "max(id)"); + RewritesOk("ifnull(1, max(id))", rule, null); RewritesOk("case when true then 0 when false then sum(id) end", rule, null); RewritesOk( "case when true then count(id) when false then sum(id) end", rule, "count(id)");
