vbarua commented on code in PR #13023:
URL: https://github.com/apache/datafusion/pull/13023#discussion_r1809253711
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -937,72 +937,42 @@ pub async fn from_substrait_rel(
}
}
Some(RelType::Set(set)) => match set_rel::SetOp::try_from(set.op) {
- Ok(set_op) => match set_op {
- set_rel::SetOp::UnionAll => {
- if !set.inputs.is_empty() {
- union_rels(&set.inputs, ctx, extensions, true).await
- } else {
- not_impl_err!("Union relation requires at least one
input")
- }
- }
- set_rel::SetOp::UnionDistinct => {
- if !set.inputs.is_empty() {
- union_rels(&set.inputs, ctx, extensions, false).await
- } else {
- not_impl_err!("Union relation requires at least one
input")
- }
- }
- set_rel::SetOp::IntersectionPrimary => {
- if set.inputs.len() >= 2 {
- LogicalPlanBuilder::intersect(
- from_substrait_rel(ctx, &set.inputs[0],
extensions).await?,
- union_rels(&set.inputs[1..], ctx, extensions,
true).await?,
- false,
- )
- } else {
- not_impl_err!(
- "Primary Intersect relation requires at least two
inputs"
- )
- }
- }
- set_rel::SetOp::IntersectionMultiset => {
- if set.inputs.len() >= 2 {
- intersect_rels(&set.inputs, ctx, extensions,
false).await
- } else {
- not_impl_err!(
- "Multiset Intersect relation requires at least two
inputs"
- )
- }
- }
- set_rel::SetOp::IntersectionMultisetAll => {
- if set.inputs.len() >= 2 {
- intersect_rels(&set.inputs, ctx, extensions,
true).await
- } else {
- not_impl_err!(
- "MultisetAll Intersect relation requires at least
two inputs"
- )
- }
- }
- set_rel::SetOp::MinusPrimary => {
- if set.inputs.len() >= 2 {
- except_rels(&set.inputs, ctx, extensions, false).await
- } else {
- not_impl_err!(
- "Primary Minus relation requires at least two
inputs"
- )
- }
- }
- set_rel::SetOp::MinusPrimaryAll => {
- if set.inputs.len() >= 2 {
- except_rels(&set.inputs, ctx, extensions, true).await
- } else {
- not_impl_err!(
- "PrimaryAll Minus relation requires at least two
inputs"
- )
+ Ok(set_op) => {
+ if set.inputs.len() >= 2 {
Review Comment:
minor: Would it be cleaner to pull this out to the top like
```rust
if set.inputs.len() <= 1 {
substrait_err!("Set operation requires at least two inputs")
}
```
so it's clearer what the invariant is (by having the check right next to the
error) and avoid indenting the match block?
(I'm not sure if this is idiomatic Rust)
--
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]
