gabotechs commented on code in PR #26603:
URL: https://github.com/apache/datafusion/pull/26603#discussion_r4452105450
##########
datafusion/substrait/src/logical_plan/consumer/rel/join_rel.rs:
##########
@@ -33,12 +36,20 @@ pub async fn from_join_rel(
return not_impl_err!("JoinRel with post_join_filter is not yet
supported");
}
- let left: LogicalPlanBuilder = LogicalPlanBuilder::from(
- consumer.consume_rel(join.left.as_ref().unwrap()).await?,
- );
- let right = LogicalPlanBuilder::from(
- consumer.consume_rel(join.right.as_ref().unwrap()).await?,
- );
+ let (Some(left), Some(right)) = (join.left.as_deref(),
join.right.as_deref()) else {
+ return substrait_err!("JoinRel requires left and right inputs");
+ };
+ let [left, right] = consumer
+ .consume_rel_inputs(&[left, right])
Review Comment:
Then this is still a problem for the new `.consume_rel_inputs()` API, people
implementing it can decide to do `try_join_all()` inside and shoot themselves
in the foot producing incorrect plans because of mutation races to the lambda
parameters or the correlated subqueries.
So whether we ship the `.consume_rel_inputs()` or do the `try_join_all()`
ourselves in this code here seems like a matter of who is responsible for
producing incorrect plans, DataFusion or its consumers, but from an API design
standpoint, I don't think neither of the two are acceptable, we should not
offer APIs that can silently produce incorrect plans.
--
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]
