You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/04/29 16:07:18 UTC

[PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

adriangb opened a new pull request, #21929:
URL: https://github.com/apache/datafusion/pull/21929

   ## Which issue does this PR close?
   
   - Closes #21835.
   
   ## Rationale for this change
   
   `datafusion-proto` serializes every built-in `PhysicalExpr` through a single ~300-line `downcast_ref` chain in `serialize_physical_expr_with_converter`, with a symmetric `match` on `ExprType` in `from_proto.rs`. Because the serializer lives outside the crate where each expression is defined, **every piece of internal state an expression wants to round-trip has to be `pub`**. The motivating example was #21807, which had to expose `pub struct Inner`, `pub fn inner()`, `pub fn from_parts()`, `pub fn original_children()`, `pub fn remapped_children()` on `DynamicFilterPhysicalExpr` — all "warning: not stable; proto-only".
   
   This PR puts the infrastructure in place so future stateful expressions don't hit that wall:
   
   1. The prost-generated types move to a new lightweight `datafusion-proto-models` crate.
   2. `PhysicalExpr` gains an opt-in `to_proto` method (feature-gated).
   3. Per-expression `try_from_proto` constructors land via a parallel `PhysicalExprDecodeCtx`.
   4. `Column` is migrated end-to-end as a working demo of the new pattern.
   
   The remaining built-in expressions stay on the existing downcast/match path; they migrate one-by-one in follow-ups (each migration is independent and keeps behavior unchanged).
   
   ## What changes are included in this PR?
   
   Five stacked commits, each independently mergeable / splittable later:
   
   1. **Extract `datafusion-proto-models` crate.** Pure refactor — moves `.proto`, gen, `src/generated/*` into a new lightweight crate (mirror of `datafusion-proto-common`'s split). `datafusion-proto` re-exports the proto types via `pub mod protobuf`, so external consumers (`datafusion-ffi`, `datafusion-examples`, `benchmarks`) need no changes.
   
      Because the prost types are now foreign to `datafusion-proto`, the orphan rule forced two pieces of trait surgery:
      - Inherent `impl protobuf::PhysicalPlanNode { ... }` → new `PhysicalPlanNodeExt` trait. Callers that used `protobuf::PhysicalPlanNode::try_from_physical_plan_with_converter(...)` need a `use datafusion_proto::physical_plan::PhysicalPlanNodeExt;` (small API break, all in-tree call sites updated).
      - `impl From<&protobuf::X> for Y` (and `TryFrom`) for foreign-foreign pairs → new `FromProto`/`TryFromProto` traits in `datafusion_proto::convert`. Callers go from `(&p).into()` to `Y::from_proto(&p)`.
   
   2. **Add `PhysicalExpr::to_proto` hook (feature-gated).** Adds an opt-in `proto` feature on `datafusion-physical-expr-common`:
   
      ```rust
      #[cfg(feature = \"proto\")]
      fn to_proto(
          &self,
          ctx: &PhysicalExprEncodeCtx<'_>,
      ) -> Result<Option<PhysicalExprNode>> { Ok(None) }
      ```
   
      `Ok(None)` (the default) means \"fall through\" — `datafusion-proto`'s downcast chain handles the expression. `Ok(Some(node))` lets the expression serialize itself, so types with private state (`DynamicFilterPhysicalExpr`'s `RwLock`-wrapped inner, etc.) won't need pub-for-proto accessors when they migrate.
   
   3. **Migrate `Column` to `PhysicalExpr::to_proto`.** First expression to use the hook. Removes its arm from the downcast chain.
   
   4. **Wrap encoder in `PhysicalExprEncodeCtx` struct.** The bare `PhysicalExprEncoder` trait moves behind a concrete `PhysicalExprEncodeCtx` struct so expression authors don't see `&dyn` in their signatures. Mirrors how `PhysicalPlanDecodeContext` is shaped today. Gives us a stable place to add helpers (UDF/UDAF/UDWF encoding, future registry hooks) without expanding a public trait surface.
   
   5. **Add `PhysicalExprDecodeCtx` and migrate `Column` decode.** Decode-side counterpart with a single-method API:
   
      ```rust
      pub struct PhysicalExprDecodeCtx<'a> { ... }
      impl PhysicalExprDecodeCtx<'_> {
          pub fn schema(&self) -> &Schema;
          pub fn decode(&self, node: &PhysicalExprNode) -> Result<Arc<dyn PhysicalExpr>>;
      }
   
      impl Column {
          pub fn try_from_proto(
              node: &PhysicalColumn,
              ctx: &PhysicalExprDecodeCtx<'_>,
          ) -> Result<Arc<Self>>;
      }
      ```
   
      Whatever \"decode\" needs to do (central match for built-ins, codec/registry for extensions) is hidden behind one `decode` method on the ctx — same shape regardless of expression origin.
   
   ## Are these changes tested?
   
   The existing `roundtrip_physical_plan` and `roundtrip_physical_expr` tests cover `Column` round-tripping in both directions, and they exercise the new `to_proto` / `try_from_proto` path. All 159 proto tests pass. (6 pre-existing failures require the `parquet-testing` git submodule, unrelated to this change.)
   
   No new tests were added because no new behavior was introduced — the migrated `Column` produces and consumes the same wire format as before.
   
   ## Are there any user-facing changes?
   
   Yes, small API breaks in `datafusion-proto`:
   
   - `protobuf::PhysicalPlanNode::try_from_physical_plan_with_converter` and `try_into_physical_plan_with_converter` are now methods on the `PhysicalPlanNodeExt` trait. External callers need `use datafusion_proto::physical_plan::PhysicalPlanNodeExt;` to continue calling them with the same syntax. (Two examples in this repo updated.)
   - `impl From<...> for protobuf::X` / `impl From<&protobuf::X> for ...` between proto types and types from `datafusion-common` / `datafusion-expr` / `datafusion-datasource` are replaced by `FromProto`/`TryFromProto` traits in `datafusion_proto::convert`. Callers replace `Y::from(...)` / `(&x).into()` with `Y::from_proto(...)`.
   
   The new `proto` feature on `datafusion-physical-expr-common` and `datafusion-physical-expr` is **off by default**; crates that don't serialize plans pay nothing. `datafusion-proto` flips it on.
   
   ## Notes for reviewers
   
   - Each commit is independently mergeable and could be split into its own PR — happy to do that if reviewers prefer. Kept stacked here for higher-level review of the design as a whole.
   - Only `Column` is migrated to the new pattern. Other built-ins (`Literal`, `BinaryExpr`, etc.) and `DynamicFilterPhysicalExpr` are intentionally left on the existing downcast/match path; their migration is purely additive in follow-up PRs and doesn't change wire format.
   - The decode side uses a single-method `decode(node)` API on the ctx, matching the encode side's `to_proto(&ctx)` shape — both built-in and third-party expressions will look the same to recurse into children. Third-parties still need `PhysicalExtensionCodec` for the registry / dispatch role; an inventory-style `register::<T>()` builder is feasible later without further public-API churn (the ctx hides the dispatch trait, so the implementation behind it can swap freely).
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #21929:
URL: https://github.com/apache/datafusion/pull/21929


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#issuecomment-4448010055

   @timsaucer wonder if you'd be willing to do an initial review on this change since we discussed it in the call yesterday?


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#issuecomment-4471390451

   Thanks so much @timsaucer !
   
   > LMK if you want me to take another look since you did another push since I reviewed
   
   It's the same content, I was just re-organizing the commits to make it easier to review 😄. I think it would be good for @alamb to take a look at this, especially since we can wait to ship it until after 54 has been released (so no big rush).
   
   > I found the description to have the answers I was looking for eventually, but the llm generated text had so much content I didn't want to read it all just to understand what is actually not a very complex refactor. This held me up more than anything. It would have been more helpful to have a more pithy description that got me right to the useful pieces.
   
   > I'm finding for my own PRs that I need to make a lot of edits to these generated descriptions because they're not always great at isolating what a reviewer really needs to understand vs cataloging all the work that was done.
   
   Sorry about this. I agree with you. I usually leave the LLM generated descriptions up for draft PRs or where they are "fine" and better than "I'm a lazy human so really didn't write much at all" but I do try to rewrite them where it would be helpful (e.g. incidentally I was *just* rewriting the one in https://github.com/apache/datafusion/pull/22300 by hand while you were reviewing this). Just bad timing / judgment on this one. I'll update it for the next reviewer and note the feedback.


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#discussion_r3255038319


##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -477,6 +477,95 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
     fn expression_id(&self) -> Option<u64> {
         None
     }
+
+    /// Serialize this expression to a [`PhysicalExprNode`] proto message.
+    ///
+    /// Returning `Ok(None)` means "this expression does not know how to
+    /// serialize itself"; the caller (typically `datafusion-proto`) will fall
+    /// back to its existing codec / extension paths. This matches today's
+    /// behavior for expressions that aren't built into `datafusion-proto`.
+    ///
+    /// Returning `Ok(Some(node))` means the expression has serialized itself
+    /// fully; the caller should not try any further fallback path.
+    ///
+    /// Returning `Err(_)` means a real serialization failure (e.g. the
+    /// expression knows it should serialize but a child failed).
+    ///
+    /// The motivating use case is letting expressions with private state
+    /// (e.g. `DynamicFilterPhysicalExpr`'s `RwLock`-protected inner fields)
+    /// reach into their own internals for `try_to_proto`/`try_from_proto`
+    /// without having to expose `pub` accessors to `datafusion-proto`. See
+    /// <https://github.com/apache/datafusion/issues/21835>.
+    ///
+    /// The `try_` prefix matches the fallible `try_from_proto` decode
+    /// constructors (and the `TryFromProto` trait in `datafusion-proto`);
+    /// both sides of the round-trip are fallible and named consistently.
+    ///
+    /// [`PhysicalExprNode`]: datafusion_proto_models::protobuf::PhysicalExprNode
+    #[cfg(feature = "proto")]
+    fn try_to_proto(
+        &self,
+        _ctx: &proto_encode::PhysicalExprEncodeCtx<'_>,
+    ) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {

Review Comment:
   @timsaucer do you think we should make this `Result<...>` instead of `Result<Option<...>>`? If we ever plan to remove the default impl, remove the codecs and force ourselves to implement this method on all expressions we produce then the default returning an error would make sense IMO. The price we pay is that in the meantime it would be hard to differentiate between "I called an expression with an implementation and it error, I should bubble that up to users so they can fix it instead of getting an `unknown expression Column` type error (by falling back to the match -> codec, etc.)" vs. "this expression hasn't been updated yet".



-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#discussion_r3281704218


##########
datafusion/physical-expr-common/src/physical_expr.rs:
##########
@@ -477,6 +477,95 @@ pub trait PhysicalExpr: Any + Send + Sync + Display + Debug + DynEq + DynHash {
     fn expression_id(&self) -> Option<u64> {
         None
     }
+
+    /// Serialize this expression to a [`PhysicalExprNode`] proto message.
+    ///
+    /// Returning `Ok(None)` means "this expression does not know how to
+    /// serialize itself"; the caller (typically `datafusion-proto`) will fall
+    /// back to its existing codec / extension paths. This matches today's
+    /// behavior for expressions that aren't built into `datafusion-proto`.
+    ///
+    /// Returning `Ok(Some(node))` means the expression has serialized itself
+    /// fully; the caller should not try any further fallback path.
+    ///
+    /// Returning `Err(_)` means a real serialization failure (e.g. the
+    /// expression knows it should serialize but a child failed).
+    ///
+    /// The motivating use case is letting expressions with private state
+    /// (e.g. `DynamicFilterPhysicalExpr`'s `RwLock`-protected inner fields)
+    /// reach into their own internals for `try_to_proto`/`try_from_proto`
+    /// without having to expose `pub` accessors to `datafusion-proto`. See
+    /// <https://github.com/apache/datafusion/issues/21835>.
+    ///
+    /// The `try_` prefix matches the fallible `try_from_proto` decode
+    /// constructors (and the `TryFromProto` trait in `datafusion-proto`);
+    /// both sides of the round-trip are fallible and named consistently.
+    ///
+    /// [`PhysicalExprNode`]: datafusion_proto_models::protobuf::PhysicalExprNode
+    #[cfg(feature = "proto")]
+    fn try_to_proto(
+        &self,
+        _ctx: &proto_encode::PhysicalExprEncodeCtx<'_>,
+    ) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {

Review Comment:
   I think what we have is fine. We can iterate on the API as we implement more expressions.



-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#issuecomment-4471363128

   LMK if you want me to take another look since you did another push since I reviewed


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on code in PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#discussion_r3254905345


##########
datafusion/proto/src/logical_plan/file_formats.rs:
##########
@@ -79,8 +80,8 @@ impl CsvOptionsProto {
     }
 }
 
-impl From<&CsvOptionsProto> for CsvOptions {
-    fn from(proto: &CsvOptionsProto) -> Self {

Review Comment:
   Same argument holds for json options, parquet, etc



##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -152,19 +154,25 @@ impl From<&protobuf::StringifiedPlan> for StringifiedPlan {
     }
 }
 
-impl TryFrom<protobuf::WindowFrame> for WindowFrame {

Review Comment:
   Ok, so I spent way more time than I wanted to understand the problem with the dependencies and why you had to introduce this.



##########
datafusion/physical-expr/src/expressions/binary.rs:
##########
@@ -610,6 +610,124 @@ impl PhysicalExpr for BinaryExpr {
         write!(f, " {} ", self.op)?;
         write_child(f, self.right.as_ref(), precedence)
     }
+
+    #[cfg(feature = "proto")]
+    fn try_to_proto(
+        &self,
+        ctx: &datafusion_physical_expr_common::physical_expr::proto_encode::PhysicalExprEncodeCtx<'_>,
+    ) -> Result<Option<datafusion_proto_models::protobuf::PhysicalExprNode>> {
+        use datafusion_proto_models::protobuf;
+
+        // Linearize a nested binary expression tree of the same operator
+        // into a flat vector of operands to avoid deep recursion in proto.
+        let op = self.op;
+        let mut operand_refs: Vec<&Arc<dyn PhysicalExpr>> = vec![&self.right];
+        let mut current_expr: &BinaryExpr = self;
+        loop {
+            match current_expr.left.downcast_ref::<BinaryExpr>() {
+                Some(bin) if bin.op == op => {
+                    operand_refs.push(&bin.right);
+                    current_expr = bin;
+                }
+                _ => {
+                    operand_refs.push(&current_expr.left);
+                    break;
+                }
+            }
+        }
+        // Reverse so operands are ordered from left innermost to right outermost.
+        operand_refs.reverse();
+
+        let operands = operand_refs
+            .iter()
+            .map(|e| ctx.encode_child(e))
+            .collect::<Result<Vec<_>>>()?;
+
+        Ok(Some(protobuf::PhysicalExprNode {
+            expr_id: None,
+            expr_type: Some(protobuf::physical_expr_node::ExprType::BinaryExpr(
+                Box::new(protobuf::PhysicalBinaryExprNode {
+                    l: None,
+                    r: None,
+                    op: format!("{op:?}"),
+                    operands,
+                }),
+            )),
+        }))
+    }
+}
+
+#[cfg(feature = "proto")]
+impl BinaryExpr {
+    /// Reconstruct a [`BinaryExpr`] (or a left-deep tree of them when the proto
+    /// uses the linearized `operands` form) from its protobuf representation.
+    ///
+    /// Takes the whole [`PhysicalExprNode`] — the exact inverse of what
+    /// [`PhysicalExpr::try_to_proto`] produces — so every expression's
+    /// `try_from_proto` shares one signature. The operator string is parsed
+    /// via the canonical [`Operator::from_proto_name`] mapping, so no `op`
+    /// argument needs to be threaded in by the caller.
+    ///
+    /// [`PhysicalExprNode`]: datafusion_proto_models::protobuf::PhysicalExprNode
+    /// [`PhysicalExpr::try_to_proto`]: datafusion_physical_expr_common::physical_expr::PhysicalExpr::try_to_proto
+    /// [`PhysicalExprDecodeCtx::decode`]: datafusion_physical_expr_common::physical_expr::proto_decode::PhysicalExprDecodeCtx::decode

Review Comment:
   Seems like some generic text that doesn't necessarily need to be in each implementation



##########
datafusion/proto/src/logical_plan/file_formats.rs:
##########
@@ -79,8 +80,8 @@ impl CsvOptionsProto {
     }
 }
 
-impl From<&CsvOptionsProto> for CsvOptions {
-    fn from(proto: &CsvOptionsProto) -> Self {

Review Comment:
   On second thought, `from_factory` wasn't `pub` so maybe this argument is moot.



##########
datafusion/proto/src/logical_plan/file_formats.rs:
##########
@@ -79,8 +80,8 @@ impl CsvOptionsProto {
     }
 }
 
-impl From<&CsvOptionsProto> for CsvOptions {
-    fn from(proto: &CsvOptionsProto) -> Self {

Review Comment:
   For this and also `from_factory` probably needs a line in the upgrade doc



##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -152,19 +154,25 @@ impl From<&protobuf::StringifiedPlan> for StringifiedPlan {
     }
 }
 
-impl TryFrom<protobuf::WindowFrame> for WindowFrame {

Review Comment:
   TBH Since the methods are so similar it feels like keeping `impl TryFrom` is better.



-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#issuecomment-4471337801

   > I've started reviewing but this is a big one and it will take a bit
   
   I've tried to restack it to clean it up. But yes I know it's big, if you have any ideas for how to split it up or make it easier to review I'm all ears.


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#discussion_r3163306149


##########
.github/workflows/rust.yml:
##########
@@ -604,7 +601,6 @@ jobs:
           rust-version: stable

Review Comment:
   This seems sus



##########
datafusion/proto-models/LICENSE.txt:
##########
@@ -0,0 +1,212 @@
+

Review Comment:
   We could split this into 2 PRs or stack the commits so that the diff is not polluted by 800 lines of boilerplate



##########
datafusion/proto/src/logical_plan/file_formats.rs:
##########
@@ -79,8 +80,8 @@ impl CsvOptionsProto {
     }
 }
 
-impl From<&CsvOptionsProto> for CsvOptions {
-    fn from(proto: &CsvOptionsProto) -> Self {
+impl FromProto<&CsvOptionsProto> for CsvOptions {

Review Comment:
   I want to double check this change. It doesn't seem obviously linked to the other changes.



-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "timsaucer (via GitHub)" <gi...@apache.org>.
timsaucer commented on PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#issuecomment-4471034585

   I've started reviewing but this is a big one and it will take a bit


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] Split proto serialization to encapsulate private state (#21835) [datafusion]

Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #21929:
URL: https://github.com/apache/datafusion/pull/21929#issuecomment-4508986841

   Tracking issue for implementing the remaining physical expressions: https://github.com/apache/datafusion/issues/22418.
   
   Issue for doing the same thing for `ExecutionPlan`: 


-- 
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@datafusion.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org