alamb commented on code in PR #8427:
URL: https://github.com/apache/arrow-datafusion/pull/8427#discussion_r1416235766
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -332,6 +333,18 @@ pub fn to_substrait_rel(
}))),
}))
}
+ LogicalPlan::CrossJoin(cross_join) => {
+ let left = to_substrait_rel(cross_join.left.as_ref(), ctx,
extension_info)?;
Review Comment:
👍
I double checked there are no extra fields that are missed:
https://github.com/apache/arrow-datafusion/blob/2e5ad7a3cb3b3f3e96f931b903eb8bb6639329ff/datafusion/expr/src/logical_plan/plan.rs#L2050
One way to potentially make this code more future proof if we add fields to
`CrossJoin` in the future (aka have the compiler tell about places that may
need updates) would be to use a direct restructuring
Like
```suggestion
let CrossJoin { left, right, schema: _} = cross_join
let left = to_substrait_rel(left.as_ref(), ctx, extension_info)?;
```
...
##########
datafusion/substrait/src/logical_plan/producer.rs:
##########
@@ -332,6 +333,18 @@ pub fn to_substrait_rel(
}))),
}))
}
+ LogicalPlan::CrossJoin(cross_join) => {
+ let left = to_substrait_rel(cross_join.left.as_ref(), ctx,
extension_info)?;
Review Comment:
👍
I double checked there are no extra fields that are missed:
https://github.com/apache/arrow-datafusion/blob/2e5ad7a3cb3b3f3e96f931b903eb8bb6639329ff/datafusion/expr/src/logical_plan/plan.rs#L2050
One way to potentially make this code more future proof if we add fields to
`CrossJoin` in the future (aka have the compiler tell about places that may
need updates) would be to use a direct restructuring
Like
```suggestion
let CrossJoin { left, right, schema: _} = cross_join
let left = to_substrait_rel(left.as_ref(), ctx, extension_info)?;
```
...
--
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]
