Skip to content

Commit

Permalink
Inline only simple expressions in predicate pushdown
Browse files Browse the repository at this point in the history 
This is to fix github issue #10455.

We used to inline all predicate when doing push down predicates. This could have problems when the projection columns contains complex expressions like the following:

WITH
t1 (v) AS (VALUES 1),
t2 AS( select if(v = 0, v, v) v from t1 ),
t3 AS( select if(v = 0, v, v) v from t2 ),
t4 AS( select if(v = 0, v, v) v from t3 ),
t5 AS( select if(v = 0, v, v) v from t4 ),
t6 AS( select if(v = 0, v, v) v from t5 ),
t7 AS( select if(v = 0, v, v) v from t6 ),
t8 AS( select if(v = 0, v, v) v from t7 ),
t9 AS( select if(v = 0, v, v) v from t8 ),
t10 AS( select if(v = 0, v, v) v from t9 ),
t11 AS( select if(v = 0, v, v) v from t10 ),
t12 AS( select if(v = 0, v, v) v from t11 ),
t13 AS( select if(v = 0, v, v) v from t12 ),
t14 AS( select if(v = 0, v, v) v from t13 ),
t15 AS( select if(v = 0, v, v) v from t14 ),
t16 AS( select if(v = 0, v, v) v from t15 )
select *
from t16
where v = 0

This short-term fix is to adjust the inlining heuristics to only do it if the expressions are trivial or appear only once (similar to how the InlineProjections rule works)
  • Loading branch information
@yingsu00
Ying Su authored and yingsu00 committed Aug 2, 2018
1 parent 5315bc5 commit 6dcd67f
Show file tree
Hide file tree
Showing 2 changed files with 109 additions and 5 deletions.
51 changes: 46 additions & 5 deletions ...o-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,14 @@
import com.facebook.presto.sql.tree.BooleanLiteral;
import com.facebook.presto.sql.tree.ComparisonExpression;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.Literal;
import com.facebook.presto.sql.tree.LongLiteral;
import com.facebook.presto.sql.tree.NodeRef;
import com.facebook.presto.sql.tree.NullLiteral;
import com.facebook.presto.sql.tree.SymbolReference;
import com.facebook.presto.sql.tree.TryExpression;
import com.facebook.presto.sql.util.AstUtils;
import com.google.common.base.Verify;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
Expand All @@ -65,6 +69,7 @@
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
import java.util.function.Predicate;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -212,17 +217,53 @@ public PlanNode visitProject(ProjectNode node, RewriteContext<Expression> contex

Map<Boolean, List<Expression>> conjuncts = extractConjuncts(context.get()).stream().collect(Collectors.partitioningBy(deterministic));

// Push down conjuncts from the inherited predicate that don't depend on non-deterministic assignments
PlanNode rewrittenNode = context.defaultRewrite(node, inlineSymbols(node.getAssignments().getMap(), combineConjuncts(conjuncts.get(true))));
// Push down conjuncts from the inherited predicate that only depend on deterministic assignments with
// certain limitations.
List<Expression> deterministicConjuncts = conjuncts.get(true);

// All non-deterministic conjuncts, if any, will be in the filter node.
if (!conjuncts.get(false).isEmpty()) {
rewrittenNode = new FilterNode(idAllocator.getNextId(), rewrittenNode, combineConjuncts(conjuncts.get(false)));
// We partition the expressions in the deterministicConjuncts into two lists, and only inline the
// expressions that are in the inlining targets list.
Map<Boolean, List<Expression>> inlineConjuncts = deterministicConjuncts.stream()
.collect(Collectors.partitioningBy(expression -> isInliningCandidate(expression, node)));

List<Expression> inlinedDeterministicConjuncts = inlineConjuncts.get(true).stream()
.map(entry -> inlineSymbols(node.getAssignments().getMap(), entry))
.collect(Collectors.toList());

PlanNode rewrittenNode = context.defaultRewrite(node, combineConjuncts(inlinedDeterministicConjuncts));

// All deterministic conjuncts that contains non-inlining targets, and non-deterministic conjuncts,
// if any, will be in the filter node.
List<Expression> nonInliningConjuncts = inlineConjuncts.get(false);
nonInliningConjuncts.addAll(conjuncts.get(false));

if (!nonInliningConjuncts.isEmpty()) {
rewrittenNode = new FilterNode(idAllocator.getNextId(), rewrittenNode, combineConjuncts(nonInliningConjuncts));
}

return rewrittenNode;
}

private boolean isInliningCandidate(Expression expression, ProjectNode node)
{
// TryExpressions should not be pushed down. However they are now being handled as lambda
// passed to a FunctionCall now and should not affect predicate push down. So we want to make
// sure the conjuncts are not TryExpressions.
Verify.verify(AstUtils.preOrder(expression).noneMatch(TryExpression.class::isInstance));

// candidate symbols for inlining are
// 1. references to simple constants
// 2. references to complex expressions that appear only once
// which come from the node, as opposed to an enclosing scope.
Set<Symbol> childOutputSet = ImmutableSet.copyOf(node.getOutputSymbols());
Map<Symbol, Long> dependencies = SymbolsExtractor.extractAll(expression).stream()
.filter(childOutputSet::contains)
.collect(Collectors.groupingBy(Function.identity(), Collectors.counting()));

return dependencies.entrySet().stream()
.allMatch(entry -> entry.getValue() == 1 || node.getAssignments().get(entry.getKey()) instanceof Literal);
}

@Override
public PlanNode visitGroupId(GroupIdNode node, RewriteContext<Expression> context)
{
Expand Down
63 changes: 63 additions & 0 deletions presto-main/src/test/java/com/facebook/presto/sql/planner/TestPredicatePushdown.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.assignUniqueId;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.equiJoinClause;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.exchange;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.expression;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.filter;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.join;
import static com.facebook.presto.sql.planner.assertions.PlanMatchPattern.node;
Expand Down Expand Up @@ -267,4 +268,66 @@ public void testPredicatePushDownThroughMarkDistinct()
project(assignUniqueId("unique", filter("A = 1", values("A")))),
project(filter("1 = B", values("B"))))));
}

@Test
public void testPredicatePushDownOverProjection()
{
// Non-singletons should not be pushed down
assertPlan(
"WITH t AS (SELECT orderkey * 2 x FROM orders) " +
"SELECT * FROM t WHERE x + x > 1",
anyTree(
filter("((expr + expr) > BIGINT '1')",
project(ImmutableMap.of("expr", expression("orderkey * BIGINT '2'")),
tableScan("orders", ImmutableMap.of("ORDERKEY", "orderkey"))))));

// constant non-singleton should be pushed down
assertPlan(
"with t AS (SELECT orderkey * 2 x, 1 y FROM orders) " +
"SELECT * FROM t WHERE x + y + y >1",
anyTree(
project(
filter("(((orderkey * BIGINT '2') + BIGINT '1') + BIGINT '1') > BIGINT '1'",
tableScan("orders", ImmutableMap.of(
"orderkey", "orderkey"))))));

// singletons should be pushed down
assertPlan(
"WITH t AS (SELECT orderkey * 2 x FROM orders) " +
"SELECT * FROM t WHERE x > 1",
anyTree(
project(
filter("(orderkey * BIGINT '2') > BIGINT '1'",
tableScan("orders", ImmutableMap.of(
"orderkey", "orderkey"))))));

// composite singletons should be pushed down
assertPlan(
"with t AS (SELECT orderkey * 2 x, orderkey y FROM orders) " +
"SELECT * FROM t WHERE x + y > 1",
anyTree(
project(
filter("((orderkey * BIGINT '2') + orderkey) > BIGINT '1'",
tableScan("orders", ImmutableMap.of(
"orderkey", "orderkey"))))));

// Identities should be pushed down
assertPlan(
"WITH t AS (SELECT orderkey x FROM orders) " +
"SELECT * FROM t WHERE x >1",
anyTree(
filter("orderkey > BIGINT '1'",
tableScan("orders", ImmutableMap.of(
"orderkey", "orderkey")))));

// Non-deterministic predicate should not be pushed down
assertPlan(
"WITH t AS (SELECT rand() * orderkey x FROM orders) " +
"SELECT * FROM t WHERE x > 5000",
anyTree(
filter("expr > 5E3",
project(ImmutableMap.of("expr", expression("rand() * CAST(orderkey AS double)")),
tableScan("orders", ImmutableMap.of(
"ORDERKEY", "orderkey"))))));
}
}

0 comments on commit 6dcd67f

Please sign in to comment.