refactor: add data type to define_plan_node macro (#136)

Add data type to `define_plan_node`. The benefits include:

- Pass actual data type in `new` instead of converting it to `Value`
outside first to reduce boilerplate code.
- Aggregate the serialization and deserialization to one place, to
ensure serialization and deserialization use the same format
- Data type is clear from the macro invocation

optd-datafusion-repr/src/plan_nodes.rs

@@ -208,8 +208,17 @@ pub trait OptRelNode: 'static + Clone {
     }
 }
 
-pub trait ExplainData: OptRelNode {
-    fn explain_data(data: &Value) -> Vec<(&'static str, Pretty<'static>)>;
+/// Plan nodes that are defined through `define_plan_node` macro with data
+/// field should implement this trait. Since data is stored as `Value` in
+/// `RelNode`, we need to now how to convert between `Value` and the actual
+/// data type.
+///
+/// For reasons why `explain_data`` needs to be explicitly implemented, see
+/// `define_plan_node`.
+pub trait ExplainData<T>: OptRelNode {
+    fn data_to_value(data: &T) -> Value;
+    fn value_to_data(value: &Value) -> T;
+    fn explain_data(data: &T) -> Vec<(&'static str, Pretty<'static>)>;
 }
 
 #[derive(Clone, Debug)]

optd-datafusion-repr/src/plan_nodes/macros.rs

@@ -1,29 +1,19 @@
-/// Plan nodes with data fields must implement `ExplainData` trait. An example:
+/// Plan nodes with data fields must implement `ExplainData` trait.
 ///
-/// ```ignore
-/// #[derive(Clone, Debug)]
-/// struct PhysicalDummy(PlanNode);
-///
-/// // Implement `OptRelNode` using `define_plan_node!`...
-///
-/// impl ExplainData for PhysicalDummy {
-///     fn explain_data(data: &Value) -> Vec<(&'static str, Pretty<'static>)> {
-///         if let Value::Int32(i) = data {
-///             vec![("primitive_data", i.to_string().into())]
-///         } else {
-///             unreachable!()
-///         }
-///     }
-/// }
-/// ```
+/// The generated `dispatch_explain` method delegates explaining data to
+/// rel node implementations of `ExplainData` trait instead of just debug
+/// printing it, because for complex data type (struct), derived debug printing
+/// displays struct name which should be hidden from the user. It also wraps
+/// the fields in braces, unlike the rest of the fields as children.
+
 macro_rules! define_plan_node {
     (
         $struct_name:ident : $meta_typ:tt,
         $variant:ident,
         [ $({ $child_id:literal, $child_name:ident : $child_meta_typ:ty }),* ] ,
         [ $({ $attr_id:literal, $attr_name:ident : $attr_meta_typ:ty }),* ]
         $(, { $inner_name:ident : $inner_typ:ty })?
-        $(, $data_name:ident)?
+        $(, $data_name:ident: $data_typ: ty )?
     ) => {
         impl OptRelNode for $struct_name {
             fn into_rel_node(self) -> OptRelNodeRef {
@@ -49,7 +39,7 @@ macro_rules! define_plan_node {
                 if let Some(meta_map) = meta_map {
                     fields = fields.with_meta(self.0.get_meta(meta_map));
                 };
-                define_plan_node!(@expand_fields self, $struct_name, fields $(, $data_name)?);
+                define_plan_node!(@expand_data_fields self, $struct_name, fields $(, $data_name)?);
 
                 pretty_xmlish::Pretty::simple_record(
                     stringify!($struct_name),
@@ -65,13 +55,13 @@ macro_rules! define_plan_node {
             pub fn new(
                 $($child_name : $child_meta_typ,)*
                 $($attr_name : $attr_meta_typ),*
-                $($data_name: Value)?
+                $($data_name: $data_typ)?
                 $(, $inner_name : $inner_typ)?
             ) -> $struct_name {
                 #[allow(unused_mut, unused)]
                 let mut data = None;
                 $(
-                    data = Some($data_name);
+                    data = Some($struct_name::data_to_value(&$data_name));
                 )*
                 $struct_name($meta_typ(
                     optd_core::rel_node::RelNode {
@@ -111,11 +101,11 @@ macro_rules! define_plan_node {
         }
     };
     // Dummy branch that does nothing when data is `None`.
-    (@expand_fields $self:ident, $struct_name:ident, $fields:ident) => {};
+    (@expand_data_fields $self:ident, $struct_name:ident, $fields:ident) => {};
     // Expand explain fields with data.
-    (@expand_fields $self:ident, $struct_name:ident, $fields:ident, $data_name:ident) => {
-        let data = $self.0 .0.data.as_ref().unwrap();
-        $fields.extend($struct_name::explain_data(data));
+    (@expand_data_fields $self:ident, $struct_name:ident, $fields:ident, $data_name:ident) => {
+        let value = $self.0 .0.data.as_ref().unwrap();
+        $fields.extend($struct_name::explain_data(&$struct_name::value_to_data(&value)));
     };
 }
 
@@ -151,40 +141,42 @@ mod test {
         #[derive(Clone, Debug)]
         struct PhysicalComplexDummy(PlanNode);
 
-        impl ExplainData for PhysicalComplexDummy {
-            fn explain_data(data: &Value) -> Vec<(&'static str, Pretty<'static>)> {
-                if let Value::Serialized(serialized_data) = data {
-                    let data: ComplexData = bincode::deserialize(serialized_data).unwrap();
-                    vec![
-                        ("a", data.a.to_string().into()),
-                        ("b", data.b.to_string().into()),
-                    ]
-                } else {
-                    unreachable!()
-                }
-            }
-        }
-
         define_plan_node!(
             PhysicalComplexDummy: PlanNode,
             PhysicalScan, [
                 { 0, child: PlanNode }
             ], [
             ],
-            complex_data
+            complex_data: ComplexData
         );
 
+        impl ExplainData<ComplexData> for PhysicalComplexDummy {
+            fn data_to_value(data: &ComplexData) -> Value {
+                Value::Serialized(bincode::serialize(data).unwrap().into_iter().collect())
+            }
+
+            fn value_to_data(value: &Value) -> ComplexData {
+                if let Value::Serialized(serialized_data) = value {
+                    bincode::deserialize(serialized_data).unwrap()
+                } else {
+                    unreachable!()
+                }
+            }
+
+            fn explain_data(data: &ComplexData) -> Vec<(&'static str, Pretty<'static>)> {
+                vec![
+                    ("a", data.a.to_string().into()),
+                    ("b", data.b.to_string().into()),
+                ]
+            }
+        }
+
         let node = PhysicalComplexDummy::new(
             LogicalScan::new("a".to_string()).0,
-            Value::Serialized(
-                bincode::serialize(&ComplexData {
-                    a: 1,
-                    b: "a".to_string(),
-                })
-                .unwrap()
-                .into_iter()
-                .collect(),
-            ),
+            ComplexData {
+                a: 1,
+                b: "a".to_string(),
+            },
         );
         let pretty = node.dispatch_explain(None);
         println!("{}", get_explain_str(&pretty));