Copilot commented on code in PR #24723:
URL: https://github.com/apache/datafusion/pull/24723#discussion_r3872998877
##########
datafusion/ffi/src/table_provider.rs:
##########
@@ -564,14 +564,26 @@ impl FFI_TableProvider {
)
}
+ /// Creates an [`FFI_TableProvider`] using a prebuilt FFI logical codec.
+ ///
+ /// If `provider` is already foreign, this re-exports its original FFI
+ /// handle rather than adding another wrapper layer. The handle still
adopts
+ /// the `logical_codec` supplied here, so it is never silently discarded
and
+ /// an imported provider can be rebound to a different session.
+ ///
+ /// `runtime` is only honored when a new wrapper is created. An
+ /// already-foreign handle keeps the runtime of the library that owns it,
+ /// because that value lives in private data this side cannot reach.
pub fn new_with_ffi_codec(
provider: Arc<dyn TableProvider>,
can_support_pushdown_filters: bool,
runtime: Option<Handle>,
logical_codec: FFI_LogicalExtensionCodec,
) -> Self {
if let Some(provider) =
provider.downcast_ref::<ForeignTableProvider>() {
- return provider.0.clone();
+ let mut provider = provider.0.clone();
+ provider.logical_codec = logical_codec;
Review Comment:
`new_with_ffi_codec` now correctly adopts `logical_codec` on the
already-foreign path, but it still appears to silently discard at least one
other provided argument (`can_support_pushdown_filters`) in that same path.
Since the function signature accepts that flag, callers may reasonably expect
it to apply even when re-exporting a foreign handle. Consider either
(mandatory) documenting that `can_support_pushdown_filters` is not honored for
already-foreign providers (similar to the `runtime` note), or (preferred, if
the field is part of the public `#[repr(C)]` handle like `logical_codec`)
overwriting the stored flag on the cloned handle as well to fully preserve
supplied arguments.
--
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]
