Pushdown dereference expression in query plan by qqibrow · Pull Request #1435 · trinodb/trino

qqibrow · 2019-09-03T17:05:07Z

it aims to do:

Pushdown dereferences down above tableScan, which can be used by pushdownProjectionToTableScan
Reduce shuffling cost:
1. Dereference before exchange can avoid shuffle whoe struct in exchange.
2. Dereference exact once can avoid shuffling the same subField twice in exchange. For example,
  query:
```
explain WITH t(msg) AS (SELECT * FROM (VALUES ROW(CAST(ROW(1, 2.0) AS ROW(x BIGINT, y 
DOUBLE))))) 
SELECT b.msg.x FROM t a, t b WHERE a.msg.x = b.msg.x
```
One side of join outputs [field_6:row(x bigint, y double), expr_19:bigint] but in fact field_6 contains expr_19.

qqibrow · 2019-09-03T18:58:22Z

PRODUCT_TESTS_SUITE=suite-4 failed but I didn't find which test case it failed. https://travis-ci.com/prestosql/presto/jobs/230815571

findepi · 2019-09-03T19:00:01Z

PRODUCT_TESTS_SUITE=suite-4 failed

@qqibrow it's unfortunately flaky. Retriggered.

martint

Some initial comments. I'm looking at the PushDownDereferences class now.

martint · 2019-09-09T19:56:46Z

presto-main/src/main/java/io/prestosql/sql/planner/PlanOptimizers.java

Why is this placed here? Ideally, it should be closer to (or part of) the IterativeOptimizer that calls all the other push/prune/merge/etc rules (around line 300 in this file).

It should be at least below TransformUncorrelatedInPredicateSubqueryToSemiJoin, where SimiJoinNode is first generated.

...-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PushDownDereferences.java

martint · 2019-09-16T22:03:50Z

...-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PushDownDereferences.java

We need to make sure it won't "undo" decisions made by InlineProjections. Otherwise, it's possible to end up with an infinite optimization loop. It may be necessary to adjust the heuristics in InlineProjections.

martint · 2019-09-16T22:04:59Z

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

Place closing ) in previous line.

martint · 2019-09-16T22:05:38Z

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

Capitalize SQL keywords (FROM, CROSS JOIN, etc)

martint · 2019-09-16T22:21:57Z

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

The intermediate SELECT * is unnecessary. Also, please format the query like this for better readability:

WITH t(msg) AS (VALUES ROW(CAST(ROW(1, 2.0) AS ROW(x BIGINT, y DOUBLE)))) SELECT b.msg.x FROM t a, t b WHERE a.msg.y = b.msg.y

Similar comment applies to all the queries below.

martint · 2019-09-16T22:24:10Z

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

Implement this test or remove it.

martint · 2019-09-16T22:26:08Z

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

It's good to have these tests, but it would be ideal to also add rule-specific tests. See io.prestosql.sql.planner.iterative.rule.TestXXX for some examples.

martint · 2019-09-16T22:48:22Z

...-main/src/main/java/com/facebook/presto/sql/planner/iterative/rule/PushDownDereferences.java

Relying inheritance to reuse behavior makes the code harder to follow and reason about (is it just about code reuse? Is there shared state that's important? etc). It'd be better to model it via some form of delegation. E.g., The rule can take another object that performs the job of pushDownDereferences.

phd3 · 2019-10-08T21:13:00Z

I can also help with the review of this one, once it's updated with @martint 's comments.

qqibrow · 2019-10-13T16:50:20Z

@phd3 sure. thanks. Working on this.

Co-authored-by: Zhenxiao Luo <luoz@uber.com>

qqibrow · 2019-11-04T22:36:01Z

@martint @findepi @phd3 Addressed all comments. Please take a look. thanks

phd3

@qqibrow Thanks for your work on this. I've added some comments.

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/InlineProjections.java

phd3 · 2019-11-06T00:50:39Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushDownDereferences.java

+        // When nested child and parent dereferences both exist, Pushdown rule will be trigger one more time
+        // and lead to runtime error. E.g. [msg.foo, msg.foo.bar] => [exp, exp.bar] (should stop here but
+        // since there are still dereferences, pushdown rule will trigger again)
+        if (dereferences.stream().anyMatch(exp -> baseExists(exp, dereferences))) {


if input variable expressions is [x.y.z + f(x.y) + a.b] then this code will not pushdown a.b, right?

I think there is an alternative approach to doing this.

Given the expressions, we only create required symbols. i.e. for [x.y.z + f(x.y) + a.b], we can create symbols for x.y and a.b.

We can return ImmutableMap.of() in cases where all bases of all dereference expressions are symbolreferences. In the example you provided, the optimizer will stop the second time around there since the base exp is a symbolreference.

Yes. it will not pushdown a.b. I don't know how to stop iteration in this case so this is just a walkaround, not final solution.

We can return ImmutableMap.of() in cases where all bases of all dereference expressions are symbolreferences

How does this work? It will stop the first time for [f(x.y) + a.b] because x and a are both symbol references.

For example, let's say the variable expressions is a single element list [x.y.z + f(x.y) + a.b] . dereferences would come out to be {x.y.z, x.y, a.b}. Now here's where we can change the implementation. We can return ImmutableMap.of() if ALL elements in this set of expressions dereferences are 1-level dereferences. i.e. the bases are symbolreferences. It doesn't happen the first time, since x.y (the base of x.y.z) is not a symbol reference. In this new implementation, we also need to make sure that we only create symbol references for "superset" references. i.e. in this example, we only create symbols for x.y and a.b.

I think optimizing such pushdowns here will impact the effectiveness of all individual pushdown rules, since this method is being used everywhere.

phd3 · 2019-11-06T01:10:25Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushDownDereferences.java

+    private static boolean validPushDown(DereferenceExpression dereference)
+    {
+        Expression base = dereference.getBase();
+        return (base instanceof SymbolReference) || (base instanceof DereferenceExpression);


Can you elaborate on why in other cases the pushdown should not be considered valid?

like CAST(ROW(1, 2.0) AS ROW(x BIGINT, y DOUBLE)).x

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushDownDereferences.java

phd3 · 2019-11-06T05:54:00Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushDownDereferences.java

+        }
+    }
+
+    static class PushDownDereferenceThroughUnnest


should we also implement ExtractFromUnnest rule since this can have an optional predicate based on recent changes?

It depends on when we run PushdownDereferences I guess. If it runs before filter is empty then that's fine.

phd3 · 2019-11-11T22:48:45Z

I've a concern about the placement of PushDownDereferences rule in the list of optimizers. If we want to be able to pushdown predicates+projections as in #1953, we need to make sure that all dereference projections are right above table scan. If there is a filter, that should be above the dereference projections.

For a query like the following, current PR produces a plan that has FilterNode in between ProjectNode and ValuesNode at the end of all optimizations.

WITH t(msg) AS (VALUES ROW(CAST(ROW(1, 2.0) AS ROW(x BIGINT, y DOUBLE)))) SELECT msg.x FROM t where msg.y = 5

This happens because PredicatePushDown rule undoes the dereference projection pushdown done by this rule. Is this expected or we should be protecting the dereference projections against a filter pushdown?

If we go with the current placement of PushdownDereferences, we need to invoke the connector pushdown rules right after this rule is invoked, to capture the connector pushdown before the plan is mutated by PredicatePushdown (or any other optimizers later). What do you think @martint @qqibrow ?

martint · 2019-11-13T07:30:33Z

Is this expected or we should be protecting the dereference projections against a filter pushdown?

Yes. We need to be careful about making sure rules don't undo each other's work, as this will lead to an infinite loop once the rules are placed in the same IterativeOptimizer. In particular, we'll need to adjust PredicatePushdown to not push filters below a dereference-expression-only projection. Ideally, we should migrate Predicate pushdown to a Rule-based implementation instead of a Visitor, but that's an orthogonal issue. We should also make sure InlineProjections doesn't undo a pushdown of dereference expressions into a separate projection.

qqibrow · 2019-11-21T04:44:54Z

This happens because PredicatePushDown rule undoes the dereference projection pushdown done by this rule.

Your optimizer PushdownDereferenceToTableScan needs to run right after PushdownDereferences. You can refer the ordering to my previous diff

InlineProjections doesn't undo a pushdown of dereference expressions into a separate projection.

That's done in current work. Please review.

I am concerned that current PushdownDereferences doesn't cover all PlanNode. So there might be cases that this doesn't work.

phd3

I think it will help to add tests that cover more complex expressions and multiple level of dereferences.

phd3 · 2019-11-25T05:42:16Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushDownDereferences.java

+        // When nested child and parent dereferences both exist, Pushdown rule will be trigger one more time
+        // and lead to runtime error. E.g. [msg.foo, msg.foo.bar] => [exp, exp.bar] (should stop here but
+        // since there are still dereferences, pushdown rule will trigger again)
+        if (dereferences.stream().anyMatch(exp -> baseExists(exp, dereferences))) {


For example, let's say the variable expressions is a single element list [x.y.z + f(x.y) + a.b] . dereferences would come out to be {x.y.z, x.y, a.b}. Now here's where we can change the implementation. We can return ImmutableMap.of() if ALL elements in this set of expressions dereferences are 1-level dereferences. i.e. the bases are symbolreferences. It doesn't happen the first time, since x.y (the base of x.y.z) is not a symbol reference. In this new implementation, we also need to make sure that we only create symbol references for "superset" references. i.e. in this example, we only create symbols for x.y and a.b.

phd3 · 2019-11-25T06:20:14Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushDownDereferences.java

+                    leftNode,
+                    rightNode,
+                    joinNode.getCriteria(),
+                    ImmutableList.<Symbol>builder().addAll(leftNode.getOutputSymbols()).addAll(rightNode.getOutputSymbols()).build(),


This will add symbols being referenced in filters and equijoin clauses too as output symbols. (reverse of pruning). I think we should just add joinNode.getOutputSymbols() and pushdownDereferences.values() here.

phd3 · 2019-11-25T19:01:18Z

presto-main/src/main/java/io/prestosql/sql/planner/iterative/rule/PushDownDereferences.java

+        // When nested child and parent dereferences both exist, Pushdown rule will be trigger one more time
+        // and lead to runtime error. E.g. [msg.foo, msg.foo.bar] => [exp, exp.bar] (should stop here but
+        // since there are still dereferences, pushdown rule will trigger again)
+        if (dereferences.stream().anyMatch(exp -> baseExists(exp, dereferences))) {


I think optimizing such pushdowns here will impact the effectiveness of all individual pushdown rules, since this method is being used everywhere.

phd3 · 2019-11-27T17:56:02Z

There is an interesting case of dereference expressions buried inside lambda expressions, which I believe we're missing here. For example, the following test case:

WITH t(msg) AS (VALUES ROW(CAST(ROW(1, 2.0) AS ROW(x BIGINT, y DOUBLE))))" +
                        "SELECT try(1.0/msg.x) FROM t where msg.y = 5

The try(...) function is converted to a lambda expression. so msg.x is not pushed down. I believe this is thew case with explicit lambdas and any functions that convert to lambdas. The reasons seems to be DefaultExpressionTraversalVisitor not taking into account lambda expressions, from which the visitor in extractDereferenceExpressions is extended.

phd3 · 2020-01-29T22:08:11Z

superceded by #2672

cla-bot bot added the cla-signed label Sep 3, 2019

qqibrow requested a review from findepi September 4, 2019 15:30

findepi requested review from martint and removed request for findepi September 5, 2019 07:12

martint mentioned this pull request Sep 9, 2019

Prune nested fields #812

Closed

dain assigned martint Sep 9, 2019

martint reviewed Sep 16, 2019

View reviewed changes

qqibrow force-pushed the pushdown_rebase branch 4 times, most recently from a4f0734 to f921a6e Compare November 4, 2019 05:20

Pushdown dereference expression

a58f08e

Co-authored-by: Zhenxiao Luo <luoz@uber.com>

qqibrow force-pushed the pushdown_rebase branch from f921a6e to a58f08e Compare November 4, 2019 16:14

phd3 mentioned this pull request Nov 4, 2019

Pushdown of dereference expressions (with focus on Hive connector) #1953

Closed

10 tasks

phd3 reviewed Nov 6, 2019

View reviewed changes

martint self-requested a review November 14, 2019 10:50

address code review comments

5c38617

phd3 reviewed Nov 25, 2019

View reviewed changes

phd3 mentioned this pull request Dec 21, 2019

Benchmark for Hive Projection Pushdown #2307

Merged

martint assigned phd3 Jan 22, 2020

martint removed their assignment Jan 22, 2020

phd3 mentioned this pull request Jan 29, 2020

Dereference Projection Pushdown in Query Plan #2672

Merged

martint closed this Feb 11, 2020

Conversation

qqibrow commented Sep 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qqibrow commented Sep 3, 2019

Uh oh!

findepi commented Sep 3, 2019

Uh oh!

martint left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phd3 commented Oct 8, 2019

Uh oh!

qqibrow commented Oct 13, 2019

Uh oh!

qqibrow commented Nov 4, 2019

Uh oh!

phd3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qqibrow Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phd3 commented Nov 11, 2019

Uh oh!

martint commented Nov 13, 2019

Uh oh!

qqibrow commented Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phd3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phd3 commented Nov 27, 2019

Uh oh!

phd3 commented Jan 29, 2020

Uh oh!

qqibrow commented Sep 3, 2019 •

edited

Loading

qqibrow Nov 21, 2019 •

edited

Loading

qqibrow commented Nov 21, 2019 •

edited

Loading