You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/09/16 21:41:00 UTC
[1/5] incubator-impala git commit: Increment version to
2.11.0-SNAPSHOT
Repository: incubator-impala
Updated Branches:
refs/heads/master 0a54cb5ec -> f0e79314f
Increment version to 2.11.0-SNAPSHOT
Change-Id: I2a60fbc5f2c1a9ba9697e6f1114bdf18997aa92c
Reviewed-on: http://gerrit.cloudera.org:8080/8080
Reviewed-by: Lars Volker <lv...@cloudera.com>
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/bdbfe22c
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/bdbfe22c
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/bdbfe22c
Branch: refs/heads/master
Commit: bdbfe22c7f19a6c5599b5bb7d59b8e035ffc6cc9
Parents: 0a54cb5
Author: Bharath Vissapragada <bh...@cloudera.com>
Authored: Thu Sep 14 22:49:04 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Sep 15 20:48:00 2017 +0000
----------------------------------------------------------------------
bin/save-version.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/bdbfe22c/bin/save-version.sh
----------------------------------------------------------------------
diff --git a/bin/save-version.sh b/bin/save-version.sh
index 3c23613..1b1507a 100755
--- a/bin/save-version.sh
+++ b/bin/save-version.sh
@@ -21,7 +21,7 @@
# Note: for internal (aka pre-release) versions, the version should have
# "-INTERNAL" appended. Parts of the code will look for this to distinguish
# between released and internal versions.
-VERSION=2.10.0-SNAPSHOT
+VERSION=2.11.0-SNAPSHOT
GIT_HASH=$(git rev-parse HEAD 2> /dev/null)
if [ -z $GIT_HASH ]
then
[2/5] incubator-impala git commit: IMPALA-5211: Simplifying nullif
conditional.
Posted by ta...@apache.org.
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 <al...@cloudera.com>
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 <ph...@cloudera.com>
Authored: Thu Aug 24 09:58:29 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
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");
}
[5/5] incubator-impala git commit: Revert "IMPALA-5589: change "set"
in impala-shell to show empty string for unset query options"
Posted by ta...@apache.org.
Revert "IMPALA-5589: change "set" in impala-shell to show empty string for unset query options"
Due to re-use of connections in the test infrastructure, this commit
is causing ASAN failures in the builds. That is being worked out
as part of IMPALA-5908, but, in the meanwhile, it's prudent
to revert.
This reverts commit 387bde0639ffd8ef580ccbf727152954e62bacbe.
Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
Reviewed-on: http://gerrit.cloudera.org:8080/8087
Reviewed-by: Dan Hecht <dh...@cloudera.com>
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/f0e79314
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/f0e79314
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/f0e79314
Branch: refs/heads/master
Commit: f0e79314fe9d9d3e920ad65c3ca6a4ef279e68fc
Parents: 5119ced
Author: Philip Zeyliger <ph...@cloudera.com>
Authored: Fri Sep 15 16:55:02 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Sep 16 04:06:53 2017 +0000
----------------------------------------------------------------------
be/src/service/query-options-test.cc | 12 ---
be/src/service/query-options.cc | 10 +-
be/src/service/query-options.h | 4 +-
.../functional-query/queries/QueryTest/set.test | 24 ++---
tests/hs2/hs2_test_suite.py | 10 +-
tests/hs2/test_hs2.py | 104 ++++++++-----------
tests/shell/test_shell_commandline.py | 2 -
7 files changed, 60 insertions(+), 106 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f0e79314/be/src/service/query-options-test.cc
----------------------------------------------------------------------
diff --git a/be/src/service/query-options-test.cc b/be/src/service/query-options-test.cc
index 1bec770..397df5a 100644
--- a/be/src/service/query-options-test.cc
+++ b/be/src/service/query-options-test.cc
@@ -135,16 +135,4 @@ TEST(QueryOptions, ParseQueryOptions) {
}
}
-TEST(QueryOptions, MapOptionalDefaultlessToEmptyString) {
- TQueryOptions options;
- map<string, string> map;
-
- TQueryOptionsToMap(options, &map);
- // No default set
- EXPECT_EQ(map["COMPRESSION_CODEC"], "");
- EXPECT_EQ(map["MT_DOP"], "");
- // Has defaults
- EXPECT_EQ(map["EXPLAIN_LEVEL"], "1");
-}
-
IMPALA_TEST_MAIN();
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f0e79314/be/src/service/query-options.cc
----------------------------------------------------------------------
diff --git a/be/src/service/query-options.cc b/be/src/service/query-options.cc
index e950d31..21327f3 100644
--- a/be/src/service/query-options.cc
+++ b/be/src/service/query-options.cc
@@ -67,13 +67,9 @@ void impala::TQueryOptionsToMap(const TQueryOptions& query_options,
map<string, string>* configuration) {
#define QUERY_OPT_FN(NAME, ENUM)\
{\
- if (query_options.__isset.NAME) { \
- stringstream val;\
- val << query_options.NAME;\
- (*configuration)[#ENUM] = val.str();\
- } else { \
- (*configuration)[#ENUM] = ""; \
- }\
+ stringstream val;\
+ val << query_options.NAME;\
+ (*configuration)[#ENUM] = val.str();\
}
QUERY_OPTS_TABLE
#undef QUERY_OPT_FN
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f0e79314/be/src/service/query-options.h
----------------------------------------------------------------------
diff --git a/be/src/service/query-options.h b/be/src/service/query-options.h
index 3dada7d..bb8c301 100644
--- a/be/src/service/query-options.h
+++ b/be/src/service/query-options.h
@@ -99,9 +99,7 @@ class TQueryOptions;
;
-/// Converts a TQueryOptions struct into a map of key, value pairs. Options that
-/// aren't set and lack defaults in common/thrift/ImpalaInternalService.thrift are
-/// mapped to the empty string.
+/// Converts a TQueryOptions struct into a map of key, value pairs.
void TQueryOptionsToMap(const TQueryOptions& query_options,
std::map<std::string, std::string>* configuration);
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f0e79314/testdata/workloads/functional-query/queries/QueryTest/set.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/set.test b/testdata/workloads/functional-query/queries/QueryTest/set.test
index 45c8343..a7004e0 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/set.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/set.test
@@ -20,13 +20,13 @@ set
'MEM_LIMIT','0'
'NUM_NODES','0'
'NUM_SCANNER_THREADS','0'
-'COMPRESSION_CODEC',''
+'COMPRESSION_CODEC','NONE'
'PARQUET_FILE_SIZE','0'
'REQUEST_POOL',''
-'RESERVATION_REQUEST_TIMEOUT',''
+'RESERVATION_REQUEST_TIMEOUT','0'
'RM_INITIAL_MEM','0'
'SYNC_DDL','0'
-'V_CPU_CORES',''
+'V_CPU_CORES','0'
---- TYPES
STRING, STRING
====
@@ -52,13 +52,13 @@ set;
'MEM_LIMIT','0'
'NUM_NODES','0'
'NUM_SCANNER_THREADS','0'
-'COMPRESSION_CODEC',''
+'COMPRESSION_CODEC','NONE'
'PARQUET_FILE_SIZE','0'
'REQUEST_POOL',''
-'RESERVATION_REQUEST_TIMEOUT',''
+'RESERVATION_REQUEST_TIMEOUT','0'
'RM_INITIAL_MEM','0'
'SYNC_DDL','0'
-'V_CPU_CORES',''
+'V_CPU_CORES','0'
---- TYPES
STRING, STRING
====
@@ -84,13 +84,13 @@ set;
'MEM_LIMIT','0'
'NUM_NODES','0'
'NUM_SCANNER_THREADS','0'
-'COMPRESSION_CODEC',''
+'COMPRESSION_CODEC','NONE'
'PARQUET_FILE_SIZE','0'
'REQUEST_POOL',''
-'RESERVATION_REQUEST_TIMEOUT',''
+'RESERVATION_REQUEST_TIMEOUT','0'
'RM_INITIAL_MEM','0'
'SYNC_DDL','0'
-'V_CPU_CORES',''
+'V_CPU_CORES','0'
---- TYPES
STRING, STRING
====
@@ -117,13 +117,13 @@ set;
'MEM_LIMIT','0'
'NUM_NODES','0'
'NUM_SCANNER_THREADS','0'
-'COMPRESSION_CODEC',''
+'COMPRESSION_CODEC','NONE'
'PARQUET_FILE_SIZE','1610612736'
'REQUEST_POOL',''
-'RESERVATION_REQUEST_TIMEOUT',''
+'RESERVATION_REQUEST_TIMEOUT','0'
'RM_INITIAL_MEM','0'
'SYNC_DDL','0'
-'V_CPU_CORES',''
+'V_CPU_CORES','0'
---- TYPES
STRING, STRING
====
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f0e79314/tests/hs2/hs2_test_suite.py
----------------------------------------------------------------------
diff --git a/tests/hs2/hs2_test_suite.py b/tests/hs2/hs2_test_suite.py
index 1b2f89f..2c2cd51 100644
--- a/tests/hs2/hs2_test_suite.py
+++ b/tests/hs2/hs2_test_suite.py
@@ -124,6 +124,7 @@ class HS2TestSuite(ImpalaTestSuite):
fetch_results_req.maxRows = size
fetch_results_resp = self.hs2_client.FetchResults(fetch_results_req)
HS2TestSuite.check_response(fetch_results_resp)
+ num_rows = size
if expected_num_rows is not None:
assert self.get_num_rows(fetch_results_resp.results) == expected_num_rows
return fetch_results_resp
@@ -227,12 +228,3 @@ class HS2TestSuite(ImpalaTestSuite):
sleep(interval)
assert False, 'Did not reach expected operation state %s in time, actual state was ' \
'%s' % (expected_state, get_operation_status_resp.operationState)
-
- def execute_statement(self, statement):
- """Executes statement and returns response, which is checked."""
- execute_statement_req = TCLIService.TExecuteStatementReq()
- execute_statement_req.sessionHandle = self.session_handle
- execute_statement_req.statement = statement
- execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
- HS2TestSuite.check_response(execute_statement_resp)
- return execute_statement_resp
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f0e79314/tests/hs2/test_hs2.py
----------------------------------------------------------------------
diff --git a/tests/hs2/test_hs2.py b/tests/hs2/test_hs2.py
index 2f984b1..f42b29a 100644
--- a/tests/hs2/test_hs2.py
+++ b/tests/hs2/test_hs2.py
@@ -45,48 +45,6 @@ class TestHS2(HS2TestSuite):
for k, v in open_session_req.configuration.items():
assert open_session_resp.configuration[k] == v
- @needs_session()
- def test_session_options_via_set(self):
- """
- Tests that session options are returned by a SET
- query and can be updated by a "SET k=v" query.
- """
- def get_session_options():
- """Returns dictionary of query options."""
- execute_statement_resp = self.execute_statement("SET")
-
- fetch_results_req = TCLIService.TFetchResultsReq()
- fetch_results_req.operationHandle = execute_statement_resp.operationHandle
- fetch_results_req.maxRows = 1000
- fetch_results_resp = self.hs2_client.FetchResults(fetch_results_req)
- TestHS2.check_response(fetch_results_resp)
-
- # Close the query
- close_operation_req = TCLIService.TCloseOperationReq()
- close_operation_req.operationHandle = execute_statement_resp.operationHandle
- TestHS2.check_response(self.hs2_client.CloseOperation(close_operation_req))
-
- # Results are returned in a columnar way:
- cols = fetch_results_resp.results.columns
- assert len(cols) == 2
- vals = dict(zip(cols[0].stringVal.values, cols[1].stringVal.values))
- return vals
-
- vals = get_session_options()
-
- # No default; should be empty string.
- assert vals["COMPRESSION_CODEC"] == ""
- # Has default of 0
- assert vals["SYNC_DDL"] == "0"
-
- # Set an option using SET
- self.execute_statement("SET COMPRESSION_CODEC=gzip")
-
- vals2 = get_session_options()
- assert vals2["COMPRESSION_CODEC"] == "GZIP"
- # Should be unchanged
- assert vals2["SYNC_DDL"] == "0"
-
def test_open_session_http_addr(self):
"""Check that OpenSession returns the coordinator's http address."""
open_session_req = TCLIService.TOpenSessionReq()
@@ -214,8 +172,11 @@ class TestHS2(HS2TestSuite):
@needs_session()
def test_get_operation_status(self):
"""Tests that GetOperationStatus returns a valid result for a running query"""
- statement = "SELECT COUNT(*) FROM functional.alltypes"
- execute_statement_resp = self.execute_statement(statement)
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
+ execute_statement_req.statement = "SELECT COUNT(*) FROM functional.alltypes"
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
+ TestHS2.check_response(execute_statement_resp)
get_operation_status_resp = \
self.get_operation_status(execute_statement_resp.operationHandle)
@@ -252,8 +213,11 @@ class TestHS2(HS2TestSuite):
def test_get_operation_status_error(self):
"""Tests that GetOperationStatus returns a valid result for a query that encountered
an error"""
- statement = "SELECT * FROM functional.alltypeserror"
- execute_statement_resp = self.execute_statement(statement)
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
+ execute_statement_req.statement = "SELECT * FROM functional.alltypeserror"
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
+ TestHS2.check_response(execute_statement_resp)
get_operation_status_resp = self.wait_for_operation_state( \
execute_statement_resp.operationHandle, TCLIService.TOperationState.ERROR_STATE)
@@ -368,15 +332,17 @@ class TestHS2(HS2TestSuite):
assert "Sql Statement: GET_SCHEMAS" in profile_page
assert "Query Type: DDL" in profile_page
-
@needs_session(conf_overlay={"idle_session_timeout": "5"})
def test_get_operation_status_session_timeout(self):
"""Regression test for IMPALA-4488: GetOperationStatus() would not keep a session
alive"""
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
# Choose a long-running query so that it can't finish before the session timeout.
- statement = """select * from functional.alltypes a
+ execute_statement_req.statement = """select * from functional.alltypes a
join functional.alltypes b join functional.alltypes c"""
- execute_statement_resp = self.execute_statement(statement)
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
+ TestHS2.check_response(execute_statement_resp)
now = time.time()
# Loop until the session would be timed-out if IMPALA-4488 had not been fixed.
@@ -388,7 +354,11 @@ class TestHS2(HS2TestSuite):
time.sleep(0.1)
def get_log(self, query_stmt):
- execute_statement_resp = self.execute_statement(query_stmt)
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
+ execute_statement_req.statement = query_stmt
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
+ TestHS2.check_response(execute_statement_resp)
# Fetch results to make sure errors are generated. Errors are only guaranteed to be
# seen by the coordinator after FetchResults() returns eos.
@@ -419,8 +389,11 @@ class TestHS2(HS2TestSuite):
@needs_session()
def test_get_exec_summary(self):
- statement = "SELECT COUNT(1) FROM functional.alltypes"
- execute_statement_resp = self.execute_statement(statement)
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
+ execute_statement_req.statement = "SELECT COUNT(1) FROM functional.alltypes"
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
+ TestHS2.check_response(execute_statement_resp)
exec_summary_req = ImpalaHiveServer2Service.TGetExecSummaryReq()
exec_summary_req.operationHandle = execute_statement_resp.operationHandle
@@ -442,15 +415,18 @@ class TestHS2(HS2TestSuite):
@needs_session()
def test_get_profile(self):
- statement = "SELECT COUNT(2) FROM functional.alltypes"
- execute_statement_resp = self.execute_statement(statement)
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
+ execute_statement_req.statement = "SELECT COUNT(2) FROM functional.alltypes"
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
+ TestHS2.check_response(execute_statement_resp)
get_profile_req = ImpalaHiveServer2Service.TGetRuntimeProfileReq()
get_profile_req.operationHandle = execute_statement_resp.operationHandle
get_profile_req.sessionHandle = self.session_handle
get_profile_resp = self.hs2_client.GetRuntimeProfile(get_profile_req)
TestHS2.check_response(get_profile_resp)
- assert statement in get_profile_resp.profile
+ assert execute_statement_req.statement in get_profile_resp.profile
# If ExecuteStatement() has completed but the results haven't been fetched yet, the
# query must have at least reached RUNNING.
assert "Query State: RUNNING" in get_profile_resp.profile or \
@@ -463,7 +439,7 @@ class TestHS2(HS2TestSuite):
get_profile_resp = self.hs2_client.GetRuntimeProfile(get_profile_req)
TestHS2.check_response(get_profile_resp)
- assert statement in get_profile_resp.profile
+ assert execute_statement_req.statement in get_profile_resp.profile
# After fetching the results, we must be in state FINISHED.
assert "Query State: FINISHED" in get_profile_resp.profile, get_profile_resp.profile
@@ -473,20 +449,26 @@ class TestHS2(HS2TestSuite):
get_profile_resp = self.hs2_client.GetRuntimeProfile(get_profile_req)
TestHS2.check_response(get_profile_resp)
- assert statement in get_profile_resp.profile
+ assert execute_statement_req.statement in get_profile_resp.profile
assert "Query State: FINISHED" in get_profile_resp.profile, get_profile_resp.profile
@needs_session(conf_overlay={"use:database": "functional"})
def test_change_default_database(self):
- statement = "SELECT 1 FROM alltypes LIMIT 1"
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
+ execute_statement_req.statement = "SELECT 1 FROM alltypes LIMIT 1"
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
# Will fail if there's no table called 'alltypes' in the database
- self.execute_statement(statement)
+ TestHS2.check_response(execute_statement_resp)
@needs_session(conf_overlay={"use:database": "FUNCTIONAL"})
def test_change_default_database_case_insensitive(self):
- statement = "SELECT 1 FROM alltypes LIMIT 1"
+ execute_statement_req = TCLIService.TExecuteStatementReq()
+ execute_statement_req.sessionHandle = self.session_handle
+ execute_statement_req.statement = "SELECT 1 FROM alltypes LIMIT 1"
+ execute_statement_resp = self.hs2_client.ExecuteStatement(execute_statement_req)
# Will fail if there's no table called 'alltypes' in the database
- self.execute_statement(statement)
+ TestHS2.check_response(execute_statement_resp)
@needs_session(conf_overlay={"use:database": "doesnt-exist"})
def test_bad_default_database(self):
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/f0e79314/tests/shell/test_shell_commandline.py
----------------------------------------------------------------------
diff --git a/tests/shell/test_shell_commandline.py b/tests/shell/test_shell_commandline.py
index 0602e77..488de49 100644
--- a/tests/shell/test_shell_commandline.py
+++ b/tests/shell/test_shell_commandline.py
@@ -248,8 +248,6 @@ class TestImpalaShell(ImpalaTestSuite):
assert 'MEM_LIMIT: [0]' in result_set.stdout
# test to check that explain_level is 1
assert 'EXPLAIN_LEVEL: [1]' in result_set.stdout
- # test to check that configs without defaults show up as []
- assert 'COMPRESSION_CODEC: []' in result_set.stdout
# test values displayed after setting value
args = '-q "set mem_limit=1g;set"'
result_set = run_impala_shell_cmd(args)
[4/5] incubator-impala git commit: IMPALA-5199: prevent hang on empty
row batch exchange
Posted by ta...@apache.org.
IMPALA-5199: prevent hang on empty row batch exchange
The error path where delivery of "eos" fails now behaves
the same as if delivery of a row batch fails.
Testing:
Added a timeout test where the leaf fragments return 0 rows. Before
the change this reproduced the hang.
Change-Id: Ib370ebe44e3bb34d3f0fb9f05aa6386eb91c8645
Reviewed-on: http://gerrit.cloudera.org:8080/8005
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
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/5119ced5
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/5119ced5
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/5119ced5
Branch: refs/heads/master
Commit: 5119ced50c0e0c4001621c9d4da598c187bdb580
Parents: 491822f
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Thu Sep 7 16:28:46 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Sep 16 00:50:07 2017 +0000
----------------------------------------------------------------------
be/src/runtime/data-stream-mgr.cc | 17 +++++++++++++----
.../queries/QueryTest/exchange-delays.test | 10 ++++++++++
2 files changed, 23 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5119ced5/be/src/runtime/data-stream-mgr.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/data-stream-mgr.cc b/be/src/runtime/data-stream-mgr.cc
index a161ad3..9af8384 100644
--- a/be/src/runtime/data-stream-mgr.cc
+++ b/be/src/runtime/data-stream-mgr.cc
@@ -197,10 +197,19 @@ Status DataStreamMgr::CloseSender(const TUniqueId& fragment_instance_id,
PlanNodeId dest_node_id, int sender_id) {
VLOG_FILE << "CloseSender(): fragment_instance_id=" << fragment_instance_id
<< ", node=" << dest_node_id;
- bool unused;
+ Status status;
+ bool already_unregistered;
shared_ptr<DataStreamRecvr> recvr = FindRecvrOrWait(fragment_instance_id, dest_node_id,
- &unused);
- if (recvr.get() != NULL) recvr->RemoveSender(sender_id);
+ &already_unregistered);
+ if (recvr == nullptr) {
+ // Was not able to notify the receiver that this was the end of stream. Notify the
+ // sender that this failed so that they can take appropriate action (i.e. failing
+ // the query).
+ status = already_unregistered ? Status::OK() :
+ Status(TErrorCode::DATASTREAM_SENDER_TIMEOUT, PrintId(fragment_instance_id));
+ } else {
+ recvr->RemoveSender(sender_id);
+ }
{
// Remove any closed streams that have been in the cache for more than
@@ -221,7 +230,7 @@ Status DataStreamMgr::CloseSender(const TUniqueId& fragment_instance_id,
<< PrettyPrinter::Print(MonotonicMillis() - now, TUnit::TIME_MS);
}
}
- return Status::OK();
+ return status;
}
Status DataStreamMgr::DeregisterRecvr(
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/5119ced5/testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test b/testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test
index 0dac1d9..b1f6f75 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test
@@ -8,3 +8,13 @@ from tpch.lineitem
---- CATCH
Sender timed out waiting for receiver fragment instance
====
+---- QUERY
+# IMPALA-5199: Query with zero rows sent over exchange.
+select l_orderkey, count(*)
+from tpch.lineitem
+where l_linenumber = -1
+group by l_orderkey
+---- RESULTS
+---- CATCH
+Sender timed out waiting for receiver fragment instance
+====
[3/5] incubator-impala git commit: IMPALA-3877: support unpatched LLVM
Posted by ta...@apache.org.
IMPALA-3877: support unpatched LLVM
The p1 patch we use for LLVM avoided merging of structurally
identical Struct types in unpredictable ways when linking in
IR UDF modules. This avoided hitting type assertions when
generating calls to IR UDfs.
This implements an alternative solution, which is to bitcast
the arguments when calling IR UDFs. This means we do not need to carry
the patch when we upgrade LLVM.
Testing:
Ran core tests with unpatched LLVM 3.8, including the IR UDF test that
originally required the patch to pass.
Change-Id: I3dfbe44ed8a1464b9b0991fd54e72b194ad6155d
Reviewed-on: http://gerrit.cloudera.org:8080/7973
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
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/491822f0
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/491822f0
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/491822f0
Branch: refs/heads/master
Commit: 491822f0e3c8e5b4890533924d72375caad4bc74
Parents: 02302b7
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Fri Aug 25 11:54:14 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Sat Sep 16 00:46:05 2017 +0000
----------------------------------------------------------------------
be/src/codegen/CMakeLists.txt | 1 +
be/src/codegen/codegen-anyval.cc | 14 +++++--
be/src/codegen/codegen-util.cc | 76 ++++++++++++++++++++++++++++++++++
be/src/codegen/codegen-util.h | 65 +++++++++++++++++++++++++++++
be/src/codegen/llvm-codegen.h | 4 +-
be/src/exprs/expr-codegen-test.cc | 3 +-
6 files changed, 158 insertions(+), 5 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/491822f0/be/src/codegen/CMakeLists.txt
----------------------------------------------------------------------
diff --git a/be/src/codegen/CMakeLists.txt b/be/src/codegen/CMakeLists.txt
index e640009..56228a2 100644
--- a/be/src/codegen/CMakeLists.txt
+++ b/be/src/codegen/CMakeLists.txt
@@ -30,6 +30,7 @@ add_library(CodeGen
codegen-anyval.cc
codegen-callgraph.cc
codegen-symbol-emitter.cc
+ codegen-util.cc
llvm-codegen.cc
instruction-counter.cc
${IR_SSE_C_FILE}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/491822f0/be/src/codegen/codegen-anyval.cc
----------------------------------------------------------------------
diff --git a/be/src/codegen/codegen-anyval.cc b/be/src/codegen/codegen-anyval.cc
index bdd49c9..b116cfe 100644
--- a/be/src/codegen/codegen-anyval.cc
+++ b/be/src/codegen/codegen-anyval.cc
@@ -17,6 +17,8 @@
#include "codegen/codegen-anyval.h"
+#include "codegen/codegen-util.h"
+
#include "common/names.h"
using namespace impala;
@@ -138,15 +140,21 @@ Value* CodegenAnyVal::CreateCall(LlvmCodeGen* cg, LlvmBuilder* builder, Function
cg->CreateEntryBlockAlloca(*builder, ret_type, name) : result_ptr;
vector<Value*> new_args = args.vec();
new_args.insert(new_args.begin(), ret_ptr);
- builder->CreateCall(fn, new_args);
+ // Bitcasting the args is often necessary when calling an IR UDF because the types
+ // in the IR module may have been renamed while linking. Bitcasting them avoids a
+ // type assertion.
+ CodeGenUtil::CreateCallWithBitCasts(builder, fn, new_args);
// If 'result_ptr' was specified, we're done. Otherwise load and return the result.
if (result_ptr != NULL) return NULL;
return builder->CreateLoad(ret_ptr, name);
} else {
// Function returns *Val normally (note that it could still be returning a DecimalVal,
- // since we generate non-complaint functions)
- Value* ret = builder->CreateCall(fn, args, name);
+ // since we generate non-compliant functions).
+ // Bitcasting the args is often necessary when calling an IR UDF because the types
+ // in the IR module may have been renamed while linking. Bitcasting them avoids a
+ // type assertion.
+ Value* ret = CodeGenUtil::CreateCallWithBitCasts(builder, fn, args, name);
if (result_ptr == NULL) return ret;
builder->CreateStore(ret, result_ptr);
return NULL;
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/491822f0/be/src/codegen/codegen-util.cc
----------------------------------------------------------------------
diff --git a/be/src/codegen/codegen-util.cc b/be/src/codegen/codegen-util.cc
new file mode 100644
index 0000000..9890ac2
--- /dev/null
+++ b/be/src/codegen/codegen-util.cc
@@ -0,0 +1,76 @@
+// 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.
+
+#include "codegen/codegen-util.h"
+
+#include <cctype>
+
+#include "common/names.h"
+
+using namespace llvm;
+using std::isdigit;
+
+namespace impala {
+
+CallInst* CodeGenUtil::CreateCallWithBitCasts(LlvmBuilder* builder,
+ Function *callee, ArrayRef<Value*> args, const Twine& name) {
+ vector<Value*> bitcast_args;
+ bitcast_args.reserve(args.size());
+ Function::arg_iterator fn_arg = callee->arg_begin();
+ for (Value* arg: args) {
+ bitcast_args.push_back(
+ CheckedBitCast(builder, arg, fn_arg->getType(), "create_call_bitcast"));
+ ++fn_arg;
+ }
+ return builder->CreateCall(callee, bitcast_args, name);
+}
+
+Value* CodeGenUtil::CheckedBitCast(LlvmBuilder* builder, Value* value,
+ Type* dst_type, const Twine& name) {
+ DCHECK(TypesAreStructurallyIdentical(value->getType(), dst_type))
+ << Print(value->getType()) << " " << Print(dst_type);
+ return builder->CreateBitCast(value, dst_type, name);
+}
+
+bool CodeGenUtil::TypesAreStructurallyIdentical(Type* t1, Type* t2) {
+ // All primitive types are deduplicated by LLVM, so we can just compare the pointers.
+ if (t1 == t2) return true;
+ // Derived types are structurally identical if they are the same kind of compound type
+ // and the elements are structurally identical. Check to see which of the Type
+ // subclasses t1 belongs to.
+ if (t1->isPointerTy()) {
+ if (!t2->isPointerTy()) return false;
+ } else if (t1->isStructTy()) {
+ if (!t2->isStructTy()) return false;
+ } else if (t1->isArrayTy()) {
+ if (!t2->isArrayTy()) return false;
+ } else if (t1->isFunctionTy()) {
+ if (!t2->isFunctionTy()) return false;
+ } else {
+ DCHECK(t1->isVectorTy()) << Print(t1);
+ if (!t2->isVectorTy()) return false;
+ }
+ if (t1->getNumContainedTypes() != t2->getNumContainedTypes()) return false;
+ for (int i = 0; i < t1->getNumContainedTypes(); ++i) {
+ if (!TypesAreStructurallyIdentical(
+ t1->getContainedType(i), t2->getContainedType(i))) {
+ return false;
+ }
+ }
+ return true;
+}
+}
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/491822f0/be/src/codegen/codegen-util.h
----------------------------------------------------------------------
diff --git a/be/src/codegen/codegen-util.h b/be/src/codegen/codegen-util.h
new file mode 100644
index 0000000..0a2199b
--- /dev/null
+++ b/be/src/codegen/codegen-util.h
@@ -0,0 +1,65 @@
+// 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.
+
+#ifndef IMPALA_CODEGEN_CODEGEN_UTIL_H
+#define IMPALA_CODEGEN_CODEGEN_UTIL_H
+
+#include <vector>
+#include <llvm/IR/IRBuilder.h>
+
+#include "codegen/llvm-codegen.h"
+
+namespace impala {
+
+/// Miscellaneous codegen utility functions that don't depend on the rest of the Impala
+/// codegen infrastructure.
+class CodeGenUtil {
+ public:
+ /// Wrapper around IRBuilder::CreateCall() that automatically bitcasts arguments
+ /// using CheckedBitCast(). This should be used instead of IRBuilder::CreateCall()
+ /// when calling functions from a linked module because the LLVM linker may merge
+ /// different struct types with the same memory layout during linking. E.g. if the
+ /// IR UDF module has type TinyIntVal that has the same memory layout as BooleanVal:
+ /// {i8, i8}, then the linker may substitute references to TinyIntVal with BooleanVal
+ /// in the IR UDF. Calling a function which has a BooleanVal* argument with a TinyInt*
+ /// argument without bitcasting then would result in hitting an internal LLVM assertion.
+ static llvm::CallInst* CreateCallWithBitCasts(LlvmBuilder* builder,
+ llvm::Function* callee, llvm::ArrayRef<llvm::Value*> args,
+ const llvm::Twine& name="");
+
+ /// Same as IRBuilder::CreateBitCast() except that it checks that the types are either
+ /// exactly the same, or, if they are both struct types (or pointers to struct types),
+ /// that they are structurally identical. Either returns 'value' if no conversion is
+ /// necessary, or returns a bitcast instruction converting 'value' to 'dst_type'.
+ static llvm::Value* CheckedBitCast(LlvmBuilder* builder,
+ llvm::Value* value, llvm::Type* dst_type, const llvm::Twine& name);
+
+ /// Return true if the two types are structurally identical.
+ static bool TypesAreStructurallyIdentical(llvm::Type *t1, llvm::Type *t2);
+
+ /// Returns the string representation of a llvm::Value* or llvm::Type*
+ template <typename T> static std::string Print(T* value_or_type) {
+ std::string str;
+ llvm::raw_string_ostream stream(str);
+ value_or_type->print(stream);
+ return str;
+ }
+
+};
+}
+
+#endif
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/491822f0/be/src/codegen/llvm-codegen.h
----------------------------------------------------------------------
diff --git a/be/src/codegen/llvm-codegen.h b/be/src/codegen/llvm-codegen.h
index dca344a..ea31372 100644
--- a/be/src/codegen/llvm-codegen.h
+++ b/be/src/codegen/llvm-codegen.h
@@ -310,7 +310,9 @@ class LlvmCodeGen {
LibCacheEntry** cache_entry);
/// Replaces all instructions in 'caller' that call 'target_name' with a call
- /// instruction to 'new_fn'. Returns the number of call sites updated.
+ /// instruction to 'new_fn'. The argument types of 'new_fn' must exactly match
+ /// the argument types of the function to be replaced. Returns the number of
+ /// call sites updated.
///
/// 'target_name' must be a substring of the mangled symbol of the function to be
/// replaced. This usually means that the unmangled function name is sufficient.
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/491822f0/be/src/exprs/expr-codegen-test.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-codegen-test.cc b/be/src/exprs/expr-codegen-test.cc
index 9ea865a..4b13073 100644
--- a/be/src/exprs/expr-codegen-test.cc
+++ b/be/src/exprs/expr-codegen-test.cc
@@ -55,6 +55,7 @@ DecimalVal TestGetFnAttrs(
#ifndef IR_COMPILE
#include "testutil/gtest-util.h"
+#include "codegen/codegen-util.h"
#include "codegen/llvm-codegen.h"
#include "common/init.h"
#include "exprs/anyval-util.h"
@@ -328,7 +329,7 @@ TEST_F(ExprCodegenTest, TestInlineConstFnAttrs) {
EXPECT_EQ(replaced, 9);
ResetVerification(codegen.get());
verification_succeeded = VerifyFunction(codegen.get(), fn);
- EXPECT_TRUE(verification_succeeded) << LlvmCodeGen::Print(fn);
+ EXPECT_TRUE(verification_succeeded) << CodeGenUtil::Print(fn);
// Compile module
fn = codegen->FinalizeFunction(fn);