Repository navigation
Conversation
After flattening a chain of inner/cross joins and reassembling it, `EliminateCrossJoin` called `rewrite_children` on the reassembled tree. Its left child is the reassembled join chain minus one input, which the rule flattened and rebuilt again, and so on at every nesting level, making the rule quadratic in the number of joined inputs (with each rebuild also searching the accumulated left schema). Instead, rewrite each flattened input once before reassembling the joins. `rewrite` preserves the schema of its input, which is all that `find_inner_join` looks at, so the result is unchanged. Uncorrelated subqueries in the join keys, previously visited by `rewrite_children`, are now rewritten explicitly. Also check the right schema first in `find_valid_equijoin_key_pair`: the left schema accumulates all previous inputs and is much more expensive to search, while most candidate keys do not reference the right input. Add an `eliminate_cross_join` benchmark.
xudong963
marked this pull request as ready for review
October 27, 2026 04:13
The previous commit made `find_valid_equijoin_key_pair` always check the right schema first, which assumes the left-deep join trees built by `EliminateCrossJoin`. Other callers, such as `ExtractEquijoinPredicate` and `LogicalPlanBuilder` joins, may see a narrow left input and a wide right one (e.g. `t JOIN (t1 JOIN t2 ...)`), which that order made slower. Check the narrower schema first instead. Looking up a column scans the schema, so a key that does not belong to the join inputs is then usually rejected without scanning the wider one. `EliminateCrossJoin` keeps its speedup, as it joins the accumulated left input with narrower inputs. Add a `find_valid_equijoin_key_pair` benchmark covering both a wide left and a wide right input, and a unit test covering both orders.
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
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
EliminateCrossJoinis quadratic (or worse) in the number of joined inputs, both for comma joins (FROM t0, t1, ..., tn WHERE t0.a = t1.a AND ...) and for explicit inner join chains (t0 JOIN t1 ON ... JOIN t2 ON ...). With 128 tables the rule alone takes ~0.6-0.7 s.After flattening the join tree and reassembling it, the rule calls
rewrite_childrenon the reassembled tree. The left child is the reassembled chain minus one input, so the rule flattens and rebuilds it again, and so on at every nesting level. Each rebuild also checks candidate join keys against the accumulated left schema, which grows with the number of inputs.What changes are included in this PR?
EliminateCrossJoin::rewrite: rewrite each flattened input once, before reassembling the joins, instead of callingrewrite_childrenon the reassembled tree.rewritepreserves the schema of its input, which is all thatfind_inner_joinlooks at, so the result is unchanged. Uncorrelated subqueries in the join keys of each reassembled join (previously visited byrewrite_childrenviamap_uncorrelated_subqueries) are now rewritten explicitly.find_valid_equijoin_key_pair: check the narrower of the two schemas first. Looking up a column scans the schema, so a candidate key that does not belong to the join inputs is then usually rejected without scanning the wider one. ForEliminateCrossJoin, the wider schema is the accumulated left input of the left-deep join tree, while most candidate keys don't reference the (narrower) right input at all. Other callers (ExtractEquijoinPredicate,LogicalPlanBuilderjoins) may instead see a narrow left and a wide right input, e.g.t JOIN (t1 JOIN t2 ...), which is why the order depends on the schema widths rather than always checking the right schema first.check_all_columns_from_schemais side-effect free and never returns an error, so reordering the checks does not change the result.eliminate_cross_joincriterion benchmark (comma joins and inner join chains over 8-128 tables), and afind_valid_equijoin_key_pairbenchmark covering a wide left and a wide right input.Benchmark results (Apple M-series laptop,
main@ c18e5d9 vs this PR):cargo bench -p datafusion-optimizer --bench eliminate_cross_joincargo bench -p datafusion-expr --bench find_valid_equijoin_key_pair(a key that belongs to neither input, against a 3-column schema and a schema of the given width; "right first" is the order used by an earlier revision of this PR):What is the testing strategy for this PR?
eliminate_cross_joinunit tests, plus a neweliminate_cross_join_in_join_key_subquerytest covering an uncorrelated subquery (containing an eliminable cross join) that ends up in a join key.find_valid_equijoin_key_pair_inputs_of_different_widthsunit test, covering matching, swapped and non-matching keys with both a narrower left and a narrower right input.joins.sltandjoin.slt.part).ONfilters,AND/ORpredicates and uncorrelated scalar subqueries in filters and join keys): the output plans,transformedflags and errors were identical. It is not included here because it needs a copy of the old implementation.Are there any user-facing changes?
No, other than faster planning for queries with many joins. No public API changes.
🤖 Generated with Claude Code