adriangb commented on code in PR #24631:
URL: https://github.com/apache/datafusion/pull/24631#discussion_r3846331513
##########
datafusion/physical-plan/src/proto/mod.rs:
##########
@@ -155,6 +176,12 @@ pub trait ExecutionPlanDecode {
fn decode_udwf(&self, name: &str, payload: Option<&[u8]>) ->
Result<Arc<WindowUDF>>;
}
+/// The wire parts of an extension plan: its opaque payload, borrowed from the
+/// node it was read from, and its already-decoded children.
+///
+/// Returned by [`ExecutionPlanDecodeCtx::decode_extension`].
+pub type ExtensionPlanParts<'n> = (&'n [u8], Vec<Arc<dyn ExecutionPlan>>);
Review Comment:
If we're going to make a public type lets make it a struct with named fields.
##########
datafusion/physical-plan/src/proto/registry.rs:
##########
@@ -0,0 +1,454 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+//! Name-keyed decode registry for extension [`ExecutionPlan`]s.
+//!
+//! Built-in plans are dispatched by their `PhysicalPlanType` oneof variant, so
+//! the wire format names them. Extension plans all share the single
+//! `PhysicalExtensionNode` variant, which historically carried no
+//! discriminator: the `PhysicalExtensionCodec` *was* the discriminator, and
+//! `ComposedPhysicalExtensionCodec` had to try each registered codec in
+//! sequence and read a decode error as "not mine".
+//!
+//! This module supplies the missing discriminator. An extension plan declares
a
+//! globally unique name via [`ExtensionExecutionPlan::PLAN_NAME`], stamps it
on
+//! the wire with
+//!
[`ExecutionPlanEncodeCtx::encode_extension`](super::ExecutionPlanEncodeCtx::encode_extension),
+//! and registers its decoder on the session with
+//! [`ExecutionPlanRegistryExt::register_execution_plan`]. Decoding then
selects
+//! the decoder by name instead of by trial and error.
+//!
+//! The registry is *session scoped*, matching the `FunctionRegistry`
+//! precedent: it lives in the [`SessionConfig`] extension map, so it travels
+//! with the session into every [`TaskContext`] without any new plumbing, stays
+//! testable, and stays multi-tenant safe.
+//!
+//! Registration is per plan type and entirely opt-in. A
`PhysicalExtensionNode`
+//! with no name — or with a name no decoder claims — falls back to the
existing
+//! `PhysicalExtensionCodec` chain, unchanged.
+//!
+//! [`TaskContext`]: datafusion_execution::TaskContext
+
+use std::any::{TypeId, type_name};
+use std::collections::HashMap;
+use std::collections::hash_map::Entry;
+use std::sync::Arc;
+
+use datafusion_common::{Result, config_err};
+use datafusion_execution::config::SessionConfig;
+use datafusion_proto_models::protobuf::PhysicalPlanNode;
+
+use crate::ExecutionPlan;
+use crate::proto::ExecutionPlanDecodeCtx;
+
+/// The decode half of an extension plan's serialization, as a plain function
+/// pointer.
+///
+/// This is exactly the signature every migrated plan's `try_from_proto`
+/// associated function already has. `try_from_proto` is an inherent function
+/// rather than a trait method precisely so it can return
+/// `Arc<dyn ExecutionPlan>` and be stored like this.
+pub type ExecutionPlanDecoder =
+ fn(&PhysicalPlanNode, &ExecutionPlanDecodeCtx<'_>) -> Result<Arc<dyn
ExecutionPlan>>;
Review Comment:
I don't recall why we went with free-standing functions, the justification
here is that if it was on the trait it would not be able to return `Arc<dyn
ExecutionPlan>` but I'm not sure if that's right. Having this type requirement
seems a bit unfortunate, it's like a trait w/ a single method being defined as
a type. We should double check if there's a refactor that would make this nicer
before we double down on the current status quo.
--
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]
