alamb commented on code in PR #2639:
URL: https://github.com/apache/arrow-datafusion/pull/2639#discussion_r884249643
##########
datafusion/proto/src/lib.rs:
##########
@@ -23,24 +27,42 @@ pub mod protobuf {
pub mod bytes;
pub mod from_proto;
+pub mod logical_plan;
pub mod to_proto;
+#[cfg(doctest)]
+doc_comment::doctest!("../README.md", readme_example_test);
Review Comment:
👍
##########
datafusion/proto/src/bytes/mod.rs:
##########
@@ -93,6 +101,69 @@ impl Serializeable for Expr {
}
}
+/// Serialize a LogicalPlan as bytes
Review Comment:
I wonder if it is time to think about a slightly more encapsulated API,
something that would allow
```rust
let bytes = PlanSerializer::new()
.with_extension_codec(&extension_codec)
.serialize()?;
```
Rather than having two separate free functions 🤷
##########
datafusion/proto/src/lib.rs:
##########
@@ -62,6 +84,18 @@ mod roundtrip_tests {
Box::new(Field::new(name, dt, nullable))
}
+ #[tokio::test]
+ async fn roundtrip_logical_plan() -> Result<(), DataFusionError> {
Review Comment:
It might be good to add a test that uses an extension codec, to prevent
future regressions
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
