IMPALA-4213: Fix Kudu predicates that need constant folding Folding const exprs where there were implicit casts on the slot resulted in the predicate not being pushed to Kudu.
Change-Id: I3bab22d90ee00a054c847de6c734b4f24a3f5a85 Reviewed-on: http://gerrit.cloudera.org:8080/4613 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/2b5d1344 Tree: http://git-wip-us.apache.org/repos/asf/incubator-impala/tree/2b5d1344 Diff: http://git-wip-us.apache.org/repos/asf/incubator-impala/diff/2b5d1344 Branch: refs/heads/master Commit: 2b5d1344c9a0a86807a54c7bdaa643c531a52054 Parents: 485022a Author: Matthew Jacobs <[email protected]> Authored: Mon Oct 3 17:00:48 2016 -0700 Committer: Internal Jenkins <[email protected]> Committed: Thu Oct 6 04:06:38 2016 +0000 ---------------------------------------------------------------------- .../apache/impala/analysis/BinaryPredicate.java | 39 +----------- .../java/org/apache/impala/analysis/Expr.java | 20 +++--- .../org/apache/impala/planner/KuduScanNode.java | 65 +++++++++++++++----- .../queries/PlannerTest/kudu.test | 11 ++++ 4 files changed, 78 insertions(+), 57 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/2b5d1344/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java b/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java index ccb09de..78fff56 100644 --- a/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java +++ b/fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java @@ -21,9 +21,6 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import org.apache.impala.catalog.Db; import org.apache.impala.catalog.Function.CompareMode; import org.apache.impala.catalog.ScalarFunction; @@ -34,6 +31,9 @@ import org.apache.impala.common.Reference; import org.apache.impala.extdatasource.thrift.TComparisonOp; import org.apache.impala.thrift.TExprNode; import org.apache.impala.thrift.TExprNodeType; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.base.Objects; import com.google.common.base.Preconditions; import com.google.common.base.Predicates; @@ -114,39 +114,6 @@ public class BinaryPredicate extends Predicate { } } - /** - * 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) { - 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)) { - Preconditions.checkState(ref == predicate.getChild(1)); - predicate = new BinaryPredicate(predicate.getOp().converse(), ref, - predicate.getChild(0)); - predicate.analyzeNoThrow(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; - } - private Operator op_; public Operator getOp() { return op_; } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/2b5d1344/fe/src/main/java/org/apache/impala/analysis/Expr.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/analysis/Expr.java b/fe/src/main/java/org/apache/impala/analysis/Expr.java index 35a87f7..6eed7a8 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Expr.java +++ b/fe/src/main/java/org/apache/impala/analysis/Expr.java @@ -24,9 +24,6 @@ import java.util.List; import java.util.ListIterator; import java.util.Set; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import org.apache.impala.catalog.Catalog; import org.apache.impala.catalog.Function; import org.apache.impala.catalog.Function.CompareMode; @@ -37,6 +34,9 @@ import org.apache.impala.common.AnalysisException; import org.apache.impala.common.TreeNode; import org.apache.impala.thrift.TExpr; import org.apache.impala.thrift.TExprNode; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.base.Joiner; import com.google.common.base.Objects; import com.google.common.base.Preconditions; @@ -1192,9 +1192,8 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl /** * For children of 'this' that are constant expressions and the type of which has a * LiteralExpr subclass, 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. + * resulting LiteralExpr. Modifies 'this' in place. If any children are folded, this + * Expr is reset and re-analyzed. * * Throws an AnalysisException if the evaluation fails in the BE. * @@ -1203,14 +1202,21 @@ abstract public class Expr extends TreeNode<Expr> implements ParseNode, Cloneabl public void foldConstantChildren(Analyzer analyzer) throws AnalysisException { Preconditions.checkState(isAnalyzed_); Preconditions.checkNotNull(analyzer); + + int numFolded = 0; for (int i = 0; i < children_.size(); ++i) { Expr child = getChild(i); if (child.isLiteral() || !child.isConstant()) continue; LiteralExpr literalExpr = LiteralExpr.create(child, analyzer.getQueryCtx()); if (literalExpr == null) continue; setChild(i, literalExpr); + ++numFolded; + } + + if (numFolded > 0) { + reset(); + analyze(analyzer); } - isAnalyzed_ = false; } /** http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/2b5d1344/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java ---------------------------------------------------------------------- diff --git a/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java b/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java index a2473ae..64ef822 100644 --- a/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/KuduScanNode.java @@ -22,18 +22,6 @@ import java.util.List; import java.util.ListIterator; import java.util.Set; -import org.apache.kudu.ColumnSchema; -import org.apache.kudu.Schema; -import org.apache.kudu.client.KuduClient; -import org.apache.kudu.client.KuduClient.KuduClientBuilder; -import org.apache.kudu.client.KuduPredicate; -import org.apache.kudu.client.KuduPredicate.ComparisonOp; -import org.apache.kudu.client.KuduScanToken; -import org.apache.kudu.client.KuduScanToken.KuduScanTokenBuilder; -import org.apache.kudu.client.LocatedTablet; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import org.apache.impala.analysis.Analyzer; import org.apache.impala.analysis.BinaryPredicate; import org.apache.impala.analysis.BoolLiteral; @@ -46,6 +34,7 @@ import org.apache.impala.analysis.SlotRef; import org.apache.impala.analysis.StringLiteral; import org.apache.impala.analysis.TupleDescriptor; import org.apache.impala.catalog.KuduTable; +import org.apache.impala.common.AnalysisException; import org.apache.impala.common.ImpalaRuntimeException; import org.apache.impala.thrift.TExplainLevel; import org.apache.impala.thrift.TKuduScanNode; @@ -55,7 +44,20 @@ import org.apache.impala.thrift.TPlanNodeType; import org.apache.impala.thrift.TScanRange; import org.apache.impala.thrift.TScanRangeLocation; import org.apache.impala.thrift.TScanRangeLocations; +import org.apache.kudu.ColumnSchema; +import org.apache.kudu.Schema; +import org.apache.kudu.client.KuduClient; +import org.apache.kudu.client.KuduClient.KuduClientBuilder; +import org.apache.kudu.client.KuduPredicate; +import org.apache.kudu.client.KuduPredicate.ComparisonOp; +import org.apache.kudu.client.KuduScanToken; +import org.apache.kudu.client.KuduScanToken.KuduScanTokenBuilder; +import org.apache.kudu.client.LocatedTablet; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + import com.google.common.base.Charsets; +import com.google.common.base.Preconditions; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Sets; @@ -283,9 +285,9 @@ public class KuduScanNode extends ScanNode { BinaryPredicate predicate = (BinaryPredicate) expr; // TODO KUDU-931 look into handling implicit/explicit casts on the SlotRef. - predicate = BinaryPredicate.normalizeSlotRefComparison(predicate, analyzer); + predicate = normalizeSlotRefComparison(predicate, analyzer); if (predicate == null) return false; - ComparisonOp op = getKuduOperator(((BinaryPredicate)predicate).getOp()); + ComparisonOp op = getKuduOperator(predicate.getOp()); if (op == null) return false; SlotRef ref = (SlotRef) predicate.getChild(0); @@ -355,4 +357,39 @@ public class KuduScanNode extends ScanNode { default: return null; } } + + + /** + * Normalizes and returns a copy of '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. + */ + private static BinaryPredicate normalizeSlotRefComparison(BinaryPredicate predicate, + Analyzer analyzer) { + try { + predicate = (BinaryPredicate) predicate.clone(); + predicate.foldConstantChildren(analyzer); + } catch (AnalysisException ex) { + // Throws if the expression cannot be evaluated by the BE. + return null; + } + + 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)) { + Preconditions.checkState(ref == predicate.getChild(1)); + predicate = new BinaryPredicate(predicate.getOp().converse(), ref, + predicate.getChild(0)); + predicate.analyzeNoThrow(analyzer); + } + + if (!(predicate.getChild(1) instanceof LiteralExpr)) return null; + return predicate; + } } http://git-wip-us.apache.org/repos/asf/incubator-impala/blob/2b5d1344/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 ce2892d..4df133a 100644 --- a/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test +++ b/testdata/workloads/functional-planner/queries/PlannerTest/kudu.test @@ -181,3 +181,14 @@ where t.c <= cast('1995-01-01 00:00:00' as timestamp) order by c predicates: CAST(o_orderdate AS TIMESTAMP) <= CAST('1995-01-01 00:00:00' AS TIMESTAMP) kudu predicates: o_orderkey < 10 ==== +# IMPALA-4213: Planner not pushing some predicates with constant exprs to Kudu +select count(*) from functional_kudu.alltypes +where id < 1475059765 + 10 +and 1475059765 + 100 < id +---- PLAN +01:AGGREGATE [FINALIZE] +| output: count(*) +| +00:SCAN KUDU [functional_kudu.alltypes] + kudu predicates: id < 1475059775, id > 1475059865 +====
