You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "tokoko (via GitHub)" <gi...@apache.org> on 2024/10/20 17:56:51 UTC
[PR] fix(substrait): disallow union with a single input [datafusion]
tokoko opened a new pull request, #13023:
URL: https://github.com/apache/datafusion/pull/13023
## Which issue does this PR close?
Closes #12862
## Are there any user-facing changes?
union with a single input no longer works
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): disallow union with a single input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13023:
URL: https://github.com/apache/datafusion/pull/13023#issuecomment-2427605588
> This matches what the Substrait spec [requires](https://substrait.io/relations/logical_relations/#set-properties). One thing we should mentioned is that this is technically a breaking change, as UNION or UNION ALL plans with only a single-input will stop working.
Marked it as API change
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): disallow union with a single input [datafusion]
Posted by "tokoko (via GitHub)" <gi...@apache.org>.
tokoko commented on code in PR #13023:
URL: https://github.com/apache/datafusion/pull/13023#discussion_r1809271608
##########
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:
Can't really judge if it's idiomatic or not with my whole 2 weeks of rust experience :laughing: but makes sense. I'll flip it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): disallow union with a single input [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13023:
URL: https://github.com/apache/datafusion/pull/13023
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix(substrait): disallow union with a single input [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org