IMPALA-3856,IMPALA-3871: Fix BinaryPredicate normalization for Kudu Change-Id: Iae7612433a2e27f8887abe6624f9ee0f4867b934 Reviewed-on: http://gerrit.cloudera.org:8080/3986 Reviewed-by: Matthew Jacobs <[email protected]> Tested-by: Internal 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/0983da92 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/0983da92 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/0983da92 Branch: refs/heads/master Commit: 0983da92bad9407c864f884973f9530e0059036d Parents: 50e2124 Author: Matthew Jacobs <[email protected]> Authored: Mon Jul 18 12:42:26 2016 -0700 Committer: Internal Jenkins <[email protected]> Committed: Thu Aug 18 04:03:00 2016 +0000 ---------------------------------------------------------------------- .../impala/analysis/BinaryPredicate.java | 31 +++++++++++++------- .../java/com/cloudera/impala/analysis/Expr.java | 15 +++++----- .../cloudera/impala/analysis/LiteralExpr.java | 10 +++---- .../queries/PlannerTest/kudu.test | 21 +++++++++++++ 4 files changed, 54 insertions(+), 23 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0983da92/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java b/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java index 02b5c12..35d03e1 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/BinaryPredicate.java @@ -115,24 +115,35 @@ public class BinaryPredicate extends Predicate { } /** - * Returns a version of 'predicate' that always has the SlotRef, if there is one, on the - * left and the other expression on the right. This also folds constant children of the - * predicate into literals, if possible. - * Returns null if this is not a SlotRef comparison. + * Normalizes a 'predicate' consisting of an uncast SlotRef and a constant Expr into + * the following form: <SlotRef> <Op> <LiteralExpr> + * If 'predicate' cannot be expressed in this way, null is returned. */ public static BinaryPredicate normalizeSlotRefComparison(BinaryPredicate predicate, - Analyzer analyzer) - throws AnalysisException { - SlotRef ref = predicate.getBoundSlot(); + Analyzer analyzer) { + SlotRef ref = null; + if (predicate.getChild(0) instanceof SlotRef) { + ref = (SlotRef) predicate.getChild(0); + } else if (predicate.getChild(1) instanceof SlotRef) { + ref = (SlotRef) predicate.getChild(1); + } + if (ref == null) return null; - if (ref != predicate.getChild(0).unwrapSlotRef(true)) { - Preconditions.checkState(ref == predicate.getChild(1).unwrapSlotRef(true)); + if (ref != predicate.getChild(0)) { + Preconditions.checkState(ref == predicate.getChild(1)); predicate = new BinaryPredicate(predicate.getOp().converse(), ref, predicate.getChild(0)); predicate.analyzeNoThrow(analyzer); } - predicate.foldConstantChildren(analyzer); + + try { + predicate.foldConstantChildren(analyzer); + } catch (AnalysisException ex) { + // Throws if the expression cannot be evaluated by the BE. + return null; + } predicate.analyzeNoThrow(analyzer); + if (!(predicate.getChild(1) instanceof LiteralExpr)) return null; return predicate; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0983da92/fe/src/main/java/com/cloudera/impala/analysis/Expr.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/analysis/Expr.java b/fe/src/main/java/com/cloudera/impala/analysis/Expr.java index 12763f7..91d240a 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/Expr.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/Expr.java @@ -1190,16 +1190,15 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl } /** - * Evaluate in the BE the children of 'this' that are constant expressions and - * substitute them with the evaluation result as LiteralExprs. Modifies 'this' - * in place and does not re-analyze it. Hence, it is not safe to evaluate the - * modified expr in the BE as the resolved fn_ may be incorrect given the new - * arguments. + * For children of 'this' that are constant expressions capable of being expressed + * as LiteralExprs, evaluate them in the BE and substitute the child with the + * resulting LiteralExpr. Modifies 'this' in place and does not re-analyze it. Hence, + * it is not safe to evaluate the modified expr in the BE as the resolved fn_ may be + * incorrect given the new arguments. * * Throws an AnalysisException if the evaluation fails in the BE. * - * TODO: Used only during partition pruning. Convert it to a generic constant - * expression folding function to be used during the analysis. + * TODO: Convert to a generic constant expr folding function to be used during analysis. */ public void foldConstantChildren(Analyzer analyzer) throws AnalysisException { Preconditions.checkState(isAnalyzed_); @@ -1208,7 +1207,7 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl Expr child = getChild(i); if (child.isLiteral() || !child.isConstant()) continue; LiteralExpr literalExpr = LiteralExpr.create(child, analyzer.getQueryCtx()); - Preconditions.checkNotNull(literalExpr); + if (literalExpr == null) continue; setChild(i, literalExpr); } isAnalyzed_ = false; http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0983da92/fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java b/fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java index a48b46c..d387b54 100644 --- a/fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java +++ b/fe/src/main/java/com/cloudera/impala/analysis/LiteralExpr.java @@ -49,7 +49,8 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr } /** - * Returns an analyzed literal of 'type'. + * Returns an analyzed literal of 'type'. Returns null for types that cannot be + * expressed as literals, e.g. TIMESTAMP. */ public static LiteralExpr create(String value, Type type) throws AnalysisException { Preconditions.checkArgument(type.isValid()); @@ -79,8 +80,7 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr case DATETIME: case TIMESTAMP: // TODO: we support TIMESTAMP but no way to specify it in SQL. - throw new AnalysisException( - "DATE/DATETIME/TIMESTAMP literals not supported: " + value); + return null; default: Preconditions.checkState(false, String.format("Literals of type '%s' not supported.", type.toSql())); @@ -151,6 +151,7 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr /** * Evaluates the given constant expr and returns its result as a LiteralExpr. * Assumes expr has been analyzed. Returns constExpr if is it already a LiteralExpr. + * Returns null for types that cannot be expressed as literals, e.g. TIMESTAMP. * TODO: Support non-scalar types. */ public static LiteralExpr create(Expr constExpr, TQueryCtx queryCtx) @@ -216,8 +217,7 @@ public abstract class LiteralExpr extends Expr implements Comparable<LiteralExpr case DATE: case DATETIME: case TIMESTAMP: - throw new AnalysisException( - "DATE/DATETIME/TIMESTAMP literals not supported: " + constExpr.toSql()); + return null; default: Preconditions.checkState(false, String.format("Literals of type '%s' not supported.", http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/0983da92/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test ---------------------------------------------------------------------- diff --git a/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test b/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test index 6118c00..7d139d2 100644 --- a/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test +++ b/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test @@ -147,3 +147,24 @@ NODE 0: 00:SCAN KUDU [functional_kudu.testtbl] predicates: name IS NULL, CAST(sin(id) AS BOOLEAN) = TRUE ==== +# IMPALA-3856: KuduScanNode crash when pushing predicates including a cast +select o_orderkey from tpch_kudu.orders where o_orderkey < 10.0 order by 1 +---- PLAN +01:SORT +| order by: o_orderkey ASC +| +00:SCAN KUDU [tpch_kudu.orders] + predicates: o_orderkey < 10.0 +==== +# IMPALA-3871: Casting literals to TIMESTAMP throw when pushed to KuduScanNode +select t.c from + (select cast(o_orderdate as timestamp) c from tpch_kudu.orders where o_orderkey < 10) t +where t.c <= cast('1995-01-01 00:00:00' as timestamp) order by c +---- PLAN +01:SORT +| order by: c ASC +| +00:SCAN KUDU [tpch_kudu.orders] + predicates: CAST(o_orderdate AS TIMESTAMP) <= CAST('1995-01-01 00:00:00' AS TIMESTAMP) + kudu predicates: o_orderkey <= 9 +====
