Remove logical cross join in planning #12985
Conversation
github-actions
bot
added
logical-expr
github-actions
bot
added
core
Dandandan
changed the title
Dandandan
changed the title
|
This is now ready for review |
| Arc::new(CrossJoinExec::new(physical_left, physical_right)) | ||
| } else { | ||
| // there is no equal join condition, use the nested loop join | ||
| // TODO optimize the plan, and use the config of `target_partitions` and `repartition_joins` |
There was a problem hiding this comment.
should we create a separate ticket on "optimize the plan, and use the config of target_partitions and repartition_joins" and remove the comments?
There was a problem hiding this comment.
I am not sure what the comment means, it was copied. I think repartition_joins is not needed for NestedLoopJoin and target_partitions should be already used on child plans...
There was a problem hiding this comment.
Maybe we can just remove the comment
| @@ -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() | |||
There was a problem hiding this comment.
should we extend JoinType enum to support Cross?
There was a problem hiding this comment.
I think it’s better not to, otherwise it will be similar to having LogicalPlan::CrossJoin (I.e. unnecesary).
There was a problem hiding this comment.
lgtm thanks @Dandandan some comments and the PR is good to go
Co-authored-by: Oleks V <comphead@users.noreply.github.com>
| Arc::new(CrossJoinExec::new(physical_left, physical_right)) | ||
| } else { | ||
| // there is no equal join condition, use the nested loop join | ||
| // TODO optimize the plan, and use the config of `target_partitions` and `repartition_joins` |
There was a problem hiding this comment.
Maybe we can just remove the comment
still todo: - [x] add tests ## Notes for reviewers: This does not actually implement a physical cross join, but just implements the logical cross join as well as cross join to inner join optimization `eliminate_cross_join.rs` This treats an inner join with no join conditions as cross join. (inspired by a recent [change in datafusion](apache/datafusion#12985)). If the cross join can not be optimized away, an error will be raised when attempting to execute the plan.
Which issue does this PR close?
Closes #13007
Rationale for this change
This simplifies (optimization) code dealing with logical plan, as it only has to deal with
LogicalPlan::Join.What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?