ryux1 commented on code in PR #24973:
URL: https://github.com/apache/datafusion/pull/24973#discussion_r3944355279
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -2046,15 +2203,67 @@ impl PhysicalExtensionCodec for
ComposedPhysicalExtensionCodec {
}
fn try_encode_udf(&self, node: &ScalarUDF, buf: &mut Vec<u8>) ->
Result<()> {
- self.encode_protobuf(buf, |codec, data| codec.try_encode_udf(node,
data))
+ self.encode_protobuf_by_name_aware(buf, |codec, data| {
+ codec.try_encode_udf(node, data)
+ })
+ }
+
+ fn try_decode_higher_order_function(
+ &self,
+ name: &str,
+ buf: &[u8],
+ ) -> Result<Arc<HigherOrderUDF>> {
+ self.decode_protobuf(buf, |codec, data| {
+ codec.try_decode_higher_order_function(name, data)
+ })
+ }
+
+ fn try_encode_higher_order_function(
+ &self,
+ node: &HigherOrderUDF,
+ buf: &mut Vec<u8>,
+ ) -> Result<()> {
+ self.encode_protobuf_by_name_aware(buf, |codec, data| {
+ codec.try_encode_higher_order_function(node, data)
+ })
+ }
+
+ fn try_decode_expr(
+ &self,
+ buf: &[u8],
+ inputs: &[Arc<dyn PhysicalExpr>],
+ ctx: &PhysicalExprDecodeCtx<'_>,
+ ) -> Result<Arc<dyn PhysicalExpr>> {
+ self.decode_protobuf(buf, |codec, data| codec.try_decode_expr(data,
inputs, ctx))
+ }
+
+ fn try_encode_expr(
+ &self,
+ node: &Arc<dyn PhysicalExpr>,
+ buf: &mut Vec<u8>,
+ ctx: &PhysicalExprEncodeCtx<'_>,
+ ) -> Result<()> {
+ self.encode_protobuf(buf, |codec, data| codec.try_encode_expr(node,
data, ctx))
Review Comment:
This newly delegates expression encoding through `encode_protobuf`, but that
helper reuses `data` without clearing it between codecs (the third defect
documented in #24830). If codec 0 writes bytes and then returns `Err`, codec 1
appends its valid payload and returns `Ok`; the composed tuple attributes the
concatenated, corrupted blob to codec 1. Before this PR expressions were not
delegated at all, so the new capability is incorrect for a valid failure
pattern. Could `encode_protobuf` make each attempt buffer-local (or clear
`data` before every attempt), with a regression codec that writes then rejects
followed by one that succeeds? The new by-name helper already clears per
iteration, so the same invariant should apply here.
--
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]
