IMPALA-5211: Simplifying nullif conditional.
This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.
As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.
It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)
Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.
Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Reviewed-on: http://gerrit.cloudera.org:8080/7829
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/02302b7c
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/02302b7c
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/02302b7c
Branch: refs/heads/master
Commit: 02302b7cfee0397a9d8080289635fb694d89f15d
Parents: bdbfe22
Author: Philip Zeyliger <[email protected]>
Authored: Thu Aug 24 09:58:29 2017 -0700
Committer: Impala Public Jenkins <[email protected]>
Committed: Fri Sep 15 22:48:52 2017 +0000
----------------------------------------------------------------------
be/src/exprs/conditional-functions-ir.cc | 27 -----
be/src/exprs/conditional-functions.cc | 1 -
be/src/exprs/conditional-functions.h | 27 -----
be/src/exprs/scalar-expr.cc | 2 -
be/src/exprs/scalar-expr.h | 1 -
common/function-registry/impala_functions.py | 11 --
.../org/apache/impala/analysis/Analyzer.java | 2 +
.../impala/analysis/FunctionCallExpr.java | 30 +++--
.../impala/rewrite/BetweenToCompoundRule.java | 3 +-
.../rewrite/EqualityDisjunctsToInRule.java | 3 +-
.../rewrite/ExtractCommonConjunctRule.java | 3 +-
.../rewrite/NormalizeBinaryPredicatesRule.java | 3 +-
.../impala/rewrite/NormalizeCountStarRule.java | 3 +-
.../impala/rewrite/NormalizeExprsRule.java | 3 +-
.../rewrite/SimplifyConditionalsRule.java | 21 ++--
.../rewrite/SimplifyDistinctFromRule.java | 56 +++++++++
.../impala/analysis/AnalyzeExprsTest.java | 11 ++
.../impala/analysis/ExprRewriteRulesTest.java | 115 +++++++++++++++----
.../org/apache/impala/analysis/ParserTest.java | 5 +
19 files changed, 205 insertions(+), 122 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/be/src/exprs/conditional-functions-ir.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/conditional-functions-ir.cc
b/be/src/exprs/conditional-functions-ir.cc
index d984c90..e3458b7 100644
--- a/be/src/exprs/conditional-functions-ir.cc
+++ b/be/src/exprs/conditional-functions-ir.cc
@@ -43,33 +43,6 @@ IS_NULL_COMPUTE_FUNCTION(StringVal);
IS_NULL_COMPUTE_FUNCTION(TimestampVal);
IS_NULL_COMPUTE_FUNCTION(DecimalVal);
-#define NULL_IF_COMPUTE_FUNCTION(AnyValType) \
- AnyValType NullIfExpr::Get##AnyValType(ScalarExprEvaluator* eval, \
- const TupleRow* row) const { \
- DCHECK_EQ(children_.size(), 2); \
- AnyValType lhs_val = GetChild(0)->Get##AnyValType(eval, row); \
- /* Short-circuit in case lhs_val is NULL. Can never be equal to RHS. */ \
- if (lhs_val.is_null) return AnyValType::null(); \
- /* Get rhs and return NULL if lhs == rhs, lhs otherwise */ \
- AnyValType rhs_val = GetChild(1)->Get##AnyValType(eval, row); \
- if (!rhs_val.is_null && \
- AnyValUtil::Equals(GetChild(0)->type(), lhs_val, rhs_val)) { \
- return AnyValType::null(); \
- } \
- return lhs_val; \
- }
-
-NULL_IF_COMPUTE_FUNCTION(BooleanVal);
-NULL_IF_COMPUTE_FUNCTION(TinyIntVal);
-NULL_IF_COMPUTE_FUNCTION(SmallIntVal);
-NULL_IF_COMPUTE_FUNCTION(IntVal);
-NULL_IF_COMPUTE_FUNCTION(BigIntVal);
-NULL_IF_COMPUTE_FUNCTION(FloatVal);
-NULL_IF_COMPUTE_FUNCTION(DoubleVal);
-NULL_IF_COMPUTE_FUNCTION(StringVal);
-NULL_IF_COMPUTE_FUNCTION(TimestampVal);
-NULL_IF_COMPUTE_FUNCTION(DecimalVal);
-
#define NULL_IF_ZERO_COMPUTE_FUNCTION(type) \
type ConditionalFunctions::NullIfZero(FunctionContext* ctx, const type& val)
{ \
if (val.is_null || val.val == 0) return type::null(); \
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/be/src/exprs/conditional-functions.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/conditional-functions.cc
b/be/src/exprs/conditional-functions.cc
index e815b8a..19e3231 100644
--- a/be/src/exprs/conditional-functions.cc
+++ b/be/src/exprs/conditional-functions.cc
@@ -30,7 +30,6 @@ using namespace impala_udf;
}
CONDITIONAL_CODEGEN_FN(IsNullExpr);
-CONDITIONAL_CODEGEN_FN(NullIfExpr);
CONDITIONAL_CODEGEN_FN(IfExpr);
CONDITIONAL_CODEGEN_FN(CoalesceExpr);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/be/src/exprs/conditional-functions.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/conditional-functions.h
b/be/src/exprs/conditional-functions.h
index 28d0b53..40370c5 100644
--- a/be/src/exprs/conditional-functions.h
+++ b/be/src/exprs/conditional-functions.h
@@ -99,33 +99,6 @@ class IsNullExpr : public ScalarExpr {
virtual DecimalVal GetDecimalVal(ScalarExprEvaluator*, const TupleRow*)
const override;
};
-class NullIfExpr : public ScalarExpr {
- public:
- virtual Status GetCodegendComputeFn(LlvmCodeGen* codegen, llvm::Function**
fn)
- override WARN_UNUSED_RESULT;
- virtual std::string DebugString() const override {
- return ScalarExpr::DebugString("NullIfExpr");
- }
-
- protected:
- friend class ScalarExpr;
- friend class ScalarExprEvaluator;
-
- NullIfExpr(const TExprNode& node) : ScalarExpr(node) { }
- virtual BooleanVal GetBooleanVal(ScalarExprEvaluator*, const TupleRow*)
const override;
- virtual TinyIntVal GetTinyIntVal(ScalarExprEvaluator*, const TupleRow*)
const override;
- virtual SmallIntVal GetSmallIntVal(
- ScalarExprEvaluator*, const TupleRow*) const override;
- virtual IntVal GetIntVal(ScalarExprEvaluator*, const TupleRow*) const
override;
- virtual BigIntVal GetBigIntVal(ScalarExprEvaluator*, const TupleRow*) const
override;
- virtual FloatVal GetFloatVal(ScalarExprEvaluator*, const TupleRow*) const
override;
- virtual DoubleVal GetDoubleVal(ScalarExprEvaluator*, const TupleRow*) const
override;
- virtual StringVal GetStringVal(ScalarExprEvaluator*, const TupleRow*) const
override;
- virtual TimestampVal GetTimestampVal(
- ScalarExprEvaluator*, const TupleRow*) const override;
- virtual DecimalVal GetDecimalVal(ScalarExprEvaluator*, const TupleRow*)
const override;
-};
-
class IfExpr : public ScalarExpr {
public:
virtual Status GetCodegendComputeFn(LlvmCodeGen* codegen, llvm::Function**
fn)
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/be/src/exprs/scalar-expr.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-expr.cc b/be/src/exprs/scalar-expr.cc
index 956e188..04aaa6a 100644
--- a/be/src/exprs/scalar-expr.cc
+++ b/be/src/exprs/scalar-expr.cc
@@ -169,8 +169,6 @@ Status ScalarExpr::CreateNode(
// TODO: is there a better way to do this?
if (texpr_node.fn.name.function_name == "if") {
*expr = pool->Add(new IfExpr(texpr_node));
- } else if (texpr_node.fn.name.function_name == "nullif") {
- *expr = pool->Add(new NullIfExpr(texpr_node));
} else if (texpr_node.fn.name.function_name == "isnull" ||
texpr_node.fn.name.function_name == "ifnull" ||
texpr_node.fn.name.function_name == "nvl") {
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/be/src/exprs/scalar-expr.h
----------------------------------------------------------------------
diff --git a/be/src/exprs/scalar-expr.h b/be/src/exprs/scalar-expr.h
index 032ac94..e1954b7 100644
--- a/be/src/exprs/scalar-expr.h
+++ b/be/src/exprs/scalar-expr.h
@@ -198,7 +198,6 @@ class ScalarExpr : public Expr {
friend class IsNullExpr;
friend class KuduPartitionExpr;
friend class Literal;
- friend class NullIfExpr;
friend class NullLiteral;
friend class OrPredicate;
friend class Predicate;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/common/function-registry/impala_functions.py
----------------------------------------------------------------------
diff --git a/common/function-registry/impala_functions.py
b/common/function-registry/impala_functions.py
index 0e469e6..c004775 100644
--- a/common/function-registry/impala_functions.py
+++ b/common/function-registry/impala_functions.py
@@ -503,17 +503,6 @@ visible_functions = [
[['if'], 'TIMESTAMP', ['BOOLEAN', 'TIMESTAMP', 'TIMESTAMP'], ''],
[['if'], 'DECIMAL', ['BOOLEAN', 'DECIMAL', 'DECIMAL'], ''],
- [['nullif'], 'BOOLEAN', ['BOOLEAN', 'BOOLEAN'], ''],
- [['nullif'], 'TINYINT', ['TINYINT', 'TINYINT'], ''],
- [['nullif'], 'SMALLINT', ['SMALLINT', 'SMALLINT'], ''],
- [['nullif'], 'INT', ['INT', 'INT'], ''],
- [['nullif'], 'BIGINT', ['BIGINT', 'BIGINT'], ''],
- [['nullif'], 'FLOAT', ['FLOAT', 'FLOAT'], ''],
- [['nullif'], 'DOUBLE', ['DOUBLE', 'DOUBLE'], ''],
- [['nullif'], 'STRING', ['STRING', 'STRING'], ''],
- [['nullif'], 'TIMESTAMP', ['TIMESTAMP', 'TIMESTAMP'], ''],
- [['nullif'], 'DECIMAL', ['DECIMAL', 'DECIMAL'], ''],
-
[['zeroifnull'], 'TINYINT', ['TINYINT'],
'impala::ConditionalFunctions::ZeroIfNull'],
[['zeroifnull'], 'SMALLINT', ['SMALLINT'],
'impala::ConditionalFunctions::ZeroIfNull'],
[['zeroifnull'], 'INT', ['INT'], 'impala::ConditionalFunctions::ZeroIfNull'],
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
index 47acedc..c629bb7 100644
--- a/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
+++ b/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
@@ -58,6 +58,7 @@ import org.apache.impala.common.PrintUtils;
import org.apache.impala.common.RuntimeEnv;
import org.apache.impala.planner.PlanNode;
import org.apache.impala.rewrite.BetweenToCompoundRule;
+import org.apache.impala.rewrite.SimplifyDistinctFromRule;
import org.apache.impala.rewrite.EqualityDisjunctsToInRule;
import org.apache.impala.rewrite.ExprRewriteRule;
import org.apache.impala.rewrite.ExprRewriter;
@@ -343,6 +344,7 @@ public class Analyzer {
rules.add(SimplifyConditionalsRule.INSTANCE);
rules.add(EqualityDisjunctsToInRule.INSTANCE);
rules.add(NormalizeCountStarRule.INSTANCE);
+ rules.add(SimplifyDistinctFromRule.INSTANCE);
}
exprRewriter_ = new ExprRewriter(rules);
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
index 2bc2860..209cfbb 100644
--- a/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
@@ -84,27 +84,37 @@ public class FunctionCallExpr extends Expr {
*/
public static Expr createExpr(FunctionName fnName, FunctionParams params) {
FunctionCallExpr functionCallExpr = new FunctionCallExpr(fnName, params);
- if (fnName.getFnNamePath().size() == 1
- && fnName.getFnNamePath().get(0).equalsIgnoreCase("decode")
- || fnName.getFnNamePath().size() == 2
- &&
fnName.getFnNamePath().get(0).equalsIgnoreCase(Catalog.BUILTINS_DB)
- && fnName.getFnNamePath().get(1).equalsIgnoreCase("decode")) {
+ if (functionNameEqualsBuiltin(fnName, "decode")) {
return new CaseExpr(functionCallExpr);
}
- if (fnName.getFnNamePath().size() == 1
- && fnName.getFnNamePath().get(0).equalsIgnoreCase("nvl2")
- || fnName.getFnNamePath().size() == 2
- &&
fnName.getFnNamePath().get(0).equalsIgnoreCase(Catalog.BUILTINS_DB)
- && fnName.getFnNamePath().get(1).equalsIgnoreCase("nvl2")) {
+ if (functionNameEqualsBuiltin(fnName, "nvl2")) {
List<Expr> plist = Lists.newArrayList(params.exprs());
if (!plist.isEmpty()) {
plist.set(0, new IsNullPredicate(plist.get(0), true));
}
return new FunctionCallExpr("if", plist);
}
+ // nullif(x, y) -> if(x DISTINCT FROM y, x, NULL)
+ if (functionNameEqualsBuiltin(fnName, "nullif") && params.size() == 2) {
+ return new FunctionCallExpr("if", Lists.newArrayList(
+ new BinaryPredicate(BinaryPredicate.Operator.DISTINCT_FROM,
params.exprs().get(0),
+ params.exprs().get(1)), // x IS DISTINCT FROM y
+ params.exprs().get(0), // x
+ new NullLiteral() // NULL
+ ));
+ }
return functionCallExpr;
}
+ /** Returns true if fnName is a built-in with given name. */
+ private static boolean functionNameEqualsBuiltin(FunctionName fnName, String
name) {
+ return fnName.getFnNamePath().size() == 1
+ && fnName.getFnNamePath().get(0).equalsIgnoreCase(name)
+ || fnName.getFnNamePath().size() == 2
+ && fnName.getFnNamePath().get(0).equals(Catalog.BUILTINS_DB)
+ && fnName.getFnNamePath().get(1).equalsIgnoreCase(name);
+ }
+
/**
* Returns a new function call expr on the given params for performing the
merge()
* step of the given aggregate function.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
----------------------------------------------------------------------
diff --git
a/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
b/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
index 90110b8..bea4525 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
@@ -23,7 +23,6 @@ import org.apache.impala.analysis.BinaryPredicate;
import org.apache.impala.analysis.CompoundPredicate;
import org.apache.impala.analysis.Expr;
import org.apache.impala.analysis.Predicate;
-import org.apache.impala.common.AnalysisException;
/**
* Rewrites BetweenPredicates into an equivalent conjunctive/disjunctive
@@ -38,7 +37,7 @@ public class BetweenToCompoundRule implements ExprRewriteRule
{
public static ExprRewriteRule INSTANCE = new BetweenToCompoundRule();
@Override
- public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
+ public Expr apply(Expr expr, Analyzer analyzer) {
if (!(expr instanceof BetweenPredicate)) return expr;
BetweenPredicate bp = (BetweenPredicate) expr;
Expr result = null;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
----------------------------------------------------------------------
diff --git
a/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
b/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
index fa299df..d1a2ca7 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
@@ -25,7 +25,6 @@ import org.apache.impala.analysis.Analyzer;
import org.apache.impala.analysis.Expr;
import org.apache.impala.analysis.InPredicate;
import org.apache.impala.analysis.Subquery;
-import org.apache.impala.common.AnalysisException;
/**
* Coalesces disjunctive equality predicates to an IN predicate, and merges
compatible
@@ -41,7 +40,7 @@ public class EqualityDisjunctsToInRule implements
ExprRewriteRule {
public static ExprRewriteRule INSTANCE = new EqualityDisjunctsToInRule();
@Override
- public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
+ public Expr apply(Expr expr, Analyzer analyzer) {
if (!Expr.IS_OR_PREDICATE.apply(expr)) return expr;
Expr inAndOtherExpr = rewriteInAndOtherExpr(expr.getChild(0),
expr.getChild(1));
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
----------------------------------------------------------------------
diff --git
a/fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
b/fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
index 34934b0..9efe6c0 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
@@ -22,7 +22,6 @@ import java.util.List;
import org.apache.impala.analysis.Analyzer;
import org.apache.impala.analysis.CompoundPredicate;
import org.apache.impala.analysis.Expr;
-import org.apache.impala.common.AnalysisException;
import com.google.common.base.Preconditions;
import com.google.common.collect.Lists;
@@ -48,7 +47,7 @@ public class ExtractCommonConjunctRule implements
ExprRewriteRule {
private static final int MAX_EQUALS_COMPARISONS = 30 * 30;
@Override
- public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
+ public Expr apply(Expr expr, Analyzer analyzer) {
if (!Expr.IS_OR_PREDICATE.apply(expr)) return expr;
// Get childrens' conjuncts and check
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
----------------------------------------------------------------------
diff --git
a/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
b/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
index 34d7927..4871b0d 100644
---
a/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
+++
b/fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
@@ -20,7 +20,6 @@ package org.apache.impala.rewrite;
import org.apache.impala.analysis.Analyzer;
import org.apache.impala.analysis.BinaryPredicate;
import org.apache.impala.analysis.Expr;
-import org.apache.impala.common.AnalysisException;
/**
* Normalizes binary predicates of the form <expr> <op> <slot> so that the
slot is
@@ -38,7 +37,7 @@ public class NormalizeBinaryPredicatesRule implements
ExprRewriteRule {
public static ExprRewriteRule INSTANCE = new NormalizeBinaryPredicatesRule();
@Override
- public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
+ public Expr apply(Expr expr, Analyzer analyzer) {
if (!(expr instanceof BinaryPredicate)) return expr;
if (isExprOpSlotRef(expr) || isConstantOpExpr(expr)) {
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
----------------------------------------------------------------------
diff --git
a/fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
b/fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
index 90556c1..2128211 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
@@ -22,7 +22,6 @@ import org.apache.impala.analysis.Expr;
import org.apache.impala.analysis.FunctionCallExpr;
import org.apache.impala.analysis.FunctionName;
import org.apache.impala.analysis.FunctionParams;
-import org.apache.impala.common.AnalysisException;
/**
* Replaces count(<literal>) with an equivalent count{*}.
@@ -36,7 +35,7 @@ public class NormalizeCountStarRule implements
ExprRewriteRule {
public static ExprRewriteRule INSTANCE = new NormalizeCountStarRule();
@Override
- public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
+ public Expr apply(Expr expr, Analyzer analyzer) {
if (!(expr instanceof FunctionCallExpr)) return expr;
FunctionCallExpr origExpr = (FunctionCallExpr) expr;
if (!origExpr.getFnName().getFunction().equalsIgnoreCase("count")) return
expr;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
b/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
index 6ff9ba8..5706b65 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
@@ -21,7 +21,6 @@ import org.apache.impala.analysis.Analyzer;
import org.apache.impala.analysis.BoolLiteral;
import org.apache.impala.analysis.CompoundPredicate;
import org.apache.impala.analysis.Expr;
-import org.apache.impala.common.AnalysisException;
/**
* Normalizes CompoundPredicates by ensuring that if either child of AND or OR
is a
@@ -34,7 +33,7 @@ public class NormalizeExprsRule implements ExprRewriteRule {
public static ExprRewriteRule INSTANCE = new NormalizeExprsRule();
@Override
- public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
+ public Expr apply(Expr expr, Analyzer analyzer) {
if (!expr.isAnalyzed()) return expr;
// TODO: add normalization for other expr types.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/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 770bea0..0ad5748 100644
--- a/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
+++ b/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
@@ -28,8 +28,6 @@ import org.apache.impala.analysis.CaseWhenClause;
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.common.AnalysisException;
@@ -48,6 +46,11 @@ import com.google.common.collect.Lists;
* false AND id = 1 -> false
* case when false then 0 when true then 1 end -> 1
* coalesce(1, 0) -> 1
+ *
+ * Unary functions like isfalse, isnotfalse, istrue, isnottrue, nullvalue,
+ * and nonnullvalue don't need special handling as the fold constants rule
+ * will handle them. nullif and nvl2 are converted to an if in
FunctionCallExpr,
+ * and therefore don't need handling here.
*/
public class SimplifyConditionalsRule implements ExprRewriteRule {
public static ExprRewriteRule INSTANCE = new SimplifyConditionalsRule();
@@ -141,14 +144,13 @@ public class SimplifyConditionalsRule implements
ExprRewriteRule {
}
private Expr simplifyFunctionCallExpr(FunctionCallExpr expr) {
- FunctionName fnName = expr.getFnName();
+ String fnName = expr.getFnName().getFunction();
- // TODO: Add the other conditional functions, eg. istrue, etc.
- if (fnName.getFunction().equals("if")) {
+ if (fnName.equals("if")) {
return simplifyIfFunctionCallExpr(expr);
- } else if (fnName.getFunction().equals("coalesce")) {
+ } else if (fnName.equals("coalesce")) {
return simplifyCoalesceFunctionCallExpr(expr);
- } else if (IFNULL_ALIASES.contains(fnName.getFunction())) {
+ } else if (IFNULL_ALIASES.contains(fnName)) {
return simplifyIfNullFunctionCallExpr(expr);
}
return expr;
@@ -193,10 +195,13 @@ public class SimplifyConditionalsRule implements
ExprRewriteRule {
}
/**
- * Simpilfies CASE and DECODE. If any of the 'when's have constant
FALSE/NULL values,
+ * Simplifies CASE and DECODE. If any of the 'when's have constant
FALSE/NULL values,
* they are removed. If all of the 'when's are removed, just the ELSE is
returned. If
* any of the 'when's have constant TRUE values, the leftmost one becomes
the ELSE
* clause and all following cases are removed.
+ *
+ * Note that FunctionalCallExpr.createExpr() converts "nvl2" into "if",
+ * "decode" into "case", and "nullif" into "if".
*/
private Expr simplifyCaseExpr(CaseExpr expr, Analyzer analyzer)
throws AnalysisException {
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
----------------------------------------------------------------------
diff --git
a/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
b/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
new file mode 100644
index 0000000..1e56adb
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
@@ -0,0 +1,56 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+package org.apache.impala.rewrite;
+
+import org.apache.impala.analysis.Analyzer;
+import org.apache.impala.analysis.BinaryPredicate;
+import org.apache.impala.analysis.BoolLiteral;
+import org.apache.impala.analysis.Expr;
+
+/**
+ * Simplifies DISTINCT FROM and NOT DISTINCT FROM predicates
+ * where the arguments are identical expressions.
+ *
+ * x IS DISTINCT FROM x -> false
+ * x IS NOT DISTINCT FROM x -> true
+ *
+ * Note that "IS NOT DISTINCT FROM" and the "<=>" are the same.
+ */
+public class SimplifyDistinctFromRule implements ExprRewriteRule {
+ public static ExprRewriteRule INSTANCE = new SimplifyDistinctFromRule();
+
+ @Override
+ public Expr apply(Expr expr, Analyzer analyzer) {
+ if (!expr.isAnalyzed()) return expr;
+
+ if (expr instanceof BinaryPredicate) {
+ BinaryPredicate pred = (BinaryPredicate) expr;
+ if (pred.getOp() == BinaryPredicate.Operator.NOT_DISTINCT) {
+ if (pred.getChild(0).equals(pred.getChild(1))) {
+ return new BoolLiteral(true);
+ }
+ }
+ if (pred.getOp() == BinaryPredicate.Operator.DISTINCT_FROM) {
+ if (pred.getChild(0).equals(pred.getChild(1))) {
+ return new BoolLiteral(false);
+ }
+ }
+ }
+ return expr;
+ }
+}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
index 18c7263..e393f65 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
@@ -1781,6 +1781,17 @@ public class AnalyzeExprsTest extends AnalyzerTest {
AnalysisError("select nvl2(now(), true)", "No matching function with
signature: " +
"if(BOOLEAN, BOOLEAN).");
AnalysisError("select nvl2()", "No matching function with signature:
if().");
+
+ // IFNULL() is converted to IF() before analysis.
+ AnalyzesOk("select nullif(1, 1)");
+ AnalyzesOk("select nullif(NULL, 'not null')");
+ AnalyzesOk("select nullif('not null', null)");
+ AnalyzesOk("select nullif(int_col, int_col) from functional.alltypesagg");
+ // Because of conversion, nullif() isn't found rather than having no
+ // matching function with the signature.
+ AnalysisError("select nullif(1,2,3)", "default.nullif() unknown");
+ AnalysisError("select nullif('x', 1)",
+ "operands of type STRING and TINYINT are not comparable: 'x' IS
DISTINCT FROM 1");
}
@Test
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/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 1401fed..7b9d4b4 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
@@ -23,6 +23,7 @@ import org.apache.impala.catalog.Catalog;
import org.apache.impala.common.AnalysisException;
import org.apache.impala.common.FrontendTestBase;
import org.apache.impala.rewrite.BetweenToCompoundRule;
+import org.apache.impala.rewrite.SimplifyDistinctFromRule;
import org.apache.impala.rewrite.EqualityDisjunctsToInRule;
import org.apache.impala.rewrite.ExprRewriteRule;
import org.apache.impala.rewrite.ExprRewriter;
@@ -43,6 +44,22 @@ import com.google.common.collect.Lists;
*/
public class ExprRewriteRulesTest extends FrontendTestBase {
+ /** Wraps an ExprRewriteRule to count how many times it's been applied. */
+ private static class CountingRewriteRuleWrapper implements ExprRewriteRule {
+ int rewrites;
+ ExprRewriteRule wrapped;
+
+ CountingRewriteRuleWrapper(ExprRewriteRule wrapped) {
+ this.wrapped = wrapped;
+ }
+
+ public Expr apply(Expr expr, Analyzer analyzer) throws AnalysisException {
+ Expr ret = wrapped.apply(expr, analyzer);
+ if (expr != ret) rewrites++;
+ return ret;
+ }
+ }
+
public Expr RewritesOk(String exprStr, ExprRewriteRule rule, String
expectedExprStr)
throws AnalysisException {
return RewritesOk("functional.alltypessmall", exprStr, rule,
expectedExprStr);
@@ -79,11 +96,6 @@ public class ExprRewriteRulesTest extends FrontendTestBase {
return RewritesOkWhereExpr(tableName, exprStr, Lists.newArrayList(rule),
expectedExprStr);
}
- public Expr RewritesOkWhereExpr(String exprStr, List<ExprRewriteRule> rules,
String expectedExprStr)
- throws AnalysisException {
- return RewritesOkWhereExpr("functional.alltypessmall", exprStr, rules,
expectedExprStr);
- }
-
public Expr RewritesOkWhereExpr(String tableName, String exprStr,
List<ExprRewriteRule> rules,
String expectedExprStr) throws AnalysisException {
String stmtStr = "select count(1) from " + tableName + " where " +
exprStr;
@@ -99,12 +111,27 @@ public class ExprRewriteRulesTest extends FrontendTestBase
{
private Expr verifyExprEquivalence(Expr origExpr, String expectedExprStr,
List<ExprRewriteRule> rules, Analyzer analyzer) throws AnalysisException
{
String origSql = origExpr.toSql();
- ExprRewriter rewriter = new ExprRewriter(rules);
+
+ List<ExprRewriteRule> wrappedRules = Lists.newArrayList();
+ for (ExprRewriteRule r : rules) {
+ wrappedRules.add(new CountingRewriteRuleWrapper(r));
+ }
+ ExprRewriter rewriter = new ExprRewriter(wrappedRules);
+
Expr rewrittenExpr = rewriter.rewrite(origExpr, analyzer);
String rewrittenSql = rewrittenExpr.toSql();
boolean expectChange = expectedExprStr != null;
if (expectedExprStr != null) {
assertEquals(expectedExprStr, rewrittenSql);
+ // Asserts that all specified rules fired at least once. This makes sure
that the
+ // rules being tested are, in fact, being executed. A common mistake is
to write
+ // an expression that's re-written by the constant folder before getting
to the
+ // rule that is intended for the test.
+ for (ExprRewriteRule r : wrappedRules) {
+ CountingRewriteRuleWrapper w = (CountingRewriteRuleWrapper) r;
+ Assert.assertTrue("Rule " + w.wrapped.toString() + " didn't fire.",
+ w.rewrites > 0);
+ }
} else {
assertEquals(origSql, rewrittenSql);
}
@@ -320,51 +347,51 @@ public class ExprRewriteRulesTest extends
FrontendTestBase {
rules.add(rule);
// CASE with caseExpr
// Single TRUE case with no preceding non-constant cases.
- RewritesOk("case 1 when 0 then id when 1 then id + 1 when 2 then id + 2
end", rules,
+ RewritesOk("case 1 when 0 then id when 1 then id + 1 when 2 then id + 2
end", rule,
"id + 1");
// SINGLE TRUE case with preceding non-constant case.
- RewritesOk("case 1 when id then id when 1 then id + 1 end", rules,
+ RewritesOk("case 1 when id then id when 1 then id + 1 end", rule,
"CASE 1 WHEN id THEN id ELSE id + 1 END");
// Single FALSE case.
- RewritesOk("case 0 when 1 then 1 when id then id + 1 end", rules,
+ RewritesOk("case 0 when 1 then 1 when id then id + 1 end", rule,
"CASE 0 WHEN id THEN id + 1 END");
// All FALSE, return ELSE.
- RewritesOk("case 2 when 0 then id when 1 then id * 2 else 0 end", rules,
"0");
+ RewritesOk("case 2 when 0 then id when 1 then id * 2 else 0 end", rule,
"0");
// All FALSE, return implicit NULL ELSE.
- RewritesOk("case 3 when 0 then id when 1 then id + 1 end", rules, "NULL");
+ RewritesOk("case 3 when 0 then id when 1 then id + 1 end", rule, "NULL");
// Multiple TRUE, first one becomes ELSE.
RewritesOk("case 1 when id then id when 2 - 1 then id + 1 when 1 then id +
2 end",
rules, "CASE 1 WHEN id THEN id ELSE id + 1 END");
// When NULL.
- RewritesOk("case 0 when null then 0 else 1 end", rules, "1");
+ RewritesOk("case 0 when null then id else 1 end", rule, "1");
// All non-constant, don't rewrite.
- RewritesOk("case id when 1 then 1 when 2 then 2 else 3 end", rules, null);
+ RewritesOk("case id when 1 then 1 when 2 then 2 else 3 end", rule, null);
// CASE without caseExpr
// Single TRUE case with no predecing non-constant case.
- RewritesOk("case when FALSE then 0 when TRUE then 1 end", rules, "1");
+ RewritesOk("case when FALSE then 0 when TRUE then 1 end", rule, "1");
// Single TRUE case with preceding non-constant case.
- RewritesOk("case when id = 0 then 0 when true then 1 when id = 2 then 2
end", rules,
+ RewritesOk("case when id = 0 then 0 when true then 1 when id = 2 then 2
end", rule,
"CASE WHEN id = 0 THEN 0 ELSE 1 END");
// Single FALSE case.
- RewritesOk("case when id = 0 then 0 when false then 1 when id = 2 then 2
end", rules,
+ RewritesOk("case when id = 0 then 0 when false then 1 when id = 2 then 2
end", rule,
"CASE WHEN id = 0 THEN 0 WHEN id = 2 THEN 2 END");
// All FALSE, return ELSE.
RewritesOk(
- "case when false then 1 when false then 2 else id + 1 end", rules, "id
+ 1");
+ "case when false then 1 when false then 2 else id + 1 end", rule, "id
+ 1");
// All FALSE, return implicit NULL ELSE.
- RewritesOk("case when false then 0 end", rules, "NULL");
+ RewritesOk("case when false then 0 end", rule, "NULL");
// Multiple TRUE, first one becomes ELSE.
RewritesOk("case when id = 1 then 0 when 2 = 1 + 1 then 1 when true then 2
end",
rules, "CASE WHEN id = 1 THEN 0 ELSE 1 END");
// When NULL.
- RewritesOk("case when id = 0 then 0 when null then 1 else 2 end", rules,
+ RewritesOk("case when id = 0 then 0 when null then 1 else 2 end", rule,
"CASE WHEN id = 0 THEN 0 ELSE 2 END");
// All non-constant, don't rewrite.
- RewritesOk("case when id = 0 then 0 when id = 1 then 1 end", rules, null);
+ RewritesOk("case when id = 0 then 0 when id = 1 then 1 end", rule, null);
// DECODE
- // SIngle TRUE case with no preceding non-constant case.
+ // Single TRUE case with no preceding non-constant case.
RewritesOk("decode(1, 0, id, 1, id + 1, 2, id + 2)", rules, "id + 1");
// Single TRUE case with predecing non-constant case.
RewritesOk("decode(1, id, id, 1, id + 1, 0)", rules,
@@ -373,9 +400,9 @@ public class ExprRewriteRulesTest extends FrontendTestBase {
RewritesOk("decode(1, 0, id, tinyint_col, id + 1)", rules,
"CASE WHEN 1 = tinyint_col THEN id + 1 END");
// All FALSE, return ELSE.
- RewritesOk("decode(1, 0, 0, 2, 2, 3)", rules, "3");
+ RewritesOk("decode(1, 0, id, 2, 2, 3)", rules, "3");
// All FALSE, return implicit NULL ELSE.
- RewritesOk("decode(1, 1 + 1, 2, 1 + 2, 3)", rules, "NULL");
+ RewritesOk("decode(1, 1 + 1, id, 1 + 2, 3)", rules, "NULL");
// Multiple TRUE, first one becomes ELSE.
RewritesOk("decode(1, id, id, 1 + 1, 0, 1 * 1, 1, 2 - 1, 2)", rules,
"CASE WHEN 1 = id THEN id ELSE 1 END");
@@ -530,4 +557,46 @@ public class ExprRewriteRulesTest extends FrontendTestBase
{
RewritesOk("count(1 + 1)", rule, null);
RewritesOk("count(1 + null)", rule, null);
}
+
+ @Test
+ public void TestSimplifyDistinctFromRule() throws AnalysisException {
+ ExprRewriteRule rule = SimplifyDistinctFromRule.INSTANCE;
+
+ // Can be simplified
+ RewritesOk("bool_col IS DISTINCT FROM bool_col", rule, "FALSE");
+ RewritesOk("bool_col IS NOT DISTINCT FROM bool_col", rule, "TRUE");
+ RewritesOk("bool_col <=> bool_col", rule, "TRUE");
+
+ // Verify nothing happens
+ RewritesOk("bool_col IS NOT DISTINCT FROM int_col", rule, null);
+ RewritesOk("bool_col IS DISTINCT FROM int_col", rule, null);
+
+ // IF with distinct and distinct from
+ List<ExprRewriteRule> rules = Lists.newArrayList(
+ SimplifyConditionalsRule.INSTANCE,
+ SimplifyDistinctFromRule.INSTANCE);
+ RewritesOk("if(bool_col is distinct from bool_col, 1, 2)", rules, "2");
+ RewritesOk("if(bool_col is not distinct from bool_col, 1, 2)", rules, "1");
+ RewritesOk("if(bool_col <=> bool_col, 1, 2)", rules, "1");
+ RewritesOk("if(bool_col <=> NULL, 1, 2)", rules, null);
+ }
+
+ /**
+ * NULLIF gets converted to an IF, and has cases where
+ * it can be further simplified via SimplifyDistinctFromRule.
+ */
+ @Test
+ public void TestNullif() throws AnalysisException {
+ List<ExprRewriteRule> rules = Lists.newArrayList(
+ SimplifyConditionalsRule.INSTANCE,
+ SimplifyDistinctFromRule.INSTANCE);
+
+ // nullif: converted to if and simplified
+ RewritesOk("nullif(bool_col, bool_col)", rules, "NULL");
+
+ // works because the expression tree is identical;
+ // more complicated things like nullif(int_col + 1, 1 + int_col)
+ // are not simplified
+ RewritesOk("nullif(1 + int_col, 1 + int_col)", rules, "NULL");
+ }
}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/02302b7c/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
index 01ca9f5..d6d808f 100644
--- a/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/ParserTest.java
@@ -1211,6 +1211,11 @@ public class ParserTest extends FrontendTestBase {
ParsesOk("select f1(distinct col)");
ParsesOk("select f1(distinct col, col2)");
ParsesOk("select decode(col, col2, col3)");
+ // nullif should rewrite to if
+ assertEquals("SELECT if(col IS DISTINCT FROM col2, col, NULL) FROM t",
+ ParsesOk("select nullif(col, col2) from t").toSql());
+ assertEquals("SELECT if(col IS DISTINCT FROM col2, col, NULL) FROM t",
+ ParsesOk("select _impala_builtins.nullif(col, col2) from t").toSql());
ParserError("select f( from t");
ParserError("select f(5.0 5.0) from t");
}
