-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Substrait] Change API to return LogicalPlan instead of DataFrame #4896
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks @andygrove
// #[allow(deprecated)] | ||
// let plan = ctx.optimize(&plan)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some deadcode
Benchmark runs are scheduled for baseline = 2630fd7 and contender = 03ef500. 03ef500 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 #4897
Rationale for this change
As suggested by @nseekhao, given that to_substrait_rel() takes LogicalPlan and returns Substrait Rel, it would be better if from_subtrait_rel() takes Substrait Rel and returns LogicalPlan rather than DataFrame.
Note that this was originally datafusion-contrib/datafusion-substrait#35
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?