You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jr...@apache.org on 2017/01/13 23:48:00 UTC
[4/9] incubator-impala git commit: IMPALA-4716: Expr rewrite causes
IllegalStateException
IMPALA-4716: Expr rewrite causes IllegalStateException
The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.
The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.
Testing:
- Added a regression test to exprs.test
Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Reviewed-on: http://gerrit.cloudera.org:8080/5631
Reviewed-by: Marcel Kornacker <ma...@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/89a3d3c1
Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/89a3d3c1
Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/89a3d3c1
Branch: refs/heads/master
Commit: 89a3d3c1ebf85629bb20457a9672d71fbf2504ca
Parents: c4f2458
Author: Thomas Tauber-Marshall <tm...@cloudera.com>
Authored: Thu Jan 5 16:15:32 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Jan 13 04:44:07 2017 +0000
----------------------------------------------------------------------
.../java/org/apache/impala/analysis/CaseExpr.java | 14 ++++++++++----
.../java/org/apache/impala/common/TreeNode.java | 9 +++++++++
.../functional-query/queries/QueryTest/exprs.test | 16 ++++++++++++++++
3 files changed, 35 insertions(+), 4 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/89a3d3c1/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java b/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
index 4440690..3d96662 100644
--- a/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
+++ b/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
@@ -127,16 +127,17 @@ public class CaseExpr extends Expr {
if (candidate.isNullLiteral()) {
// An example case is DECODE(foo, NULL, bar), since NULLs are considered
// equal, this becomes CASE WHEN foo IS NULL THEN bar END.
- children_.add(encodedIsNull);
+ children_.add(encodedIsNull.clone());
} else {
children_.add(new BinaryPredicate(
- BinaryPredicate.Operator.EQ, encoded, candidate));
+ BinaryPredicate.Operator.EQ, encoded.clone(), candidate));
}
} else {
children_.add(new CompoundPredicate(CompoundPredicate.Operator.OR,
new CompoundPredicate(CompoundPredicate.Operator.AND,
- encodedIsNull, new IsNullPredicate(candidate, false)),
- new BinaryPredicate(BinaryPredicate.Operator.EQ, encoded, candidate)));
+ encodedIsNull.clone(), new IsNullPredicate(candidate, false)),
+ new BinaryPredicate(BinaryPredicate.Operator.EQ, encoded.clone(),
+ candidate)));
}
// Add the value
@@ -148,6 +149,11 @@ public class CaseExpr extends Expr {
hasElseExpr_ = true;
children_.add(decodeExpr.getChild(childIdx));
}
+
+ // Check that these exprs were cloned above, as reusing the same Expr object in
+ // different places can lead to bugs, eg. if the Expr has multiple parents, they may
+ // try to cast it to different types.
+ Preconditions.checkState(!contains(encoded) && !contains(encodedIsNull));
}
/**
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/89a3d3c1/fe/src/main/java/org/apache/impala/common/TreeNode.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/common/TreeNode.java b/fe/src/main/java/org/apache/impala/common/TreeNode.java
index 3231e33..730fd44 100644
--- a/fe/src/main/java/org/apache/impala/common/TreeNode.java
+++ b/fe/src/main/java/org/apache/impala/common/TreeNode.java
@@ -158,6 +158,15 @@ public abstract class TreeNode<NodeType extends TreeNode<NodeType>> {
}
/**
+ * Returns true if this node or any of its children is 'node'.
+ */
+ public boolean contains(NodeType node) {
+ if (this == node) return true;
+ for (NodeType child: children_) if (child.contains(node)) return true;
+ return false;
+ }
+
+ /**
* For each node in nodeList, return true if any subexpression satisfies
* contains('predicate').
*/
http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/89a3d3c1/testdata/workloads/functional-query/queries/QueryTest/exprs.test
----------------------------------------------------------------------
diff --git a/testdata/workloads/functional-query/queries/QueryTest/exprs.test b/testdata/workloads/functional-query/queries/QueryTest/exprs.test
index 4b1ba76..ed3c9d9 100644
--- a/testdata/workloads/functional-query/queries/QueryTest/exprs.test
+++ b/testdata/workloads/functional-query/queries/QueryTest/exprs.test
@@ -2649,3 +2649,19 @@ select count(*) from functional.alltypestiny group by concat(uuid(), "_test")
---- TYPES
BIGINT
====
+---- QUERY
+# IMPALA-4716: Tests that decode with 'when' values of different types is analyzed
+# correctly when expr rewrites are enabled.
+select decode(0, 1, 0, id, 1, 2) a from functional.alltypestiny order by a
+---- RESULTS
+1
+2
+2
+2
+2
+2
+2
+2
+---- TYPES
+TINYINT
+====