Transformations of Existential Subqueries using Early-out Joins#18381
Transformations of Existential Subqueries using Early-out Joins#18381rschlussel merged 2 commits intoprestodb:masterfrom
Conversation
1d7469f to
8db47f8
Compare
|
Can we start by having some benchmarks first? Maybe sqlebenchmark can be a good start to demonstrate the effectiveness of this optimization. |
presto-main/src/test/java/com/facebook/presto/sql/planner/TestEarlyOutJoins.java
Outdated
Show resolved
Hide resolved
504ecc0 to
2e4d267
Compare
I've added results from a tpcds benchmark run in the PR description. Let me know if you still think SQLBenchmarks are preferable |
2e4d267 to
630275c
Compare
|
Hi @aaneja , can you help take a look at this PR? |
630275c to
ef0d23e
Compare
|
@ClarenceThreepwood Vivek, thank you for sharing the TPC-DS results. It would be helpful to add % change column to the table. It looks like q95 is 2x faster, but the rest of the queries are about the same or slower (q33, q56, q60). Is this expected? Do you think the slowness of the 3 queries is just noise? |
aaneja
left a comment
There was a problem hiding this comment.
Looks like there a few other TPCDS queries that use semijoin, namely q14_1, q14_2, q70. Could you check using presto-benchto-benchmarks that these indeed did not have any plan changes and is this expected ?
...facebook/presto/sql/planner/iterative/rule/TransformDistinctInnerJoinToLeftEarlyOutJoin.java
Outdated
Show resolved
Hide resolved
...acebook/presto/sql/planner/iterative/rule/TransformDistinctInnerJoinToRightEarlyOutJoin.java
Outdated
Show resolved
Hide resolved
Good catch @mbasmanova. After more runs of the benchmarks and also running it at a higher scale factor (sf1000), I agree that the minor differences in performance are just noise (variance in S3 read latency also contributes to the noise). I've updated the description above with the latest runs and interpretations. |
|
@ClarenceThreepwood Thank you, Vivek. Would it make sense to update the % comment to show percentages as whole numbers. I don't think we care about fractional percents and having lots of numbers after period makes it a bit difficult to read. |
9348706 to
4227bf8
Compare
|
LGTM |
|
@prestodb/committers Could we get a review on this? |
|
@rschlussel - Would you mind giving this PR a once-over? |
5564398 to
86cb07e
Compare
I just meant the AbstractSqlBenchmark inheritors in general vs the standardized benchmarks. I've added a new suite - SqlEarlyOutJoinsBenchmarks - the results are as follows Summary: INFO: benchmarkTransformDistinctInnerJoinToLeftEarlyOutJoin INFO: With optimization INFO: benchmarkTransformDistinctInnerJoinToRightEarlyOutJoin INFO: With optimization INFO: benchmarkInPredicateToDistinctInnerJoin Raw Results Nov 01, 2022 1:55:44 PM com.facebook.airlift.log.Logger info |
Not quite sure I understand the question. The iterative optimizer in Presto is augmented with a framework that can infer and propagate logical properties of plans (unique keys, cardinality bounds, etc). These logical properties may be "seeded" with constraints that are defined in the catalog. There are also ways to indicate that pre-defined constraints are not to be relied on. Does that help? |
Oh, maybe an example can better demonstrate my question here. |
Looks like that you forgot to include the benchmark in your commit. |
86cb07e to
75c945a
Compare
My bad - fixed now |
Good question. However, I don't think that is possible. The LogicalProperties hang off of the Memo structure which is valid for the lifetime of the iterative optimizer. The logical properties are computed in a bottom-up manner for the groups in the memo. Any changes in the plan shape, no matter how it is achieved, that results in a "replace" in the memo, will trigger a recompute of the logical properties for that group/node. Therefore, a breaking change as you suggest, cannot sneak in. |
presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java
Outdated
Show resolved
Hide resolved
|
Thanks for the review @feilong-liu ! |
presto-main/src/main/java/com/facebook/presto/sql/analyzer/FeaturesConfig.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/SubqueryPlanner.java
Outdated
Show resolved
Hide resolved
e2be007 to
e195781
Compare
rschlussel
left a comment
There was a problem hiding this comment.
can you move the changes to LogicalProperties to a separate commit (can still be in this PR) and explain what's going on there?
...rc/main/java/com/facebook/presto/sql/planner/iterative/properties/LogicalPropertiesImpl.java
Outdated
Show resolved
Hide resolved
e195781 to
798718a
Compare
Done |
798718a to
98ce23a
Compare
rschlussel
left a comment
There was a problem hiding this comment.
Mostly looks great. A couple style comments, and also suggestions for testing.
...facebook/presto/sql/planner/iterative/rule/TransformDistinctInnerJoinToLeftEarlyOutJoin.java
Outdated
Show resolved
Hide resolved
...acebook/presto/sql/planner/iterative/rule/TransformDistinctInnerJoinToRightEarlyOutJoin.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/com/facebook/presto/sql/planner/plan/ApplyNode.java
Outdated
Show resolved
Hide resolved
...book/presto/sql/planner/iterative/rule/TestTransformDistinctInnerJoinToLeftEarlyOutJoin.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/TestEarlyOutJoins.java
Outdated
Show resolved
Hide resolved
…of intermediate plan expressions can be realized using another set Extend Rule.Context to provide a LogicalPropertiesProvider
98ce23a to
07b3d4a
Compare
Adds three optimizer rules: TransformUncorrelatedInPredicateSubqueryToDistinctInnerJoin - converts an in-predicate subquery into an inner join with distinct aggregation (logically equivalent to a semijoin) TransformDistinctInnerJoinToRightEarlyOutJoin - pushes aggregation into the left input of an inner join where applicable TransformDistinctInnerJoinToLeftEarlyOutJoin - converts an inner join with distinct aggregation into a semijoin Benchmarked on TPCDS(100G) data set. Queries impacted (9/100). Wallclock time performance improvement 10% for the impacted queries.
07b3d4a to
fbd49c8
Compare
|
Thanks @rschlussel ! |
Fixes #17927
Benchmark results on TPC-DS data set with scale factors sf100 and sf1000
Summary - 9/100 queries affected (plans changed) as a result of this feature.
Ignoring variance of less that 5%, we see that queries q58, q83, q95 are consistently improved
sf100 run with Benchto on AWS (r5.4xlarge, 4 workers + coordinator, 16vCPU, 128G RAM).
sf1000 run with Benchto on AWS (r5.4xlarge, 8 workers + coordinator, 128vCPU, 1024G RAM).