(datafusion) 01/01: refactor: mark the ExecutionPlan proto dispatch traits as non-public API (#24001)

[gi...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-24001-3aef16c74084d56b19b7295e89108f46da5ad826
in repository https://gitbox.apache.org/repos/asf/datafusion.git

commit 541caab85a265c104888f1988fe54b08fa022f53
Author: Adrian Garcia Badaracco <17...@users.noreply.github.com>
AuthorDate: Thu Jul 30 10:34:26 2026 -0500

    refactor: mark the ExecutionPlan proto dispatch traits as non-public API (#24001)
    
    ## Which issue does this PR close?
    
    - Part of #23494. Precursor to #23915 (and to the remaining `DataSource`
    / `DataSink` items, #23497 / #23498).
    
    ## Rationale for this change
    
    `ExecutionPlanEncode` / `ExecutionPlanDecode` (added in #23495) are
    dispatch details of the
    `try_to_proto` / `try_from_proto` hooks, not extension points. They are
    defined in
    `datafusion-physical-plan`, implemented only by `ConverterPlanEncoder` /
    `ConverterPlanDecoder`
    in `datafusion-proto`, and plans reach them exclusively through
    `ExecutionPlanEncodeCtx` /
    `ExecutionPlanDecodeCtx` — their own docs already say so:
    
    > Internal dispatch trait backing [`ExecutionPlanEncodeCtx`].
    Implemented by `datafusion-proto`.
    > Plan authors never name this trait.
    
    They are `pub` only because those adapters live in another crate.
    Nothing said that to the tooling,
    so every capability the epic still has to add to the ctx reads as a
    major breaking change. #23915
    hits this first: it needs `decode_plan_with_scalar_subquery_results` so
    `ScalarSubqueryExec` can
    decode its input with the subquery-results container in scope, and
    `cargo-semver-checks` flags the
    required method as `trait_method_added`. The `DataSource` / `DataSink`
    families will want their own
    primitives next.
    
    `#[doc(hidden)]` states what was already true, and makes those additions
    changes to something that
    was never public API — rather than asking each follow-up PR to explain
    away a breakage report.
    
    ## What changes are included in this PR?
    
    Mark `ExecutionPlanEncode` and `ExecutionPlanDecode` `#[doc(hidden)]`,
    and say why in their docs
    and in the module overview. One file, no behavior, wire-format, or
    signature changes.
    
    A sealed-trait supertrait was the first thing I tried. It is worse here:
    sealing across a crate
    boundary needs a `pub` marker anyway, so it does not actually prevent a
    downstream impl — it just
    adds a public item in order to say "this is not public API", plus an
    `impl` line for every
    implementor including test doubles. `#[doc(hidden)]` says the same thing
    by removing API surface
    instead of adding it, and `cargo-semver-checks` honors both identically
    (measured below).
    
    ## Are these changes tested?
    
    The property this PR buys is a `cargo-semver-checks` classification, so
    it is verified with that
    tool directly — v0.49.0, the version CI installs, invoked the way CI
    invokes it:
    
    | baseline | change under test | result |
    |---|---|---|
    | `main` | add a required method to `ExecutionPlanDecode` |
    `trait_method_added` — **major** |
    | this PR | the same required method | 223 checks pass, **no semver
    update required** |
    
    Worth recording, since it drove the shape of this PR: the lint reads
    `public_api_sealed` from the
    **baseline**, so this has to land before the PRs that add methods, not
    alongside them. Also,
    `cargo-semver-checks` does not re-qualify the traits as public API even
    though
    `ExecutionPlanEncodeCtx::new` still names them in a public signature.
    
    Also ran, on the pinned 1.97.0 toolchain: `cargo fmt --all -- --check`,
    `cargo clippy --all-targets
    --all-features` for both crates, `cargo doc` with `-D warnings` (no
    broken intra-doc links from the
    module docs to the now-hidden traits), and the full `datafusion-proto`
    integration suite
    (211 passed).
    
    ## Are there any user-facing changes?
    
    `cargo-semver-checks` will report `trait_now_doc_hidden` on this PR, and
    that is the intended
    content of the change.
    
    Real-world impact is nil: both traits were introduced by #23495, which
    merged after `54.1.0` was
    tagged, so no released version of `datafusion-physical-plan` contains
    them. The load-bearing public
    API — `ExecutionPlan::try_to_proto`, the two ctx types,
    `AsExecutionPlan`, `PhysicalExtensionCodec`,
    `PhysicalProtoConverterExtension` — is untouched, and `datafusion-proto`
    needs no change at all.
    
    Note for contributors on #23494: the expression-side equivalents
    (`PhysicalExprEncode` /
    `PhysicalExprDecode` in `physical-expr-common`) have the same shape and
    the same argument, but
    several test doubles across crates implement them. Left for a follow-up.
    
    Co-authored-by: Claude Opus 5 <no...@anthropic.com>
---
 datafusion/physical-plan/src/proto.rs | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/datafusion/physical-plan/src/proto.rs b/datafusion/physical-plan/src/proto.rs
index 2c6636a392..f84cd67d46 100644
--- a/datafusion/physical-plan/src/proto.rs
+++ b/datafusion/physical-plan/src/proto.rs
@@ -33,7 +33,8 @@
 //! * [`ExecutionPlanEncode`] / [`ExecutionPlanDecode`] — internal dispatch
 //!   traits, *defined* here but *implemented* in `datafusion-proto`, that the
 //!   context types delegate to. This is the dependency inversion that keeps the
-//!   proto types flowing in one direction only.
+//!   proto types flowing in one direction only. They are `#[doc(hidden)]`: not
+//!   public API, `pub` only because their implementors live in another crate.
 //!
 //! `datafusion-physical-plan` depends on the pure prost types in
 //! `datafusion-proto-models` (feature `proto`), never on `datafusion-proto`.
@@ -73,6 +74,11 @@ use crate::ExecutionPlan;
 ///
 /// Implemented by `datafusion-proto`. Plan authors never name this trait; they
 /// call methods on [`ExecutionPlanEncodeCtx`] instead.
+///
+/// **Not public API.** `pub` only because the implementors live in another
+/// crate; `#[doc(hidden)]` records that, so encoding primitives can be added
+/// here as the serialization hooks grow without breaking downstream code.
+#[doc(hidden)]
 pub trait ExecutionPlanEncode {
     /// Serialize a child execution plan (recursing through the central
     /// serializer, so the child's own `try_to_proto` hook is honored).
@@ -98,6 +104,11 @@ pub trait ExecutionPlanEncode {
 ///
 /// Implemented by `datafusion-proto`. Plan authors never name this trait; they
 /// call methods on [`ExecutionPlanDecodeCtx`] instead.
+///
+/// **Not public API.** `pub` only because the implementors live in another
+/// crate; `#[doc(hidden)]` records that, so decoding primitives can be added
+/// here as the serialization hooks grow without breaking downstream code.
+#[doc(hidden)]
 pub trait ExecutionPlanDecode {
     /// Deserialize a child execution plan (recursing through the central
     /// deserializer, so the child's own `try_from_proto` is honored).


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@datafusion.apache.org
For additional commands, e-mail: commits-help@datafusion.apache.org