yyy1000 commented on code in PR #9395:
URL: https://github.com/apache/arrow-datafusion/pull/9395#discussion_r1506916545
##########
datafusion/proto/src/bytes/mod.rs:
##########
@@ -87,8 +88,8 @@ pub trait Serializeable: Sized {
impl Serializeable for Expr {
fn to_bytes(&self) -> Result<Bytes> {
let mut buffer = BytesMut::new();
- let protobuf: protobuf::LogicalExprNode = self
- .try_into()
+ let extension_codec = DefaultLogicalExtensionCodec {};
+ let protobuf: protobuf::LogicalExprNode = serialize_expr(self,
&extension_codec)
Review Comment:
Would this be OK?
I think `DefaultLogicalExtensionCodec` may need to implement some functions
it didn't implement yet. I will also look into it.
##########
datafusion/proto/src/bytes/mod.rs:
##########
@@ -177,7 +178,8 @@ impl Serializeable for Expr {
let protobuf = protobuf::LogicalExprNode::decode(bytes)
.map_err(|e| plan_datafusion_err!("Error decoding expr as
protobuf: {e}"))?;
- logical_plan::from_proto::parse_expr(&protobuf, registry)
+ let extension_codec = DefaultLogicalExtensionCodec {};
+ logical_plan::from_proto::parse_expr(&protobuf, registry,
&extension_codec)
Review Comment:
And also here
##########
datafusion/proto/src/logical_plan/from_proto.rs:
##########
@@ -999,7 +1002,7 @@ pub fn parse_expr(
let operands = binary_expr
.operands
.iter()
- .map(|expr| parse_expr(expr, registry))
+ .map(|expr| parse_expr(expr, registry, codec))
Review Comment:
In from_proto.rs file, a lot of places need to change like this line.
I haven't finish it but submit this draft PR to verify the road.
##########
datafusion/proto/src/physical_plan/mod.rs:
##########
@@ -1911,6 +1912,30 @@ pub trait PhysicalExtensionCodec: Debug + Send + Sync {
) -> Result<Arc<dyn ExecutionPlan>>;
fn try_encode(&self, node: Arc<dyn ExecutionPlan>, buf: &mut Vec<u8>) ->
Result<()>;
+
+ // fn try_decode_udf(
+ // &self,
+ // name: &str,
+ // buf: &[u8],
+ // ) -> Result<Arc<ScalarUDF>>;
+
+ // fn try_encode_udf(&self, node: &ScalarUDF, buf: &mut Vec<u8>) ->
Result<()>;
+
+ // fn try_decode_udaf(
+ // &self,
+ // name: &str,
+ // buf: &[u8],
+ // ) -> Result<Arc<AggregateUDF>>;
+
+ // fn try_encode_udaf(&self, node: &AggregateUDF, buf: &mut Vec<u8>) ->
Result<()>;
+
+ // fn try_decode_udwf(
+ // &self,
+ // name: &str,
+ // buf: &[u8],
+ // ) -> Result<Arc<WindowUDF>>;
+
+ // fn try_encode_udwf(&self, node: &WindowUDF, buf: &mut Vec<u8>) ->
Result<()>;
Review Comment:
Plan to finish parts related to `PhysicalExtensionCodec` after
`LogicalExtensionCodec`
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -133,6 +136,10 @@ pub trait LogicalExtensionCodec: Debug + Send + Sync {
node: Arc<dyn TableProvider>,
buf: &mut Vec<u8>,
) -> Result<()>;
+
+ fn try_decode_udf(&self, name: &str, buf: &[u8]) -> Result<Arc<ScalarUDF>>;
+
+ fn try_encode_udf(&self, node: &ScalarUDF, buf: &mut Vec<u8>) ->
Result<()>;
Review Comment:
I plan to add aggregate_udf and window_udf after scalar_udf.
--
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]
