perf: Convert inner joins to semi joins when equivalent#22652
perf: Convert inner joins to semi joins when equivalent#22652neilconway wants to merge 9 commits into
Conversation
|
run benchmarks tpcds |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-fd-convert-to-semi-join (39eb398) to 2e7b8e1 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
| /// The columns that are "live" at a plan node, i.e., which of its output | ||
| /// columns are referenced by an ancestor node. Represented as a set of column | ||
| /// indices, relative to the node's schema. | ||
| type LiveColumns = HashSet<usize>; |
There was a problem hiding this comment.
- This is similar but not identical to the
RequiredIndicesdata structure used byOptimizeProjections.RequiredIndicescares about insertion order but we don't, so it seemed cleaner to use a different data structure here. - It would be better to use a bitmap than a
HashSet. We could do so by adding a dependency on a third-party bitmap implementation (e.g., https://github.com/petgraph/fixedbitset, which one of our indirect dependencies already pulls in). But the performance impact should be small, so I'm not sure it's worth adding the dep.
|
run benchmarks |
|
run benchmark tpch10 |
|
Benchmark for this request failed. Last 20 lines of output: Click to expandFile an issue against this benchmark runner |
|
Benchmark for this request failed. Last 20 lines of output: Click to expandFile an issue against this benchmark runner |
|
Benchmark for this request failed. Last 20 lines of output: Click to expandFile an issue against this benchmark runner |
|
@neilconway FYI quite some plans changed now 🚀 Can you take a look at the PR |
|
run benchmarks |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-fd-convert-to-semi-join (6b1d38a) to 7843ab3 (merge-base) diff using: clickbench_partitioned File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-fd-convert-to-semi-join (6b1d38a) to 7843ab3 (merge-base) diff using: tpcds File an issue against this benchmark runner |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-fd-convert-to-semi-join (6b1d38a) to 7843ab3 (merge-base) diff using: tpch File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch — base (merge-base)
tpch — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpcds — base (merge-base)
tpcds — branch
File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usageclickbench_partitioned — base (merge-base)
clickbench_partitioned — branch
File an issue against this benchmark runner |
|
@Dandandan I updated the expected plans; everything that changed seemed inline with the intended goal of this PR. Looking at the latest benchmark numbers, some of the TPC-DS regressions. The plan was q54 still seems to have regressed; I suspect there's another planner/stats bug here, taking a look now. |
|
run benchmark tpch10 |
|
🤖 Benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-fd-convert-to-semi-join (6b1d38a) to 7843ab3 (merge-base) diff using: tpch10 File an issue against this benchmark runner |
|
🤖 Benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagetpch10 — base (merge-base)
tpch10 — branch
File an issue against this benchmark runner |
|
The plans are looking better, although I think we probably need to have some more specialization (semi join? / single key column? etc.) to benefit from it more? |
Which issue does this PR close?
Rationale for this change
This PR extends the
EliminateJoinrewrite pass to replace inner joins with semi joins in some cases. An inner joinL ⋈ Rcan be rewritten to a left semi joinL ⋉ Rif two conditions hold:(And symmetrically with right semi joins.)
What changes are included in this PR?
for_each_referenced_indexhelper that is used by bothEliminateJoinandEliminateProjectionsLiveColumnstype to track the live (referenced by parent) columns of a plan nodeEliminateJoinAre these changes tested?
Yes; new tests added.
Are there any user-facing changes?
Some plan changes but no behavioral changes.