[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #2639: Implement `LogicalPlan` serde in `datafusion-proto`

[GitBox <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org