perf: Convert inner joins to semi joins when equivalent#22652
Open
neilconway wants to merge 9 commits into
Open
perf: Convert inner joins to semi joins when equivalent#22652neilconway wants to merge 9 commits into
neilconway wants to merge 9 commits into
Conversation
Contributor
Author
|
run benchmarks tpcds |
github-actions
Bot
added
optimizer
|
🤖 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 |
neilconway
commented
May 31, 2026
Comment on lines
+80
to
+83
| /// 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>; |
Contributor
Author
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.
Contributor
There was a problem hiding this comment.
You could potentially use arrow BooleanBufferBuilder as a mutable bitset: https://docs.rs/arrow/latest/arrow/array/struct.BooleanBufferBuilder.html
Not sure if that is actually faster or not
Contributor
Author
Contributor
|
run benchmarks |
Contributor
|
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 |
Contributor
|
@neilconway FYI quite some plans changed now 🚀 Can you take a look at the PR |
Contributor
Author
|
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 |
Contributor
Author
|
@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. |
Contributor
|
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 |
Contributor
|
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? |
Contributor
|
As I understand it, the idea of this optimization is to use semi-join which in theory should be more efficient than a normal INNER JOIN. However, it seems the DataFusion semi-join doesn't actually have a noticably faster implementation? If so it seems reasonable to me to merge this PR and maybe file a ticket to improve SEMI join performance |
Contributor
Author
|
@alamb There are two scenarios where this should be a win:
(Symmetric for right semi joins as well, of course.) I can't see a scenario where this PR would regress performance. There were some semi-join planner/stats bugs that resulted in picking worse plans; #22762 fixes the last one that I know about. Once that lands I'll rerun the benchmarks for this PR. |
Contributor
I think in this situation there is only limited difference currently, it will use most of the same code paths. There is probably some further specialization we can do for different join types. |
Contributor
|
run benchmark sql_planner |
|
🤖 Criterion benchmark running (GKE) | trigger CPU Details (lscpu)Comparing neilc/perf-fd-convert-to-semi-join (6b1d38a) to 7843ab3 (merge-base) diff File an issue against this benchmark runner |
alamb
approved these changes
Jun 5, 2026
Contributor
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
I went through this one carefully, and reviewed the plan changes and code coverage. Looks like a good improvement to me. Thank you @neilconway and @Dandandan
I think there is one good other than a few more tests
Thanks again
|
|
||
| //! [`EliminateJoin`] rewrites `INNER JOIN` with `true`/`null` | ||
| use crate::optimizer::ApplyOrder; | ||
| //! [`EliminateJoin`] rewrites inner joins to simpler forms to make them cheaper |
Contributor
There was a problem hiding this comment.
I always enjoy reading PRs where you learn something from the comments ❤️ 🤓
|
|
||
| /// 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. |
Contributor
There was a problem hiding this comment.
it might be useful to refer back to the module level comments that explain this structure in more detail
| //! only L's columns. The rewrite to `L ⋉ R` is valid when both of the | ||
| //! following are true: | ||
| //! | ||
| //! 1. None of R's columns are referenced above the join. |
Comment on lines
+80
to
+83
| /// 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>; |
Contributor
There was a problem hiding this comment.
You could potentially use arrow BooleanBufferBuilder as a mutable bitset: https://docs.rs/arrow/latest/arrow/array/struct.BooleanBufferBuilder.html
Not sure if that is actually faster or not
| live: LiveColumns, | ||
| duplicate_insensitive: bool, | ||
| ) -> Result<Transformed<LogicalPlan>> { | ||
| match plan { |
Contributor
There was a problem hiding this comment.
I think this code depends on being able to walk all expressions used in the plan to remain correct. I worry if we ever modify a LogicalPlan node to add a new Expr container -- this code would continue to compile, and we would probably end up with subtle bugs
SOme ideas on how to address:
- Use this API for visiting expressions/finding live columns: https://docs.rs/datafusion/latest/datafusion/logical_expr/enum.LogicalPlan.html#method.apply_expressions
- name all fields (remove
..) so that if any field is added, the PR must also explicitly change this file too
| (left, right) | ||
| } | ||
|
|
||
| fn side_unique_on_join<'a>( |
Contributor
There was a problem hiding this comment.
Woudl this be better / easier to discover / reuse as a method on Join?
| } | ||
|
|
||
| /// The columns of `schema` referenced by any of `exprs`. | ||
| fn live_columns<'a>( |
Contributor
There was a problem hiding this comment.
Could be LoveColumns::try_new() potentially, rather than a free function
|
|
||
| /// Inserts into `live` the index, within `schema`, of every column referenced | ||
| /// by any of `exprs`. | ||
| fn extend_live_columns<'a>( |
Contributor
There was a problem hiding this comment.
Another good candidate for a LIveColumns method perhaps
| 01)Sort: supplier.s_acctbal DESC NULLS FIRST, nation.n_name ASC NULLS LAST, supplier.s_name ASC NULLS LAST, part.p_partkey ASC NULLS LAST, fetch=10 | ||
| 02)--Projection: supplier.s_acctbal, supplier.s_name, nation.n_name, part.p_partkey, part.p_mfgr, supplier.s_address, supplier.s_phone, supplier.s_comment | ||
| 03)----Inner Join: part.p_partkey = __scalar_sq_1.ps_partkey, partsupp.ps_supplycost = __scalar_sq_1.min(partsupp.ps_supplycost) | ||
| 03)----LeftSemi Join: part.p_partkey = __scalar_sq_1.ps_partkey, partsupp.ps_supplycost = __scalar_sq_1.min(partsupp.ps_supplycost) |
Contributor
There was a problem hiding this comment.
for anyone else reviewing this, TPCH has FK:PK joins on its schema, so this transformation can potentially help here
| }, | ||
| ) | ||
| } | ||
| LogicalPlan::Distinct(Distinct::All(input)) => { |
Contributor
There was a problem hiding this comment.
I think some of this code is not covered by tests
cargo llvm-cov --html test --profile=ci -p datafusion-optimizer
cargo llvm-cov --html test --profile=ci --test sqllogictests
|
🤖 Criterion benchmark completed (GKE) | trigger Instance: CPU Details (lscpu)Details
Resource Usagebase (merge-base)
branch
File an issue against this benchmark runner |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.