Add rule to reimplement `Eliminate cross join` and remove it in planner #4185

jackwener · 2022-11-12T09:44:15Z

Which issue does this PR close?

Closes #4176.
Part of #4267

Current implementation of eliminate_cross_join is wrong. Because it can't rearrange join order. detail is inside Reimplement the eliminate_cross_join #4176..
remove the eliminate_cross_join from planner
reimplement eliminate_cross_join to avoid to use global-state. In new implementation, we just depends only on the shape of the subtree.

Rationale for this change

What changes are included in this PR?

Reimplement Eliminate cross join

Are these changes tested?

Test Reorder join for eliminating cross join.

Are there any user-facing changes?

datafusion/optimizer/src/eliminate_cross_join.rs

Dandandan · 2022-11-12T12:00:25Z

datafusion/sql/src/planner.rs

 \n TableScan: person\
 \n SubqueryAlias: p2\
 \n TableScan: person\
- \n Inner Join: person.id = p.id\
+ \n CrossJoin:\
 \n TableScan: person\


Some of the queries now have more cross joins? Is this expected?

Expected
It's just because it's just build plan without optimization.
quick_test () just invoke logical_plan_with_dialect

datafusion/expr/src/logical_plan/plan.rs

mingmwang · 2022-11-13T14:35:07Z

datafusion/sql/src/planner.rs


- // look for expressions of the form `<column> = <column>`
- let mut possible_join_keys = vec![];


Glad to see those logic is moved out from the planner.

datafusion/optimizer/src/eliminate_cross_join.rs

mingmwang · 2022-11-13T15:38:40Z

There is an important point about Schema need to discuss.

We can change Schema into set instead of list. Because it would cause many projection just for order of field.

such as
a b -> proj(join(b a))
a b c ->  join(proj(join(b a)) c) -> project(join(c, project (join(b a))))
Lots of projection will make our rule become complex, we should consider many case, especially for join reorder.

A simple but efficient way is override ==

I think it is because you make the rule run a bottom-up process, then lots of projections are added. If we make it a top-down
process, only a final projection will be added.

jackwener · 2022-11-14T02:25:48Z

Thanks @mingmwang 👍, I will fix them

jackwener · 2022-11-15T11:40:03Z

Thanks @mingmwang review !❤️
cc @liukun4515 @Dandandan @andygrove @alamb

mingmwang · 2022-11-15T11:48:25Z

@jackwener I will take a closer look Tomorrow.

alamb · 2022-11-15T19:49:59Z

I will try and review this in the next day or two -- but it may take me a while. Hopefully @mingmwang can help review as well

datafusion/optimizer/src/eliminate_cross_join.rs

mingmwang · 2022-11-18T03:38:41Z

There is specific logic in extract_possible_join_keys() to handle and pull up common Exprs in the Or branches.
But I think this logic should be handled in a common rule like ExprSimplifier.
@alamb
Once we reach the other rules, Exprs in Filters or Join conditions should be the already simplified form.

(A.a = B.b and B.x = xxx) or (A.a = B.b and B.y = yyy)

To

(A.a = B.b) and (B.x = xxx or B.y = yyy)

jackwener · 2022-11-18T04:39:11Z

There is specific logic in extract_possible_join_keys() to handle and pull up common Exprs in the Or branches. But I think this logic should be handled in a common rule like ExprSimplifier. @alamb Once we reach the other rules, Exprs in Filters or Join conditions should be the already simplified form.
(A.a = B.b and B.x = xxx) or (A.a = B.b and B.y = yyy)
To
(A.a = B.b) and (B.x = xxx or B.y = yyy)

Nice idea👍! It's a great future ticket.

alamb · 2022-11-20T12:11:42Z

I plan to review this and other join related PRs tomorrow. I apologize for the delays. The join work is really neat, but it is not a high priority at the moment in IOx so I have had to prioritize other work higher and do join related

I appreciate the help that @jackwener @mingmwang are giving each other in the review process. 🙏

alamb

I went through this PR quite thoroughly -- thank you very much @jackwener

I'll plan to merge this tomorrow unless there are any other comments

alamb · 2022-11-21T15:28:37Z

benchmarks/expected-plans/q2.txt

- Inner Join: nation.n_regionkey = region.r_regionkey
- Inner Join: supplier.s_nationkey = nation.n_nationkey
- Inner Join: partsupp.ps_suppkey = supplier.s_suppkey
+ Projection: part.p_partkey, part.p_mfgr, supplier.s_name, supplier.s_address, supplier.s_phone, supplier.s_acctbal, supplier.s_comment, nation.n_name


I found the difference much easier to see with whitespace blind diff

https://github.com/apache/arrow-datafusion/pull/4185/files?w=1

I found github's rendering of this really hard to understand the change in plan -- I drew out the join graphs by hand to make sure they were the same.

alamb · 2022-11-21T16:40:44Z

datafusion/sql/src/planner.rs

-fn extract_possible_join_keys(
- expr: &Expr,
- accum: &mut Vec<(Column, Column)>,
-) -> Result<()> {
- match expr {
- Expr::BinaryExpr(BinaryExpr { left, op, right }) => match op {
- Operator::Eq => match (left.as_ref(), right.as_ref()) {
- (Expr::Column(l), Expr::Column(r)) => {
- accum.push((l.clone(), r.clone()));
- Ok(())
- }
- _ => Ok(()),
- },
- Operator::And => {
- extract_possible_join_keys(left, accum)?;
- extract_possible_join_keys(right, accum)
- }
- _ => Ok(()),
- },
- _ => Ok(()),
- }


Maybe you are thinking about this copy: https://github.com/apache/arrow-datafusion/blob/bcd624855778384ee27648161de73951e3fb6ea1/datafusion/optimizer/src/reduce_cross_join.rs#L238-L279

alamb · 2022-11-21T16:43:06Z

datafusion/optimizer/src/eliminate_cross_join.rs

+ LogicalPlan::CrossJoin(_) => {
+ flatten_join_inputs(child, possible_join_keys, all_inputs)?;
+ }
+ _ => all_inputs.push((*child).clone()),


eventually it would be awesome to avoid so much cloneing -- maybe as a follow on PR

alamb · 2022-11-21T16:46:09Z

datafusion/optimizer/src/eliminate_cross_join.rs

@@ -849,14 +955,14 @@ mod tests {

 let expected = vec![
 "Filter: (t4.c < UInt32(15) OR t4.c = UInt32(688)) AND (t4.c < UInt32(15) OR t3.c = UInt32(688) OR t3.b = t4.b) [a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32]",
- " Inner Join: t1.a = t3.a [a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32]",


alamb · 2022-11-21T16:47:29Z

I took the liberty of merging master into the branch so that we avoid any possible logical conflicts.

Thanks again for all the comments @mingmwang and @Dandandan

alamb · 2022-11-21T16:49:00Z

There is specific logic in extract_possible_join_keys() to handle and pull up common Exprs in the Or branches. But I

Nice idea👍! It's a great future ticket.

I agree it would be a good idea for a future ticket. There is also https://github.com/apache/arrow-datafusion/blob/master/datafusion/optimizer/src/rewrite_disjunctive_predicate.rs which I think tries to do the same thing

alamb · 2022-11-21T16:49:12Z

cc @xudong963

jackwener · 2022-11-21T17:04:26Z

I took the liberty of merging master into the branch so that we avoid any possible logical conflicts.

Pulled master into it. Thanks @alamb ❤️

xudong963 · 2022-11-22T01:04:13Z

There is specific logic in extract_possible_join_keys() to handle and pull up common Exprs in the Or branches. But I

Nice idea👍! It's a great future ticket.

I agree it would be a good idea for a future ticket. There is also https://github.com/apache/arrow-datafusion/blob/master/datafusion/optimizer/src/rewrite_disjunctive_predicate.rs which I think tries to do the same thing

Yes, thanks for your mention :)

xudong963

Happy to see the logical moved from planner to optimizer, nice clean.

Maybe @DhamoPS would like to take a look, I remember he has worked similar work!

jackwener · 2022-11-22T12:53:43Z

fix new conflict.

alamb · 2022-11-22T22:25:02Z

Happy to see the logical moved from planner to optimizer, nice clean.

Maybe @DhamoPS would like to take a look, I remember he has worked similar work!

Good call @xudong963 -- I am going to merge this PR now as it has been outstanding for a long time. Thanks again @jackwener the recent contributions are awesome

ursabot · 2022-11-22T22:32:28Z

Benchmark runs are scheduled for baseline = bfce076 and contender = d355f69. d355f69 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner labels Nov 12, 2022

jackwener force-pushed the eliminate_cross_join branch from ab369b8 to 18a9743 Compare November 12, 2022 09:55

jackwener commented Nov 12, 2022

View reviewed changes

datafusion/optimizer/src/eliminate_cross_join.rs Outdated Show resolved Hide resolved

jackwener force-pushed the eliminate_cross_join branch from 2fcbd4b to acc0989 Compare November 12, 2022 10:01

Dandandan reviewed Nov 12, 2022

View reviewed changes

jackwener force-pushed the eliminate_cross_join branch from acc0989 to aee4741 Compare November 12, 2022 12:35

github-actions bot added core Core datafusion crate physical-expr Physical Expressions labels Nov 12, 2022

jackwener mentioned this pull request Nov 12, 2022

fix clippy #4190

Merged

jackwener force-pushed the eliminate_cross_join branch from aee4741 to c45744d Compare November 12, 2022 12:56

github-actions bot removed core Core datafusion crate physical-expr Physical Expressions labels Nov 12, 2022

jackwener force-pushed the eliminate_cross_join branch 3 times, most recently from a6e42cb to c5582ea Compare November 13, 2022 06:59

mingmwang reviewed Nov 13, 2022

View reviewed changes

datafusion/expr/src/logical_plan/plan.rs Outdated Show resolved Hide resolved

mingmwang reviewed Nov 13, 2022

View reviewed changes

datafusion/optimizer/src/eliminate_cross_join.rs Outdated Show resolved Hide resolved

mingmwang reviewed Nov 13, 2022

View reviewed changes

datafusion/optimizer/src/eliminate_cross_join.rs Show resolved Hide resolved

mingmwang mentioned this pull request Nov 14, 2022

Works for cross to inner join #3994

Closed

ygf11 mentioned this pull request Nov 14, 2022

Support all equality predicates in equality join #4193

Merged

jackwener force-pushed the eliminate_cross_join branch from c5582ea to beabb9c Compare November 15, 2022 11:35

github-actions bot added the core Core datafusion crate label Nov 15, 2022

andygrove self-requested a review November 18, 2022 00:40

jackwener force-pushed the eliminate_cross_join branch from b178f6e to 9a8ae5c Compare November 18, 2022 02:26

mingmwang reviewed Nov 18, 2022

View reviewed changes

datafusion/optimizer/src/eliminate_cross_join.rs Outdated Show resolved Hide resolved

mingmwang reviewed Nov 18, 2022

View reviewed changes

datafusion/optimizer/src/eliminate_cross_join.rs Show resolved Hide resolved

alamb approved these changes Nov 21, 2022

View reviewed changes

jackwener force-pushed the eliminate_cross_join branch from 2981931 to 27f965a Compare November 21, 2022 17:03

alamb mentioned this pull request Nov 21, 2022

Add another api to create join plan #4307

Closed

xudong963 approved these changes Nov 22, 2022

View reviewed changes

jackwener added 7 commits November 22, 2022 20:53

reimplement eliminate_cross_join

5d864ec

add test for subquery alias and projection alias.

5b023f0

add test

1e5955e

fix fmt

d36abd5

review

6176f1f

fmt

fb12fed

fix conflict

2e7a4c4

jackwener force-pushed the eliminate_cross_join branch from 27f965a to 2e7a4c4 Compare November 22, 2022 12:53

alamb merged commit d355f69 into apache:master Nov 22, 2022

jackwener deleted the eliminate_cross_join branch November 23, 2022 01:28

jackwener mentioned this pull request Nov 24, 2022

minor: correct name and typo #4359

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rule to reimplement `Eliminate cross join` and remove it in planner #4185

Add rule to reimplement `Eliminate cross join` and remove it in planner #4185

jackwener commented Nov 12, 2022 •

edited

Loading

Dandandan Nov 12, 2022

jackwener Nov 12, 2022 •

edited

Loading

mingmwang Nov 13, 2022

mingmwang commented Nov 13, 2022 •

edited

Loading

jackwener commented Nov 14, 2022

jackwener commented Nov 15, 2022

mingmwang commented Nov 15, 2022

alamb commented Nov 15, 2022

mingmwang commented Nov 18, 2022

jackwener commented Nov 18, 2022

alamb commented Nov 20, 2022

alamb left a comment

alamb Nov 21, 2022

alamb Nov 21, 2022

alamb Nov 21, 2022

alamb Nov 21, 2022

alamb commented Nov 21, 2022

alamb commented Nov 21, 2022

alamb commented Nov 21, 2022

jackwener commented Nov 21, 2022

xudong963 commented Nov 22, 2022

xudong963 left a comment

jackwener commented Nov 22, 2022

alamb commented Nov 22, 2022

ursabot commented Nov 22, 2022


		// look for expressions of the form `<column> = <column>`
		let mut possible_join_keys = vec![];

Add rule to reimplement Eliminate cross join and remove it in planner #4185

Add rule to reimplement Eliminate cross join and remove it in planner #4185

Conversation

jackwener commented Nov 12, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Dandandan Nov 12, 2022

Choose a reason for hiding this comment

jackwener Nov 12, 2022 • edited Loading

Choose a reason for hiding this comment

mingmwang Nov 13, 2022

Choose a reason for hiding this comment

mingmwang commented Nov 13, 2022 • edited Loading

jackwener commented Nov 14, 2022

jackwener commented Nov 15, 2022

mingmwang commented Nov 15, 2022

alamb commented Nov 15, 2022

mingmwang commented Nov 18, 2022

jackwener commented Nov 18, 2022

alamb commented Nov 20, 2022

alamb left a comment

Choose a reason for hiding this comment

alamb Nov 21, 2022

Choose a reason for hiding this comment

alamb Nov 21, 2022

Choose a reason for hiding this comment

alamb Nov 21, 2022

Choose a reason for hiding this comment

alamb Nov 21, 2022

Choose a reason for hiding this comment

alamb commented Nov 21, 2022

alamb commented Nov 21, 2022

alamb commented Nov 21, 2022

jackwener commented Nov 21, 2022

xudong963 commented Nov 22, 2022

xudong963 left a comment

Choose a reason for hiding this comment

jackwener commented Nov 22, 2022

alamb commented Nov 22, 2022

ursabot commented Nov 22, 2022

Add rule to reimplement `Eliminate cross join` and remove it in planner #4185

Add rule to reimplement `Eliminate cross join` and remove it in planner #4185

jackwener commented Nov 12, 2022 •

edited

Loading

jackwener Nov 12, 2022 •

edited

Loading

mingmwang commented Nov 13, 2022 •

edited

Loading