You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "vbarua (via GitHub)" <gi...@apache.org> on 2024/12/31 22:40:43 UTC
[PR] feat(substrait): introduce consume_rel and consume_expression [datafusion]
vbarua opened a new pull request, #13963:
URL: https://github.com/apache/datafusion/pull/13963
## Which issue does this PR close?
Additional work for https://github.com/apache/datafusion/issues/13318
## Rationale for this change
Improve the ergonomics of the consumer and fixed a gap in null literal handling.
## What changes are included in this PR?
### Consumer Improvements
Adds two handlers to the SubstraitConsumer
* `consume_rel`
* `consume_expression`
and routes all calls to `from_substrait_rel` and `from_substrait_rex` through them to allow users to customize behaviour at the Rel and Expression level.
### Null Literal Improvements
The existing code in `from_substrait_null` would fail to handle nulls of user-defined types. The null handling code has been simplified to first convert the type of the null into a DataFusion DataType, which already allows for conversion of user-defined Substrait types using `consume_user_defined_type` and then converting that DataType into a ScalarValue.
As an added benefit, this also allows us to remove a big-match block in `from_substrait_null`.
## Are these changes tested?
Covered by existing conversion tests.
## Are there any user-facing changes?
There are user-facing changes that require not work from uses. There are now additional methods on the SubstraitConsumer trait, but they have default implementations.
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13963:
URL: https://github.com/apache/datafusion/pull/13963#discussion_r1900394650
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -233,6 +233,10 @@ pub trait SubstraitConsumer: Send + Sync + Sized {
// These methods have default implementations calling the common handler code, to allow for users
// to re-use common handling logic.
+ async fn consume_rel(&self, rel: &Rel) -> Result<LogicalPlan> {
Review Comment:
I recommend documenting such methods with `///` style comments that show up in the API docs https://docs.rs/crate/datafusion-substrait/latest
- Of course the API docs seem to be broken now 😠https://github.com/apache/datafusion/issues/13853
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13963:
URL: https://github.com/apache/datafusion/pull/13963
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "Blizzara (via GitHub)" <gi...@apache.org>.
Blizzara commented on code in PR #13963:
URL: https://github.com/apache/datafusion/pull/13963#discussion_r1901637948
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -744,10 +752,10 @@ pub async fn from_substrait_plan_with_consumer(
match plan.relations[0].rel_type.as_ref() {
Some(rt) => match rt {
plan_rel::RelType::Rel(rel) => {
- Ok(from_substrait_rel(consumer, rel).await?)
+ Ok(consumer.consume_rel( rel).await?)
},
plan_rel::RelType::Root(root) => {
- let plan = from_substrait_rel(consumer, root.input.as_ref().unwrap()).await?;
+ let plan = consumer.consume_rel( root.input.as_ref().unwrap()).await?;
Review Comment:
nit: there's seems to be some extra whitespace here and above?
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13963:
URL: https://github.com/apache/datafusion/pull/13963#discussion_r1901924005
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -233,6 +233,10 @@ pub trait SubstraitConsumer: Send + Sync + Sized {
// These methods have default implementations calling the common handler code, to allow for users
// to re-use common handling logic.
+ async fn consume_rel(&self, rel: &Rel) -> Result<LogicalPlan> {
Review Comment:
Added a little bit of documentation for these. I'm planning to add some module levels docs as part of https://github.com/apache/datafusion/issues/13864.
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13963:
URL: https://github.com/apache/datafusion/pull/13963#discussion_r1900285794
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -3082,180 +3087,6 @@ fn from_substrait_literal(
Ok(scalar_value)
}
-fn from_substrait_null(
- consumer: &impl SubstraitConsumer,
- null_type: &Type,
- dfs_names: &[String],
- name_idx: &mut usize,
-) -> Result<ScalarValue> {
Review Comment:
The match block below is effectively replaced by converting the Substrait Type to a DataFusion DataType, and then invoking `ScalarValue::try_from` which does the same thing effectively: https://github.com/apache/datafusion/blob/9eca7d165c3ddcd3449e833df9391b8216e0f5bc/datafusion/common/src/scalar/mod.rs#L3408-L3523
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13963:
URL: https://github.com/apache/datafusion/pull/13963#issuecomment-2571328548
Thanks again @vbarua and @Blizzara
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on code in PR #13963:
URL: https://github.com/apache/datafusion/pull/13963#discussion_r1901926751
##########
datafusion/substrait/src/logical_plan/consumer.rs:
##########
@@ -744,10 +752,10 @@ pub async fn from_substrait_plan_with_consumer(
match plan.relations[0].rel_type.as_ref() {
Some(rt) => match rt {
plan_rel::RelType::Rel(rel) => {
- Ok(from_substrait_rel(consumer, rel).await?)
+ Ok(consumer.consume_rel( rel).await?)
},
plan_rel::RelType::Root(root) => {
- let plan = from_substrait_rel(consumer, root.input.as_ref().unwrap()).await?;
+ let plan = consumer.consume_rel( root.input.as_ref().unwrap()).await?;
Review Comment:
Removed
--
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): introduce consume_rel and consume_expression [datafusion]
Posted by "vbarua (via GitHub)" <gi...@apache.org>.
vbarua commented on PR #13963:
URL: https://github.com/apache/datafusion/pull/13963#issuecomment-2566743090
@Blizzara some more improvements for the consumer, for when you have a chance. I'm planning on splitting out the consumer and producer into submodules after this and the producer change land.
--
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