-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Remove logical cross join in planning #12985
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 22 commits
3847328
a706d33
90d49b2
8b1e0e9
74922fb
3db31ce
6ce76b5
74971e5
9d6d902
0d71841
69416ba
5f0803b
5b53758
f1c7c87
d34449a
3c71aad
bfaf917
7665fdc
a517968
9fc36e1
7f21607
4c89dd2
ef54eb7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,6 +222,7 @@ pub enum LogicalPlan { | |
| Join(Join), | ||
| /// Apply Cross Join to two logical plans. | ||
| /// This is used to implement SQL `CROSS JOIN` | ||
| /// Deprecated: use [LogicalPlan::Join] instead with empty `on` / no filter | ||
| CrossJoin(CrossJoin), | ||
| /// Repartitions the input based on a partitioning scheme. This is | ||
| /// used to add parallelism and is sometimes referred to as an | ||
|
|
@@ -1873,6 +1874,11 @@ impl LogicalPlan { | |
| .as_ref() | ||
| .map(|expr| format!(" Filter: {expr}")) | ||
| .unwrap_or_else(|| "".to_string()); | ||
| let join_type = if filter.is_none() && keys.is_empty() && matches!(join_type, JoinType::Inner) { | ||
| "Cross".to_string() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we extend
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it’s better not to, otherwise it will be similar to having |
||
| } else { | ||
| join_type.to_string() | ||
| }; | ||
| match join_constraint { | ||
| JoinConstraint::On => { | ||
| write!( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we create a separate ticket on "optimize the plan, and use the config of
target_partitionsandrepartition_joins" and remove the comments?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what the comment means, it was copied. I think
repartition_joinsis not needed forNestedLoopJoinandtarget_partitionsshould be already used on child plans...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just remove the comment