From 13bbff4e4e5fc5d459cc6f7a5512f84ceba897cd Mon Sep 17 00:00:00 2001 From: Steve Carlin Date: Wed, 1 Jun 2022 12:29:56 -0700 Subject: [PATCH] IMPALA-11323: Don't evaluate constants-only inferred predicates IMPALA-10182 fixed the problem of creating inferred predicates when both sides of an equality predicate came from the same slot. Inferred predicates also should not be created when both sides of an equality predicate are constant values which do not have scan slots. Change-Id: If1cd4559dda406d2d38703ed594b70b41ed336fd Reviewed-on: http://gerrit.cloudera.org:8080/18579 Tested-by: Impala Public Jenkins Reviewed-by: Aman Sinha --- .../java/org/apache/impala/analysis/Expr.java | 41 +++++++++++++++---- .../org/apache/impala/planner/PlanNode.java | 19 +++++++-- .../queries/QueryTest/inline-view.test | 16 +++++++- 3 files changed, 62 insertions(+), 14 deletions(-) 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 76175d51c..0bc470053 100644 --- a/fe/src/main/java/org/apache/impala/analysis/Expr.java +++ b/fe/src/main/java/org/apache/impala/analysis/Expr.java @@ -1641,6 +1641,30 @@ abstract public class Expr extends TreeNode implements ParseNode, Cloneabl return this; } + /** + * Returns the source expression for this expression. Traverses the source + * exprs of intermediate slot descriptors to resolve materialization points + * (e.g., aggregations). Returns null if there are multiple source Exprs + * mapped to the expression at any given point. + */ + public Expr findSrcExpr() { + // If the source expression is a constant expression, it won't have a scanSlotRef + // and we can return this. + if (isConstant()) { + return this; + } + SlotRef slotRef = unwrapSlotRef(false); + if (slotRef == null) return null; + SlotDescriptor slotDesc = slotRef.getDesc(); + if (slotDesc.isScanSlot()) return slotRef; + if (slotDesc.getSourceExprs().size() == 1) { + return slotDesc.getSourceExprs().get(0).findSrcExpr(); + } + // No known source expr, or there are several source exprs meaning the slot is + // has no single source table. + return null; + } + /** * Returns the descriptor of the scan slot that directly or indirectly produces * the values of 'this' SlotRef. Traverses the source exprs of intermediate slot @@ -1648,16 +1672,15 @@ abstract public class Expr extends TreeNode implements ParseNode, Cloneabl * Returns null if 'e' or any source expr of 'e' is not a SlotRef or cast SlotRef. */ public SlotDescriptor findSrcScanSlot() { - SlotRef slotRef = unwrapSlotRef(false); - if (slotRef == null) return null; - SlotDescriptor slotDesc = slotRef.getDesc(); - if (slotDesc.isScanSlot()) return slotDesc; - if (slotDesc.getSourceExprs().size() == 1) { - return slotDesc.getSourceExprs().get(0).findSrcScanSlot(); + Expr sourceExpr = findSrcExpr(); + if (sourceExpr == null) { + return null; } - // No known source expr, or there are several source exprs meaning the slot is - // has no single source table. - return null; + SlotRef slotRef = sourceExpr.unwrapSlotRef(false); + if (slotRef == null) { + return null; + } + return slotRef.getDesc(); } /** diff --git a/fe/src/main/java/org/apache/impala/planner/PlanNode.java b/fe/src/main/java/org/apache/impala/planner/PlanNode.java index b59acedc3..2dc21afe2 100644 --- a/fe/src/main/java/org/apache/impala/planner/PlanNode.java +++ b/fe/src/main/java/org/apache/impala/planner/PlanNode.java @@ -574,12 +574,23 @@ abstract public class PlanNode extends TreeNode { // Check if this is an inferred identity predicate i.e for c1 = c2 both // sides are pointing to the same source slot. In such cases it is wrong // to add the predicate to the SELECT node because it will incorrectly - // eliminate rows with NULL values. + // eliminate rows with NULL values. We also check if both sides are pointing + // to equal constant values. for (Expr e : conjuncts) { if (e instanceof BinaryPredicate && ((BinaryPredicate) e).isInferred()) { - SlotDescriptor lhs = ((BinaryPredicate) e).getChild(0).findSrcScanSlot(); - SlotDescriptor rhs = ((BinaryPredicate) e).getChild(1).findSrcScanSlot(); - if (lhs != null && rhs != null && lhs.equals(rhs)) continue; + Expr lhsSrcExpr = ((BinaryPredicate) e).getChild(0).findSrcExpr(); + Expr rhsSrcExpr = ((BinaryPredicate) e).getChild(1).findSrcExpr(); + if (lhsSrcExpr != null && rhsSrcExpr != null) { + if (lhsSrcExpr.isConstant() && rhsSrcExpr.isConstant() && + lhsSrcExpr.equals(rhsSrcExpr)) { + continue; + } + if (lhsSrcExpr instanceof SlotRef && rhsSrcExpr instanceof SlotRef) { + SlotRef lhsSlotRef = (SlotRef) lhsSrcExpr; + SlotRef rhsSlotRef = (SlotRef) rhsSrcExpr; + if (lhsSlotRef.getDesc().equals(rhsSlotRef.getDesc())) continue; + } + } } finalConjuncts.add(e); } diff --git a/testdata/workloads/functional-query/queries/QueryTest/inline-view.test b/testdata/workloads/functional-query/queries/QueryTest/inline-view.test index 2cbda7bba..5c0cc185d 100644 --- a/testdata/workloads/functional-query/queries/QueryTest/inline-view.test +++ b/testdata/workloads/functional-query/queries/QueryTest/inline-view.test @@ -578,4 +578,18 @@ NULL,NULL NULL,NULL ---- TYPES INT, INT -==== \ No newline at end of file +==== +---- QUERY +# IMPALA-11323: Constant expressions should get filtered out so they +# don't get placed in the select node in the planner. +with t as (select 1 a), v as + (select distinct a, cast(null as smallint) b, cast(null as smallint) c from t) +select distinct a,b,c from v + union all +select distinct a,b,c from v; +---- RESULTS +1,NULL,NULL +1,NULL,NULL +---- TYPES +TINYINT, SMALLINT, SMALLINT +====