thinkharderdev commented on code in PR #9395:
URL: https://github.com/apache/arrow-datafusion/pull/9395#discussion_r1507418283
##########
datafusion/proto/src/logical_plan/mod.rs:
##########
@@ -168,6 +175,14 @@ impl LogicalExtensionCodec for
DefaultLogicalExtensionCodec {
) -> Result<()> {
not_impl_err!("LogicalExtensionCodec is not provided")
}
+
+ fn try_decode_udf(&self, name: &str, buf: &[u8]) -> Result<Arc<ScalarUDF>>
{
+ not_impl_err!("LogicalExtensionCodec is not provided")
+ }
+
+ fn try_encode_udf(&self, node: &ScalarUDF, buf: &mut Vec<u8>) ->
Result<()> {
+ not_impl_err!("LogicalExtensionCodec is not provided")
+ }
Review Comment:
These shouldn't be neccessary if we add the default impl in the trait
##########
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>>;
Review Comment:
```suggestion
fn try_decode_udf(&self, name: &str, _buf: &[u8]) ->
Result<Arc<ScalarUDF>> {
not_impl_err!("LogicalExtensionCodec is not provided for scalar
function {name}")
}
```
##########
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:
Here I would include default implementations for all the new methods so it
doesn't break backwards compatibility:
```
fn try_encode_udf(&self, _node: &ScalarUDF, _buf: &mut Vec<u8>) ->
Result<()> {
Ok(())
}
```
##########
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:
Yes I think this is fine. These extension methods can be used for vanilla
serialization and then if user needs custom stuff with a custom codec they can
use the lower-level apis
--
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]
