adriangb opened a new pull request, #24001: URL: https://github.com/apache/datafusion/pull/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 module docs already say as much: > Internal dispatch trait backing [`ExecutionPlanEncodeCtx`]. Implemented by `datafusion-proto`. > Plan authors never name this trait. They were nonetheless publicly implementable, so every capability the epic still has to add to the ctx is 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. Sealing the traits records the intent and makes those additions minor changes under the [API health policy], instead of asking each follow-up PR to explain away a breakage report. [API health policy]: https://datafusion.apache.org/contributor-guide/api-health.html ## What changes are included in this PR? - Add `#[doc(hidden)] pub trait SealedPlanDispatch` in `datafusion-physical-plan`'s `proto` module and require it as a supertrait of `ExecutionPlanEncode` and `ExecutionPlanDecode`. - Implement it for the two adapters in `datafusion-proto`. It is `pub` because the only implementors live in another crate, and `#[doc(hidden)]` because it is not something to implement outside DataFusion — the combination is what `cargo-semver-checks` recognizes as *public API sealed*. No behavior, wire-format, or signature changes. ## Are these changes tested? Yes — 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, run as CI runs it): | baseline | change under test | result | |---|---|---| | `main` (unsealed) | add a required method to `ExecutionPlanDecode` | `trait_method_added` — **major** | | this PR (sealed) | the same required method | 223 checks pass, **no semver update required** | The `#[doc(hidden)]` supertrait is load-bearing: `cargo-semver-checks` has classified traits sealed this way as sealed since v0.40. Also ran, on the pinned 1.97.0 toolchain: `cargo fmt --all -- --check`, `cargo clippy --all-targets --all-features` for both crates (clean for the touched files), and the full `datafusion-proto` integration suite (211 passed). ## Are there any user-facing changes? `ExecutionPlanEncode` and `ExecutionPlanDecode` can no longer be implemented outside DataFusion. `cargo-semver-checks` will report `trait_newly_sealed` on this PR, and that is the intended content of the change — it is precisely why it lands on its own, ahead of the PRs that add methods. 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. Nothing downstream can have an implementation to break. The load-bearing public API — `ExecutionPlan::try_to_proto`, the two ctx types, `AsExecutionPlan`, `PhysicalExtensionCodec`, `PhysicalProtoConverterExtension` — is untouched. Two notes for contributors on #23494: - A test double that implements either trait (for example `UnusedPlanEncoder` in #23683) now also needs `impl SealedPlanDispatch for … {}`. One line; the marker is reachable. - The expression-side equivalents (`PhysicalExprEncode` / `PhysicalExprDecode` in `physical-expr-common`) have the same shape and the same argument for sealing, but several test doubles across crates implement them. Left for a follow-up. -- 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]
