kosiew commented on code in PR #25318:
URL: https://github.com/apache/datafusion/pull/25318#discussion_r4164308669
##########
datafusion/ffi/src/proto/physical_extension_codec.rs:
##########
@@ -148,10 +148,13 @@ unsafe extern "C" fn try_decode_fn_wrapper(
.collect::<Result<Vec<_>>>();
let inputs = sresult_return!(inputs);
- let plan = sresult_return!(codec.try_decode(
+ // The caller's decode context cannot cross the FFI boundary, so decode
+ // with a root context for this side's codec.
+ let decode_ctx = PhysicalPlanDecodeContext::new(task_ctx.as_ref(),
codec.as_ref());
Review Comment:
The FFI path still drops the active `ScalarSubqueryResults` scope, so a
foreign codec decoding an embedded `ScalarSubqueryExpr` can still hit the
original error. Please preserve that scope through an ABI-safe, versioned FFI
decode path, without passing `PhysicalPlanDecodeContext` itself, and add a
forced-foreign end-to-end `ScalarSubqueryExec` roundtrip test.
##########
datafusion/ffi/src/proto/physical_extension_codec.rs:
##########
@@ -643,6 +646,64 @@ pub(crate) mod tests {
Ok(())
}
+ /// A codec whose `try_decode` fails and that only decodes through
+ /// `try_decode_with_ctx`.
+ #[derive(Debug)]
+ struct ContextOnlyCodec;
+
+ impl PhysicalExtensionCodec for ContextOnlyCodec {
+ fn try_decode(
+ &self,
+ _buf: &[u8],
+ _inputs: &[Arc<dyn ExecutionPlan>],
+ _ctx: &TaskContext,
+ _proto_converter: &dyn PhysicalProtoConverterExtension,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ exec_err!("ContextOnlyCodec decodes through try_decode_with_ctx")
+ }
+
+ fn try_decode_with_ctx(
+ &self,
+ _buf: &[u8],
+ _inputs: &[Arc<dyn ExecutionPlan>],
+ _ctx: &PhysicalPlanDecodeContext<'_>,
+ _proto_converter: &dyn PhysicalProtoConverterExtension,
+ ) -> Result<Arc<dyn ExecutionPlan>> {
+ Ok(create_test_exec())
+ }
+
+ fn try_encode(
+ &self,
+ _node: Arc<dyn ExecutionPlan>,
+ _buf: &mut Vec<u8>,
+ _proto_converter: &dyn PhysicalProtoConverterExtension,
+ ) -> Result<()> {
+ Ok(())
+ }
+ }
+
+ #[test]
+ fn ffi_physical_extension_codec_decodes_through_try_decode_with_ctx() ->
Result<()> {
+ let codec = Arc::new(ContextOnlyCodec);
+ let (ctx, task_ctx_provider) =
crate::util::tests::test_session_and_ctx();
+
+ let mut ffi_codec =
+ FFI_PhysicalExtensionCodec::new(codec, None, task_ctx_provider);
+ ffi_codec.library_marker_id = crate::mock_foreign_marker_id;
+ let foreign_codec: Arc<dyn PhysicalExtensionCodec> =
(&ffi_codec).into();
+
+ let returned_exec = foreign_codec.try_decode(
Review Comment:
Could you clarify the test name or comment to show that it intentionally
calls the foreign adapter's legacy `try_decode`, which then reaches the
producer codec's `try_decode_with_ctx` override? For example,
`ffi_physical_extension_codec_legacy_decode_uses_context_aware_default` would
make that behavior clearer.
--
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]
