Uh oh!
There was an error while loading. Please reload this page.
Add initial support for serializing physical plans with Substrait - #5176
Conversation
andygrove
commented
Feb 5, 2023
| pub mod consumer; | ||
| pub mod producer; | ||
| pub mod logical_plan; |
There was a problem hiding this comment.
I like these name changes. Make it much more clear what the library is trying to offer.
| expression::MaskExpression, read_rel::ReadType, rel::RelType, Rel, | ||
| }; | ||
| /// Convert Substrait Rel to DataFusion LogicalPlan |
There was a problem hiding this comment.
Nit, shouldn't this be ExecutionPlan
| match &rel.rel_type { | ||
| Some(RelType::Read(read)) => { | ||
| if read.filter.is_some() || read.best_effort_filter.is_some() { | ||
| return Err(DataFusionError::NotImplemented( |
There was a problem hiding this comment.
Is this just because this is a first pass at getting the framework together or is it truly not possible with DataFusion today?
There was a problem hiding this comment.
Yes, just a first pass. I plan on following up with smaller PRs to make this usable.
waynexia
left a comment
There was a problem hiding this comment.
Looks good to me 🚀
I also tried to do physical plan - substrait proto conversion, but gives up and turns to logical plan for simplicity. Glad to see this work starts again.
ursabot
commented
Feb 7, 2023
Benchmark runs are scheduled for baseline = d5077b5 and contender = de3deb3. de3deb3 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Part of #5173
Rationale for this change
I would like to be able to use Substrait with physical plans.
What changes are included in this PR?
substrait::logical_plannamespace.substrait::physical_plannamespace.ParquetExec(really, not enough to be actually useful, but gets the structure in place).Are these changes tested?
New test added
Are there any user-facing changes?
No