Inline only expressions for filters that contains only ininling targe… #10860

yingsu00 · 2018-06-18T22:58:46Z

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)

kokosing

Can you please add some tests for that, see TestPredicatePushdown.

martint · 2018-06-20T23:57:12Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java


            return rewrittenNode;
        }

+        private Sets.SetView<Symbol> extractNonInliningTargets(List<Expression> conjuncts, ProjectNode node)


Is there any specific reason for the return type to be Sets.SetView<Symbol>. If not, declare the return type as Set<Symbol> since the fact that it's a SetView is an irrelevant implementation detail.

I'm replacing the function to return boolean indicating whether an expression should be inlined. The previous implementation was based on InlineProjections and was not right for pushing down filters because it counts the dependencies across all expressions.

martint · 2018-07-18T21:42:50Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

+                    .collect(toSet());
+        }
+
+        private Map<Boolean, List<Expression>> partitionConjuncts(List<Expression> conjuncts, ProjectNode node)


Having a separate method for this doesn't buy much, especially since the name doesn't describe what it does. Inline it where its being used above.

martint · 2018-07-18T21:50:48Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

            }

            return rewrittenNode;
        }

+        private boolean isInliningExpression(Expression expression, ProjectNode node)


I'd call this isInliningCandidate

martint · 2018-07-18T22:00:24Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

+            // change the semantics of those expressions
+            Set<Symbol> tryArguments = extractTryArguments(expression).stream().collect(toSet());
+
+            long nonInliningSymbolsCount = dependencies.entrySet().stream()


No need to count. You can just do:

return dependencies.entrySet().stream() .noneMatch(...);

or, even better, invert the condition to make it easier to reason about and use allMatch:

return dependencies.entrySet().stream() .allMatch(entry -> (entry.getValue() == 1 || node.getAssignments().get(entry.getKey()) instanceof Literal) && !tryArguments.contains(entry.getKey()))

martint · 2018-07-18T22:04:45Z

presto-main/src/test/java/com/facebook/presto/sql/planner/TestPredicatePushdown.java

+    {
+        // Non-singletons should not be pushed down
+        assertPlan(
+                "with t as (select orderkey * 2 x from orders) select * from t where x + x > 1",


I think placing the with and the select in separate lines will improve readability:

assertPlan( "with t as (select orderkey * 2 x from orders) " + "select * from t where x + x > 1", anyTree(...));

martint · 2018-07-18T22:06:49Z

presto-main/src/test/java/com/facebook/presto/sql/planner/TestPredicatePushdown.java

+                                        tableScan("orders", ImmutableMap.of(
+                                                "orderkey", "orderkey"))))));
+
+        // composit singletons should be pushed down


typo: composite

kokosing · 2018-07-19T05:52:12Z

presto-main/src/test/java/com/facebook/presto/sql/planner/TestPredicatePushdown.java

+        // Non-singletons should not be pushed down
+        assertPlan(
+                "with t as (select orderkey * 2 x from orders) " +
+                        "select * from t where x + x > 1",


please use capital letters for SQL keywords, here would be SELECT * FROM t WHERE

kokosing · 2018-07-19T05:52:37Z

presto-main/src/test/java/com/facebook/presto/sql/planner/TestPredicatePushdown.java

@@ -267,4 +268,56 @@ public void testPredicatePushDownThroughMarkDistinct()
                                project(assignUniqueId("unique", filter("A = 1", values("A")))),
                                project(filter("1 = B", values("B"))))));
    }
+
+    @Test
+    public void testPredicatePushDownOverProjection()


I do not see a test with TRY

kokosing · 2018-07-19T06:04:16Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

-            // All non-deterministic conjuncts, if any, will be in the filter node.
+            // 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>> partitionedConjuncts = deterministicConjuncts.stream()


partitionedConjuncts sounds like it might having related to data partitioning. Maybe inlineConjuncts?

kokosing · 2018-07-19T06:06:30Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

+            //   2. references to complex expressions that
+            //      a. are not inputs to try() expressions
+            //      b. appear only once across all expressions
+            // which come from the node, as opposed to an enclosing scope.


please put this comment to javadoc of this method

There is no javadoc for any other method in the class. I'm not sure if I should do that?

Up to you. In my opinion this comment explains the method behaviour so it should at the top level of the method.

kokosing · 2018-07-19T06:07:16Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

            }

            return rewrittenNode;
        }

+        private boolean isInliningCandidate(Expression expression, ProjectNode node)
+        {
+            Set<Symbol> childOutputSet = ImmutableSet.copyOf(node.getOutputSymbols());


s/childOutputSet/input ?

kokosing · 2018-07-19T06:19:47Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

+            Set<Symbol> tryArguments = extractTryArguments(expression);
+
+            return dependencies.entrySet().stream()
+                    .allMatch(entry -> (entry.getValue() == 1 || node.getAssignments().get(entry.getKey()) instanceof Literal)


is not 1 too restrictive? Mentioned issue was about inlining when number of occurrences grew exponentially. Maybe we could have 10 here? Inlining expression 10 times should not affect planning but it should allow to handle more predicate pushdowns.

No, you get exponential behavior with anything > 1 depending on the depth of the plan. In the example shown in #10455 the number of occurrences is 3 for each expression.

True, thanks for explanation.

kokosing · 2018-07-19T06:24:58Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

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


this is if is not needed

kokosing · 2018-07-19T06:42:00Z

presto-main/src/main/java/com/facebook/presto/sql/planner/optimizations/PredicatePushDown.java

+
+            // exclude any complex inputs to TRY expressions. Inlining them would potentially
+            // change the semantics of those expressions
+            Set<Symbol> tryArguments = extractTryArguments(expression);


this looks like a separate issue

This is to fix github issue prestodb#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)

facebook-github-bot · 2018-07-25T20:40:11Z

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email [email protected] with your details so we can update your status.

yingsu00 requested a review from martint June 18, 2018 22:59

facebook-github-bot added the CLA Signed label Jun 18, 2018

yingsu00 force-pushed the InlineSymbols branch from ff7ef1c to 234ca94 Compare June 19, 2018 00:11

kokosing reviewed Jun 19, 2018

View reviewed changes

martint reviewed Jun 21, 2018

View reviewed changes

yingsu00 force-pushed the InlineSymbols branch from 234ca94 to 9a0847e Compare June 21, 2018 01:39

yingsu00 assigned martint Jun 22, 2018

martint suggested changes Jul 18, 2018

View reviewed changes

yingsu00 force-pushed the InlineSymbols branch 3 times, most recently from 41762bd to fbdec2b Compare July 19, 2018 01:04

martint approved these changes Jul 19, 2018

View reviewed changes

kokosing reviewed Jul 19, 2018

View reviewed changes

yingsu00 force-pushed the InlineSymbols branch from fbdec2b to 7f4d129 Compare July 25, 2018 00:32

yingsu00 force-pushed the InlineSymbols branch from 7f4d129 to bea413b Compare July 25, 2018 00:58

yingsu00 merged commit 6dcd67f into prestodb:master Aug 2, 2018

MichelleArk mentioned this pull request Mar 27, 2019

Predicate Pushdown of Partition Key Inconsistent with Specified Filter in SQL #12538

Closed

weiatwork mentioned this pull request Jul 1, 2021

Relax the restriction on predicate pushdown over projection trinodb/trino#8451

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inline only expressions for filters that contains only ininling targe… #10860

Inline only expressions for filters that contains only ininling targe… #10860

yingsu00 commented Jun 18, 2018

kokosing left a comment

martint Jun 20, 2018

yingsu00 Jun 21, 2018

martint Jul 18, 2018

martint Jul 18, 2018

martint Jul 18, 2018

yingsu00 Jul 18, 2018

martint Jul 18, 2018

martint Jul 18, 2018

kokosing Jul 19, 2018

kokosing Jul 19, 2018

kokosing Jul 19, 2018

yingsu00 Jul 20, 2018

kokosing Jul 19, 2018

yingsu00 Jul 20, 2018

kokosing Jul 23, 2018

kokosing Jul 19, 2018

kokosing Jul 19, 2018

martint Jul 19, 2018

kokosing Jul 20, 2018

kokosing Jul 19, 2018

kokosing Jul 19, 2018

facebook-github-bot commented Jul 25, 2018

Inline only expressions for filters that contains only ininling targe… #10860

Inline only expressions for filters that contains only ininling targe… #10860

Conversation

yingsu00 commented Jun 18, 2018

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 25, 2018