kosiew commented on code in PR #24492:
URL: https://github.com/apache/datafusion/pull/24492#discussion_r3885957959
##########
datafusion/ffi/src/session/mod.rs:
##########
@@ -117,12 +115,6 @@ pub(crate) struct FFI_SessionRef {
logical_plan_serialized: SVec<u8>,
) -> FFI_Result<SVec<u8>>,
- create_physical_plan:
Review Comment:
I think we need to keep this callback slot for ABI compatibility. Removing
it changes the `#[repr(C)] FFI_SessionRef` layout while the workspace is still
at `55.0.0`.
A separately compiled 55.x consumer would still interpret this old slot as
`create_physical_plan`, so it could read `create_physical_expr` as that
callback and every field after it would be shifted. That can result in function
pointers being called with the wrong signatures, which is UB.
Could we keep the callback field in the struct and have its wrapper return
the new `NotImplemented` error instead? The other option would be to treat this
as an explicitly versioned ABI break and add compatible-version gating.
##########
datafusion/ffi/src/session/mod.rs:
##########
@@ -1014,35 +974,33 @@ mod tests {
Ok(())
}
- /// `create_physical_plan` must serialize with the session's logical codec
on
- /// both sides of the boundary. A plan that scans a custom table provider
is
- /// unserializable without it.
#[tokio::test]
- async fn test_create_physical_plan_uses_logical_codec() -> Result<(),
DataFusionError>
- {
- let (ctx, task_ctx_provider) =
crate::util::tests::test_session_and_ctx();
-
- let schema = Arc::new(Schema::new(vec![Field::new("a",
DataType::Int32, true)]));
- let batch = record_batch!(("a", Int32, [1, 2, 3]))?;
- let table = MemTable::try_new(schema, vec![vec![batch]])?;
- ctx.register_table("test_table", Arc::new(table))?;
+ async fn test_foreign_session_rejects_create_physical_plan() {
Review Comment:
Could we consider moving this assertion into the cross-library query-planner
integration path, or add a small cross-library case for it? That would verify
that a foreign planner gets the expected `NotImplemented` result when it tries
direct session delegation, while the retained-planner path still works
correctly across `dlopen`.
--
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]
