Copilot commented on code in PR #22922:
URL: https://github.com/apache/datafusion/pull/22922#discussion_r3401152818


##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -3940,18 +3940,36 @@ pub trait PhysicalExtensionCodec: Debug + Send + Sync + 
Any {
         Ok(())
     }
 
+    /// Decode a custom extension expression from `buf`.
+    ///
+    /// Implementations whose proto carries nested `PhysicalExprNode` fields
+    /// should route those through `proto_converter.proto_to_physical_expr`
+    /// (rather than the free `parse_physical_expr` function) so that an
+    /// active `DeduplicatingDeserializer` can cache-hit on matching
+    /// `expr_id`s and re-share `Arc<dyn PhysicalExpr>` (e.g. a
+    /// `DynamicFilterPhysicalExpr` referenced both from a SortExec.filter
+    /// and from a custom file-source's predicate field).
     fn try_decode_expr(
         &self,
         _buf: &[u8],
         _inputs: &[Arc<dyn PhysicalExpr>],
+        _proto_converter: &dyn PhysicalProtoConverterExtension,
     ) -> Result<Arc<dyn PhysicalExpr>> {
         not_impl_err!("PhysicalExtensionCodec is not provided")
     }
 
+    /// Encode a custom extension expression into `buf`.
+    ///
+    /// Implementations whose proto carries nested `PhysicalExprNode` fields
+    /// should route those through `proto_converter.physical_expr_to_proto`
+    /// (rather than the free `serialize_physical_expr` function) so that
+    /// an active `DeduplicatingSerializer` can stamp matching `expr_id`s
+    /// for shared inner expressions. See [`Self::try_decode_expr`].

Review Comment:
   The doc comment references an `active DeduplicatingSerializer`, but there is 
no such type in this crate (deduplication is implemented via 
`DeduplicatingProtoConverter` / `DeduplicatingDeserializer`). As written, this 
is misleading about where `expr_id` stamping and dedup behavior comes from.



-- 
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]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to