Add ExecutionPlan try_to_proto / try_from_proto hooks + ProjectionExec reference#23495
Open
adriangb wants to merge 2 commits into
Open
Add ExecutionPlan try_to_proto / try_from_proto hooks + ProjectionExec reference#23495adriangb wants to merge 2 commits into
adriangb wants to merge 2 commits into
Conversation
…nExec reference (apache#22419) General part: ExecutionPlanEncodeCtx/DecodeCtx + internal dispatch traits in datafusion-physical-plan (mirrors the PhysicalExpr pattern), the try_to_proto trait hook, expect_plan_variant! macro, and the ConverterPlanEncoder/Decoder adapters wiring the hook into the central dispatch in datafusion-proto. ProjectionExec migrated as the reference: encode downcast arm deleted, decode arm reduced to a one-liner delegating to ProjectionExec::try_from_proto. All 189 proto_integration roundtrip tests pass (incl. TPC-H). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfBPC2yYCwHUn4V8pQCVj3
…roto ctx (apache#22419) Adds encode_udf/udaf/udwf + decode_udf/udaf/udwf to ExecutionPlanEncodeCtx/ DecodeCtx (and the internal dispatch traits). These take datafusion-expr types + Vec<u8> and never name a proto type; the datafusion-proto adapter backs them over PhysicalExtensionCodec + the registry, owning the payload/registry lookup-order policy in one place. Wire-identical to the existing fun_definition semantics ((!buf.is_empty()).then_some(buf)). This lets function-carrying plans (Aggregate, window, ScalarSubquery) ride the polymorphic hook instead of staying as typed arms — possible because physical-plan sits above datafusion-expr (unlike the expr-side ctx). apache#23421's ScalarUdfCodec stays closed; ScalarFunctionExpr stays special-cased on the expr side where the crate cycle forces it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SfBPC2yYCwHUn4V8pQCVj3
This was referenced Jul 12, 2026
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which issue does this PR close?
Rationale for this change
This is the foundation for porting
ExecutionPlanserialization off the centraldowncast_refchain indatafusion/proto/src/physical_plan/mod.rs, mirroring what #21929 did forPhysicalExpr(tracked in #22418). Everything else in #23494 depends on this PR; it can't be parallelized because it defines the ctx API and wiring that every per-plan follow-up uses.What changes are included in this PR?
In
datafusion-physical-plan(feature-gated#[cfg(feature = "proto")]):ExecutionPlan::try_to_proto(&self, ctx: &ExecutionPlanEncodeCtx) -> Result<Option<PhysicalPlanNode>>— a new trait method defaulting toOk(None)(so un-migrated plans are unchanged).ExecutionPlanEncodeCtx/ExecutionPlanDecodeCtxplus the internalExecutionPlanEncode/ExecutionPlanDecodedispatch traits (defined here, implemented indatafusion-proto) — the same dependency-inversion thePhysicalExprctx uses. The ctx exposesencode_child/decode_child(+_children),encode_expr/decode_expr, and typed bytes-onlyencode_udaf/decode_udaf(+ udf/udwf) for function-carrying plans. No proto type is ever named by physical-plan beyond the pure prost types indatafusion-proto-models.expect_plan_variant!macro mirroringexpect_expr_variant!.In
datafusion-proto:ConverterPlanEncoder/ConverterPlanDecoderimplement the dispatch traits over the existingPhysicalExtensionCodec+ converter (the function serde reuses today'sfun_definitionbyte semantics exactly). The encode dispatch calls the hook first (resolvingdowncast_delegate()beforehand so wrapper plans serialize as their delegate), then falls back to the existing downcast chain.ProjectionExecis migrated as the reference implementation: its old encode arm is deleted and its decode arm reduced toProjectionExec::try_from_proto, so a green roundtrip proves the hook is the only path.Are these changes tested?
Yes — the existing
proto_integrationroundtrip suite (including TPC-H) passes unchanged (189 tests), which exercises theProjectionExecmigration end-to-end.cargo clippy --all-targets -- -D warningsandcargo fmtare clean.Are there any user-facing changes?
Additive only in this PR (new trait method with a default, new pub ctx types). No wire-format change. The broader effort (#23494) will remove the pub-for-proto
PhysicalPlanNodeExtscaffolding as plans migrate — that is a documentedapi change, but this base PR does not remove any public API.