You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/07/30 13:38:13 UTC
[PR] refactor: seal the ExecutionPlan proto dispatch traits [datafusion]
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: mark the ExecutionPlan proto dispatch traits as non-public API [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24001:
URL: https://github.com/apache/datafusion/pull/24001#issuecomment-5132192449
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/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>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion-physical-plan v54.1.0 (current)
Built [ 45.203s] (current)
Parsing datafusion-physical-plan v54.1.0 (current)
Parsed [ 0.143s] (current)
Building datafusion-physical-plan v54.1.0 (baseline)
Built [ 39.260s] (baseline)
Parsing datafusion-physical-plan v54.1.0 (baseline)
Parsed [ 0.144s] (baseline)
Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.625s] 223 checks: 221 pass, 2 fail, 0 warn, 30 skip
--- failure trait_added_supertrait: non-sealed trait added new supertraits ---
Description:
A non-sealed trait added one or more supertraits, which breaks downstream implementations of the trait
ref: https://doc.rust-lang.org/cargo/reference/semver.html#generic-bounds-tighten
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/trait_added_supertrait.ron
Failed in:
trait datafusion_physical_plan::proto::ExecutionPlanDecode gained SealedPlanDispatch in file /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/proto.rs:125
trait datafusion_physical_plan::proto::ExecutionPlanEncode gained SealedPlanDispatch in file /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/proto.rs:97
--- failure trait_newly_sealed: pub trait became sealed ---
Description:
A publicly-visible trait became sealed, so downstream crates are no longer able to implement it
ref: https://rust-lang.github.io/api-guidelines/future-proofing.html#sealed-traits-protect-against-downstream-implementations-c-sealed
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/trait_newly_sealed.ron
Failed in:
trait datafusion_physical_plan::proto::ExecutionPlanEncode in file /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/proto.rs:97
trait datafusion_physical_plan::proto::ExecutionPlanDecode in file /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/proto.rs:125
Summary semver requires new major version: 2 major and 0 minor checks failed
Finished [ 86.786s] datafusion-physical-plan
Building datafusion-proto v54.1.0 (current)
Built [ 62.387s] (current)
Parsing datafusion-proto v54.1.0 (current)
Parsed [ 0.019s] (current)
Building datafusion-proto v54.1.0 (baseline)
Built [ 62.378s] (baseline)
Parsing datafusion-proto v54.1.0 (baseline)
Parsed [ 0.019s] (baseline)
Checking datafusion-proto v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.244s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 126.303s] datafusion-proto
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: mark the ExecutionPlan proto dispatch traits as non-public API [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #24001:
URL: https://github.com/apache/datafusion/pull/24001
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: mark the ExecutionPlan proto dispatch traits as non-public API [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24001:
URL: https://github.com/apache/datafusion/pull/24001#issuecomment-5132949313
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24001?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 80.85%. Comparing base ([`2f25454`](https://app.codecov.io/gh/apache/datafusion/commit/2f25454ad0e58abd94f5f36cfc4e19854695b92a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e36e9b6`](https://app.codecov.io/gh/apache/datafusion/commit/e36e9b64f35352a302c8960fed2527ac61c46acf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 14 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24001 +/- ##
==========================================
+ Coverage 80.75% 80.85% +0.09%
==========================================
Files 1096 1096
Lines 373588 373911 +323
Branches 373588 373911 +323
==========================================
+ Hits 301687 302311 +624
+ Misses 53898 53557 -341
- Partials 18003 18043 +40
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24001?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] refactor: mark the ExecutionPlan proto dispatch traits as non-public API [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #24001:
URL: https://github.com/apache/datafusion/pull/24001#issuecomment-5132018145
@kumarUjjawal small change I found while working on our EPIC, please take a look when you can 🙏🏻
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org