Support consuming Substrait with compound signature function names #10653

Blizzara · 2024-05-24T16:22:42Z

Substrait 0.32.0+ requires functions to be specified using compound names, which include the function name as well as the arguments it takes. We don't necessarily need that information while consuming the plans, but we need to support those compound names.

Which issue does this PR close?

Closes #10412, but doesn't fix the part where DataFusion generates those outdated simple names. But that's a bit bigger fix - this small thing would be enough to unblock me if it'd be okay to merge this before fixing the producer side.

Rationale for this change

See issue.

What changes are included in this PR?

When consuming Substrait, checks if given function names are compound names, if so strips the arguments part.

Are these changes tested?

Tested manually, but will still need to add unit tests.

Are there any user-facing changes?

Blizzara · 2024-05-24T16:25:20Z

@alamb curious what you think of fixing just the consumer side first, without touching the producer - if that'd be okay, then I can add some unit tests to this PR?

alamb · 2024-05-27T12:18:16Z

@alamb curious what you think of fixing just the consumer side first, without touching the producer - if that'd be okay, then I can add some unit tests to this PR?

I think it sounds good to me as long as there are sufficient test coverage.

Thank you for pushing this along @Blizzara

Substrait 0.32.0+ requires functions to be specified using compound names, which include the function name as well as the arguments it takes. We don't necessarily need that information while consuming the plans, but we need to support those compound names.

Blizzara · 2024-05-27T17:01:32Z

@alamb Sounds good! here's an added test, though curious if you have any thoughts on how to make it simpler - given DF doesn't yet produce those compound names, I cannot use a roundtrip test, and writing the substrait manually feels both annoying and error-prone.. Using base64 encoded binary proto would be a bit easier but then there's some recent bad experiences with people uploading malicious stuff as "test binaries" so I didn't want to do that, but happy to if you'd find it better 😅

alamb · 2024-05-28T13:12:31Z

I cannot use a roundtrip test, and writing the substrait manually feels both annoying and error-prone.

I don't have any better ideas unfortunately, until DataFusion has roundtrip support

alamb

Thank you @Blizzara -- this looks great. I really like the idea of having checked in json files with substrait from other systems (even apart from this particular feature)

The only question I have is the need to add a new (non dev) dependency. Otherwise this looks great

cc @waynexia

alamb · 2024-05-28T13:12:57Z

datafusion/substrait/Cargo.toml

@@ -37,11 +37,13 @@ chrono = { workspace = true }
 datafusion = { workspace = true, default-features = true }
 itertools = { workspace = true }
 object_store = { workspace = true }
+pbjson-types = "0.6"


does this need to be a actual dependency or can it be a dev dependency?

Answered in more depth here, but yes I think it does need to be actual dep

That said we can remove the prost-types dep, we don't need both: e6ff946

datafusion/substrait/src/logical_plan/consumer.rs

alamb · 2024-05-28T13:14:55Z

datafusion/substrait/src/logical_plan/producer.rs

@@ -42,7 +42,7 @@ use datafusion::logical_expr::expr::{
 };
 use datafusion::logical_expr::{expr, Between, JoinConstraint, LogicalPlan, Operator};
 use datafusion::prelude::Expr;
-use prost_types::Any as ProtoAny;
+use pbjson_types::Any as ProtoAny;


WHy do we need to change to pbjson_types here? It seems like the json support is only for testing...

So this confuses me a bit as well - but the way I understand it, adding the "serde" feature to "substrait-rs" seems to change the types it expects from the original prost types into the pbjson types (https://github.com/substrait-io/substrait-rs/blob/d11b22c743df9b02b7e5f4c079091ed133982431/build.rs#L262), probably to make them all serde-serializable (https://docs.rs/pbjson-types/latest/pbjson_types/#).

Given we use the Any proto type in non-test code, I think this does need to change, even though we don't necessarily need the serializability here.. This is also why we need the pbjson-types dep as normal, not dev, dep.

I'd also prefer to not change this, but I couldn't find another way if we want to have the JSON proto plans :/

alamb · 2024-05-28T13:15:41Z

datafusion/substrait/tests/cases/logical_plans.rs

+// under the License.
+
+#[cfg(test)]
+mod tests {


In general the idea of having checked in substrait plans (as json protobuf) is actually likely a pretty good way to build up a library of test cases "from the wild" (not round tripped)

This is quite cool @Blizzara

datafusion/substrait/tests/cases/logical_plans.rs

Co-authored-by: Andrew Lamb <[email protected]>

alamb

Thanks @Blizzara

waynexia · 2024-05-28T15:36:38Z

though curious if you have any thoughts on how to make it simpler - given DF doesn't yet produce those compound names, I cannot use a roundtrip test, and writing the substrait manually feels both annoying and error-prone

As a big scenario for our substrait support is to inter-cooperation with other systems, it might be a good idea to have some "integration" test, which uses another project like isthmus to produce plan and then consume them in datafusion side.

But this is not trivial to achieve, just an idea for now.

Blizzara · 2024-05-28T15:59:43Z

As a big scenario for our substrait support is to inter-cooperation with other systems, it might be a good idea to have some "integration" test, which uses another project like isthmus to produce plan and then consume them in datafusion side.

But this is not trivial to achieve, just an idea for now.

Yeah, I have that kind of integration testing internally, kind of, as part of our use of datafusion-substrait (see #10412 (comment)). I'll see if I can at some point contribute something like it here as well :)

waynexia · 2024-05-28T16:11:07Z

I've skimmed this patch and it looks good to me 👍 I'll take a detailed look into the test part tomorrow.

richtia · 2024-05-28T20:14:43Z

though curious if you have any thoughts on how to make it simpler - given DF doesn't yet produce those compound names, I cannot use a roundtrip test, and writing the substrait manually feels both annoying and error-prone

As a big scenario for our substrait support is to inter-cooperation with other systems, it might be a good idea to have some "integration" test, which uses another project like isthmus to produce plan and then consume them in datafusion side.

But this is not trivial to achieve, just an idea for now.

FYI...The substrait project also has a repo that aims to do integration testing between different substrait producers/consumers. https://github.com/substrait-io/consumer-testing

alamb · 2024-05-29T10:38:38Z

though curious if you have any thoughts on how to make it simpler - given DF doesn't yet produce those compound names, I cannot use a roundtrip test, and writing the substrait manually feels both annoying and error-prone

As a big scenario for our substrait support is to inter-cooperation with other systems, it might be a good idea to have some "integration" test, which uses another project like isthmus to produce plan and then consume them in datafusion side.
But this is not trivial to achieve, just an idea for now.

FYI...The substrait project also has a repo that aims to do integration testing between different substrait producers/consumers. https://github.com/substrait-io/consumer-testing

I filed #10710 to track. Thank you @richtia

alamb · 2024-05-29T10:38:46Z

Thanks again everyone!

…pache#10653) * Support consuming Substrait with compound signature function names Substrait 0.32.0+ requires functions to be specified using compound names, which include the function name as well as the arguments it takes. We don't necessarily need that information while consuming the plans, but we need to support those compound names. * Add a test for using "not:bool" * clippy fixes * Add license to new file * Apply suggestions from code review Co-authored-by: Andrew Lamb <[email protected]> * remove prost-types dep as it's replaced by pbjson-types --------- Co-authored-by: Andrew Lamb <[email protected]>

github-actions bot added the substrait label May 24, 2024

Blizzara added 2 commits May 27, 2024 18:14

Add a test for using "not:bool"

f39c63b

Blizzara force-pushed the avo/substrait-function-signature-compound-names branch from f7f69c5 to f39c63b Compare May 27, 2024 16:36

clippy fixes

90eaed9

Blizzara marked this pull request as ready for review May 27, 2024 16:53

Add license to new file

339ea97

alamb reviewed May 28, 2024

View reviewed changes

Blizzara and others added 2 commits May 28, 2024 16:02

Apply suggestions from code review

eb6cd9a

Co-authored-by: Andrew Lamb <[email protected]>

remove prost-types dep as it's replaced by pbjson-types

e6ff946

alamb approved these changes May 28, 2024

View reviewed changes

alamb mentioned this pull request May 29, 2024

Integrate with the substrait integration test #10710

Open

alamb merged commit 80a6b65 into apache:main May 29, 2024
25 checks passed

waynexia mentioned this pull request May 29, 2024

[EPIC] Tracking issue of support substrait logical plan #8149

Open

36 tasks

Blizzara deleted the avo/substrait-function-signature-compound-names branch May 29, 2024 14:30

alamb mentioned this pull request Jun 3, 2024

DataFusion weekly project plan (Andrew Lamb) - June 3, 2024 #10779

Closed

8 tasks

Blizzara mentioned this pull request Jun 28, 2024

fix: Support Substrait's compound names also for window functions #11163

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support consuming Substrait with compound signature function names #10653

Support consuming Substrait with compound signature function names #10653

Blizzara commented May 24, 2024 •

edited

Loading

Blizzara commented May 24, 2024

alamb commented May 27, 2024

Blizzara commented May 27, 2024

alamb commented May 28, 2024

alamb left a comment

alamb May 28, 2024

Blizzara May 28, 2024

Blizzara May 28, 2024

alamb May 28, 2024

Blizzara May 28, 2024

alamb May 28, 2024

alamb left a comment

waynexia commented May 28, 2024

Blizzara commented May 28, 2024

waynexia commented May 28, 2024

richtia commented May 28, 2024

alamb commented May 29, 2024

alamb commented May 29, 2024

Support consuming Substrait with compound signature function names #10653

Support consuming Substrait with compound signature function names #10653

Conversation

Blizzara commented May 24, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Blizzara commented May 24, 2024

alamb commented May 27, 2024

Blizzara commented May 27, 2024

alamb commented May 28, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb May 28, 2024

Choose a reason for hiding this comment

Blizzara May 28, 2024

Choose a reason for hiding this comment

Blizzara May 28, 2024

Choose a reason for hiding this comment

alamb May 28, 2024

Choose a reason for hiding this comment

Blizzara May 28, 2024

Choose a reason for hiding this comment

alamb May 28, 2024

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

waynexia commented May 28, 2024

Blizzara commented May 28, 2024

waynexia commented May 28, 2024

richtia commented May 28, 2024

alamb commented May 29, 2024

alamb commented May 29, 2024

Blizzara commented May 24, 2024 •

edited

Loading