-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Upgrade to substrait 0.3 #4895
Upgrade to substrait 0.3 #4895
Conversation
datafusion/substrait/src/producer.rs
Outdated
@@ -74,6 +74,7 @@ pub fn to_substrait_plan(plan: &LogicalPlan) -> Result<Box<Plan>> { | |||
|
|||
// Return parsed plan | |||
Ok(Box::new(Plan { | |||
version: None, // TODO not sure what version should go here? |
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.
I don't know either so more of a question. Is this intended to be the substrait version?? So in this context would be 0.3
you think?
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.
My guess is that this will be the version of the substrait specification (which is separate to the Rust crate version)
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.
substrait-io/substrait-rs#46 adds a version
function that returns the version of Substrait used to build the crate. That could be used here. Or something like version_with_producer("datafusion")
. Please note that the version field of a plan is required.
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.
I filed #4949 to implement this. I would like to get this PR merged to unblock integrating this code into the main workspace
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.
I also added a link to the issue in the code comment
This |
I plan on merging this PR once CI is green unless there are objections, then I can undraft the PR to integrate datafusion-substrait with the main workspace |
Benchmark runs are scheduled for baseline = 489a555 and contender = 84ba3c2. 84ba3c2 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?
Closes #4894
Rationale for this change
Fix CI issue with protoc
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?