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/09 11:13:42 UTC
[PR] feat(substrait): add intersect support to consumer [datafusion]
tokoko opened a new pull request, #12830:
URL: https://github.com/apache/datafusion/pull/12830
## What changes are included in this PR?
Adds support for intersect (primary) set operation to substrait consumer.
## Are these changes tested?
Intersect can't be round-tripped (at least not easily) because DF has no explicit logical plan node for the op, rather splits it into lower level operations. That's why the test is comparing DF plans produced by an externally written substrait plan and equivalent datafusion sql query.
## Are there any user-facing changes?
yes, a new feature
--
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] feat(substrait): add intersect support to consumer [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #12830:
URL: https://github.com/apache/datafusion/pull/12830#discussion_r1794316654
##########
datafusion/substrait/tests/testdata/test_plans/intersect.substrait.json:
##########
@@ -0,0 +1,118 @@
+{
+ "relations": [
+ {
+ "root": {
+ "input": {
+ "set": {
+ "inputs": [
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ },
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data2"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ }
+ ],
+ "op": "SET_OP_INTERSECTION_PRIMARY"
Review Comment:
If the intent is for this plan to correspond to
```sql
SELECT a FROM data INTERSECT SELECT a FROM data2"
```
this should actually be `SET_OP_INTERSECTION_MULTISET`. See relevant [docs].(https://substrait.io/relations/logical_relations/#set-operation-types)
This is tied to a relatively recent spec change: https://github.com/substrait-io/substrait/pull/708
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -887,6 +887,17 @@ pub async fn from_substrait_rel(
not_impl_err!("Union relation requires at least one input")
}
}
+ set_rel::SetOp::IntersectionPrimary => {
Review Comment:
See note on Substrait plan around mapping SQL INTERSECT into Substrait.
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -887,6 +887,17 @@ pub async fn from_substrait_rel(
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?,
+ from_substrait_rel(ctx, &set.inputs[1], extensions).await?,
+ false,
+ )
+ } else {
+ not_impl_err!("Primary Intersect relation with more than two inputs isn't supported")
Review Comment:
If we associate this DataFusion operation with `IntersectionMultiset` instead of `IntersectionPrimary`, we could potentially handle multiple inputs by chaining intersects. For example
```
SetRel(
op = SET_OP_INTERSECTION_MULTISET,
inputs = [<A>, <B>, <C>]
)
```
becomes something like
```rust
LogicalPlanBuilder::intersect(
from_substrait_rel(ctx, &set.inputs[0], extensions).await?,
LogicalPlanBuilder::intersect(
from_substrait_rel(ctx, &set.inputs[1], extensions).await?,
from_substrait_rel(ctx, &set.inputs[2], extensions).await?,
false
)
false)
```
--
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] feat(substrait): add intersect support to consumer [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #12830:
URL: https://github.com/apache/datafusion/pull/12830
--
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] feat(substrait): add intersect support to consumer [datafusion]
Posted by "tokoko (via GitHub)" <gi...@apache.org>.
tokoko commented on code in PR #12830:
URL: https://github.com/apache/datafusion/pull/12830#discussion_r1794620771
##########
datafusion/substrait/tests/testdata/test_plans/intersect.substrait.json:
##########
@@ -0,0 +1,118 @@
+{
+ "relations": [
+ {
+ "root": {
+ "input": {
+ "set": {
+ "inputs": [
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ },
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data2"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ }
+ ],
+ "op": "SET_OP_INTERSECTION_PRIMARY"
Review Comment:
thanks, I'll add other ops in follow-ups (primary with multiple rels and multisets) and that should clear the confusion, I think. The sql query used to test "primary with multiple rels" will probably be some combination of intersect and unions, right?
--
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] feat(substrait): add intersect support to consumer [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #12830:
URL: https://github.com/apache/datafusion/pull/12830#discussion_r1794414412
##########
datafusion/substrait/tests/testdata/test_plans/intersect.substrait.json:
##########
@@ -0,0 +1,118 @@
+{
+ "relations": [
+ {
+ "root": {
+ "input": {
+ "set": {
+ "inputs": [
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ },
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data2"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ }
+ ],
+ "op": "SET_OP_INTERSECTION_PRIMARY"
Review Comment:
> the intent is to add consumer rule for SET_OP_INTERSECTION_PRIMARY which isn't generally equivalent to INTERSECT op, but it is when used with 2 inputs,
Gotcha, that is correct for that specific case. It ends up being a little confusing because in the associated test
```
assert_substrait_sql(
proto_plan,
"SELECT a FROM data INTERSECT SELECT a FROM data2",
)
.await
```
The substrait plan ends up being _semantically_ equivalent to the SQL query, but is not the correct encoding generally, but I don't think that's a blocker.
--
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] feat(substrait): add intersect support to consumer [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #12830:
URL: https://github.com/apache/datafusion/pull/12830#issuecomment-2405983114
Thank you again @tokoko and @vbarua for the review
--
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] feat(substrait): add intersect support to consumer [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #12830:
URL: https://github.com/apache/datafusion/pull/12830#discussion_r1795877281
##########
datafusion/substrait/tests/testdata/test_plans/intersect.substrait.json:
##########
@@ -0,0 +1,118 @@
+{
+ "relations": [
+ {
+ "root": {
+ "input": {
+ "set": {
+ "inputs": [
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ },
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data2"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ }
+ ],
+ "op": "SET_OP_INTERSECTION_PRIMARY"
Review Comment:
> The sql query used to test "primary with multiple rels" will probably be some combination of intersect and unions, right?
I believe that SET_OP_INTERSECTION_PRIMARY with primary input `<A>` and secondary inputs `<B>`, `<C>`, ... ends up being equivalent to something like:
```sql
<A> INTERSECT DISTINCT (<B> UNION <C> UNION ...)
```
which satisfies the requirement that it outputs a row from `<A>` if is is present in _any_ of the secondary inputs.
--
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] feat(substrait): add intersect support to consumer [datafusion]
Posted by "tokoko (via GitHub)" <gi...@apache.org>.
tokoko commented on code in PR #12830:
URL: https://github.com/apache/datafusion/pull/12830#discussion_r1794381211
##########
datafusion/substrait/tests/testdata/test_plans/intersect.substrait.json:
##########
@@ -0,0 +1,118 @@
+{
+ "relations": [
+ {
+ "root": {
+ "input": {
+ "set": {
+ "inputs": [
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ },
+ {
+ "project": {
+ "common": {
+ "emit": {
+ "outputMapping": [
+ 1
+ ]
+ }
+ },
+ "input": {
+ "read": {
+ "common": {
+ "direct": {}
+ },
+ "baseSchema": {
+ "names": [
+ "a"
+ ],
+ "struct": {
+ "types": [
+ {
+ "i64": {
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ }
+ ],
+ "nullability": "NULLABILITY_NULLABLE"
+ }
+ },
+ "namedTable": {
+ "names": [
+ "data2"
+ ]
+ }
+ }
+ },
+ "expressions": [
+ {
+ "selection": {
+ "directReference": {
+ "structField": {}
+ },
+ "rootReference": {}
+ }
+ }
+ ]
+ }
+ }
+ ],
+ "op": "SET_OP_INTERSECTION_PRIMARY"
Review Comment:
no, it's the other way around. the intent is to add consumer rule for `SET_OP_INTERSECTION_PRIMARY` which isn't generally equivalent to `INTERSECT` op, but it is when used with 2 inputs, which is what's being tested here.
`SET_OP_INTERSECTION_MULTISET` should also be handled of course, but that would be in addition to what's being added here, not instead of, right?
--
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