You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by jb...@apache.org on 2016/10/06 15:24:01 UTC
[2/2] incubator-impala git commit: IMPALA-4213: Fix Kudu predicates
that need constant folding
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 <mj...@cloudera.com>
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 <mj...@cloudera.com>
Authored: Mon Oct 3 17:00:48 2016 -0700
Committer: Internal Jenkins <cl...@gerrit.cloudera.org>
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
+====