Brijesh-Thakkar commented on code in PR #22705:
URL: https://github.com/apache/datafusion/pull/22705#discussion_r3390533147
##########
datafusion/ffi/src/udwf/mod.rs:
##########
@@ -74,17 +81,35 @@ pub struct FFI_WindowUDF {
display_name: SString,
) -> FFI_Result<WrappedSchema>,
- /// Performs type coercion. To simply this interface, all UDFs are treated
as having
- /// user defined signatures, which will in turn call coerce_types to be
called. This
- /// call should be transparent to most users as the internal function
performs the
- /// appropriate calls on the underlying [`WindowUDF`]
+ /// Pointer lifetime is tied to the inner Arc; null = None
+ pub documentation: unsafe extern "C" fn(udwf: &Self) -> *const
Documentation,
+
+ /// Returns expressions in the same order as input_exprs
+ pub expressions: unsafe extern "C" fn(
+ udwf: &Self,
+ args: FFI_ExpressionArgs,
+ ) -> SVec<FFI_PhysicalExpr>,
+
+ /// Serializes WindowFunction via DefaultLogicalExtensionCodec;
+ /// returns None variant if no simplification; only called when
has_simplify=true
Review Comment:
The FFI_WindowUDF struct was given a logical_codec field for exactly this
purpose. I'll fix ForeignWindowUDF::simplify() to use self.udf.logical_codec
instead of constructing a DefaultLogicalExtensionCodec inline.
##########
datafusion/ffi/src/udwf/mod.rs:
##########
@@ -74,17 +81,35 @@ pub struct FFI_WindowUDF {
display_name: SString,
) -> FFI_Result<WrappedSchema>,
- /// Performs type coercion. To simply this interface, all UDFs are treated
as having
- /// user defined signatures, which will in turn call coerce_types to be
called. This
- /// call should be transparent to most users as the internal function
performs the
- /// appropriate calls on the underlying [`WindowUDF`]
+ /// Pointer lifetime is tied to the inner Arc; null = None
+ pub documentation: unsafe extern "C" fn(udwf: &Self) -> *const
Documentation,
Review Comment:
The pointer's lifetime is valid as long as the FFI_WindowUDF's private_data
Arc is alive, which ForeignWindowUDF maintains via self.udf. However, I
understand the concern — I'll address it per Copilot's suggestion.
--
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]
