gabotechs commented on code in PR #18068:
URL: https://github.com/apache/datafusion/pull/18068#discussion_r2434809760
##########
datafusion/substrait/src/logical_plan/consumer/rel/join_rel.rs:
##########
@@ -75,15 +76,22 @@ pub async fn from_join_rel(
.build()
}
None => {
- let on: Vec<String> = vec![];
- left.join_detailed(
- right.build()?,
- join_type,
- (on.clone(), on),
- None,
- NullEquality::NullEqualsNothing,
- )?
- .build()
+ // For joins without conditions, use cross_join if inner,
otherwise use a filter with Boolean(true)
+ if join_type == JoinType::Inner {
+ left.cross_join(right.build()?)?.build()
+ } else {
Review Comment:
One thing to take into account is that Postgres does not allow this. In
Postgres, it's invalid to provide an inner join without a join condition
(although other DBs like MySQL allow it).
My impression is that the change in
https://github.com/apache/datafusion/pull/15334 was actually trying to adhere
to this standard, which is a fair decision, so that means that probably the
Substrait path should also adhere to that standard, unless there's a strong
reason to diverge.
Don't have a strong opinion about whether if we should automatically convert
inner joins without conditions to cross joins or not, but I'd advocate for
consistency in both the normal SQL path and the Substrait path
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
