Implement LogicalPlan serde in datafusion-proto
#2639
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
datafusion-protoLogicalPlan serde in datafusion-proto
This reverts commit 96f564f.
Member
Author
yjshen
approved these changes
May 29, 2022
thinkharderdev
approved these changes
May 29, 2022
alamb
approved these changes
May 29, 2022
| } | ||
| } | ||
|
|
||
| /// Serialize a LogicalPlan as bytes |
Contributor
There was a problem hiding this comment.
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 🤷
Member
Author
There was a problem hiding this comment.
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); |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn roundtrip_logical_plan() -> Result<(), DataFusionError> { |
Contributor
There was a problem hiding this comment.
It might be good to add a test that uses an extension codec, to prevent future regressions
Member
Author
There was a problem hiding this comment.
I added a test based on the TopK extension codec from Ballista
This was referenced May 29, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.