-
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
Implement LogicalPlan
serde in datafusion-proto
#2639
Conversation
datafusion-proto
LogicalPlan
serde in datafusion-proto
This reverts commit 96f564f.
@@ -93,6 +101,69 @@ impl Serializeable for Expr { | |||
} | |||
} | |||
|
|||
/// Serialize a LogicalPlan as bytes |
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 wonder if it is time to think about a slightly more encapsulated API, something that would allow
let bytes = PlanSerializer::new()
.with_extension_codec(&extension_codec)
.serialize()?;
Rather than having two separate free functions 🤷
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.
Yes, I had been thinking about this too and we will need a unified API to implement subquery support (because expressions now reference plans) so I think we can make this API change as part of #2640
pub mod to_proto; | ||
|
||
#[cfg(doctest)] | ||
doc_comment::doctest!("../README.md", readme_example_test); |
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.
👍
@@ -62,6 +84,18 @@ mod roundtrip_tests { | |||
Box::new(Field::new(name, dt, nullable)) | |||
} | |||
|
|||
#[tokio::test] | |||
async fn roundtrip_logical_plan() -> Result<(), DataFusionError> { |
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.
It might be good to add a test that uses an extension codec, to prevent future regressions
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 added a test based on the TopK extension codec from Ballista
Which issue does this PR close?
Closes #2630
Rationale for this change
I want to be able to serialize logical plans.
What changes are included in this PR?
Example:
Are there any user-facing changes?
New APIs
Does this PR break compatibility with Ballista?
No, but I plan on updating Ballista soon to use these new APIs.