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/08/07 15:44:58 UTC
[PR] chore(proto): deprecate accessors that only existed for proto serialization [datafusion]
adriangb opened a new pull request, #24168:
URL: https://github.com/apache/datafusion/pull/24168
> [!IMPORTANT]
> **Depends on #24166.** This branch is stacked on top of `refactor/proto-destructure-agg-window`, so the diff shown here includes that PR's commits. **Please review only the final commit** (`chore(proto): deprecate accessors that only existed for proto serialization`) until #24166 merges, at which point this diff will reduce to that commit alone.
>
> The dependency is load-bearing, not just ordering: #24166 is what rewrites the `try_to_proto` hooks in `analyze.rs`, `unnest.rs` and `async_func.rs` to read struct fields directly. Only after that change do these accessors have zero callers, which is what makes deprecating them sound.
## Which issue does this PR close?
- Follow-up cleanup for the proto hook migration EPIC #23494. Does not close it.
## Rationale for this change
Five public accessor methods on physical plan nodes exist for one reason only: an older protobuf serialization PR needed to reach a private struct field from outside the plan's own module. Each was introduced by the serialization PR that needed it, and none was ever part of an API anyone asked for.
Now that every one of these plans serializes itself through its own `try_to_proto` hook — which lives in the same module and can read the fields directly — the accessors have no callers at all. Keeping them means keeping public API surface that exists purely as a historical artifact of how serialization used to be wired.
| Method | Introduced by | Status after #24166 |
| --- | --- | --- |
| `AnalyzeExec::verbose` | "Implement protobuf serialization for AnalyzeExec" (#7574) | no callers |
| `AnalyzeExec::show_statistics` | "Implement protobuf serialization for AnalyzeExec" (#7574) | no callers |
| `UnnestExec::list_column_indices` | "Support encoding and decoding UnnestExec" (#12344) | no callers |
| `UnnestExec::struct_column_indices` | "Support encoding and decoding UnnestExec" (#12344) | no callers |
| `AsyncFuncExec::async_exprs` | "[Proto]: Serialization support for `AsyncFuncExec`" (#19118) | no callers |
`AsyncFuncExec::async_exprs` already had zero callers before #24166; its `try_to_proto` hook was written against the field from the start.
Provenance was confirmed with `git log -S` against each accessor's definition, and the "no callers" claim was checked across the whole repository — including `datafusion-cli`, `benchmarks`, `datafusion-examples`, `datafusion/substrait`, docs and tests.
## What changes are included in this PR?
Adds `#[deprecated(since = "55.0.0", note = "...")]` to the five methods listed above. Nothing is removed, and no behavior changes.
The `note` is honest that there is no replacement: the plan's own serde hook reads the field directly, so there is nothing to point users at. It follows the existing phrasing used by the deprecated shims in `datafusion/proto/src/physical_plan/mod.rs` ("unused by DataFusion; ...") combined with the repo's established no-replacement idiom ("please open an issue if you have a use case for it").
Two small notes on the mechanics:
- The `roundtrip_analyze` test in `datafusion/proto/tests/cases/roundtrip_physical_plan.rs` legitimately calls two of these accessors to assert the round-tripped plan preserved those fields. Rather than delete the assertions, the test function carries a scoped `#[expect(deprecated)]` with a one-line comment. `#[expect]` rather than `#[allow]` because the workspace enables `clippy::allow_attributes`, so an `#[allow]` would itself fail under `-D warnings`. Using `#[expect]` also means the attribute is self-policing — if these call sites ever go away, the unfulfilled expectation becomes an error.
- `UnnestExec::list_column_indices` is referenced by a rustdoc intra-doc link elsewhere in `unnest.rs`. Because this deprecates rather than removes, that link stays valid and was deliberately left untouched; `cargo doc -p datafusion-physical-plan --no-deps` completes with no warnings.
## Are these changes tested?
There is no new behavior to test — the real verification is that the compiler agrees these methods are unused. Since `deprecated` is a warning and CI builds with `-D warnings`, a clean lint over the whole workspace *is* the proof that no internal caller remains.
Run locally on this branch:
- `cargo fmt --all`
- `cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings` (CI's exact invocation) — clean across every crate, including `datafusion-cli`, `benchmarks`, `datafusion-examples` and `substrait`
- `cargo test -p datafusion-proto --test proto_integration` — 214 passed, 0 failed
- `cargo test -p datafusion-physical-plan` — 1640 + 9 passed, 0 failed
- `cargo doc -p datafusion-physical-plan --no-deps` — no warnings
## Are there any user-facing changes?
Yes, and the `api change` label applies.
Downstream users who call any of these five methods will now see a deprecation warning. Nothing breaks in this release — the methods still work exactly as before. Removal follows the normal deprecation window described in the [API health policy](https://github.com/apache/datafusion/blob/main/docs/source/contributor-guide/api-health.md) (six major versions or six months, whichever is longer), consistent with the plan in EPIC #23494.
There is intentionally no replacement API. If you have a use case for reading these fields from outside the plan, please open an issue — that is a real API request worth designing deliberately, rather than something to leave standing by accident.
--
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] chore(proto): deprecate accessors that only existed for proto serialization [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #24168:
URL: https://github.com/apache/datafusion/pull/24168#issuecomment-5219391559
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/24168?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `82.87671%` with `25 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 81.04%. Comparing base ([`e2e8e1c`](https://app.codecov.io/gh/apache/datafusion/commit/e2e8e1c88cbee9c373dab52b9ef6541d2ee6364f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`183de51`](https://app.codecov.io/gh/apache/datafusion/commit/183de516d91aa8423eaef67b093609106f0c7aea?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/24168?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-plan/src/analyze.rs](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fanalyze.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hbmFseXplLnJz) | 71.87% | [4 Missing and 5 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/aggregates/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Faggregates%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hZ2dyZWdhdGVzL21vZC5ycw==) | 88.67% | [0 Missing and 6 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/unnest.rs](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Funnest.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy91bm5lc3QucnM=) | 82.60% | [0 Missing and 4 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/physical-plan/src/async\_func.rs](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fasync_func.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9hc3luY19mdW5jLnJz) | 81.25% | [0 Missing and 3 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...usion/physical-plan/src/windows/window\_agg\_exec.rs](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fwindows%2Fwindow_agg_exec.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy93aW5kb3dzL3dpbmRvd19hZ2dfZXhlYy5ycw==) | 86.66% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ysical-plan/src/windows/bounded\_window\_agg\_exec.rs](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Fwindows%2Fbounded_window_agg_exec.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy93aW5kb3dzL2JvdW5kZWRfd2luZG93X2FnZ19leGVjLnJz) | 85.71% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/24168?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #24168 +/- ##
==========================================
- Coverage 81.05% 81.04% -0.01%
==========================================
Files 1107 1107
Lines 381407 381434 +27
Branches 381407 381434 +27
==========================================
+ Hits 309139 309143 +4
- Misses 54013 54031 +18
- Partials 18255 18260 +5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/24168?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] chore(proto): deprecate accessors that only existed for proto serialization [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #24168:
URL: https://github.com/apache/datafusion/pull/24168#issuecomment-5219158345
<!-- 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 [ 38.063s] (current)
Parsing datafusion-physical-plan v54.1.0 (current)
Parsed [ 0.140s] (current)
Building datafusion-physical-plan v54.1.0 (baseline)
Built [ 37.604s] (baseline)
Parsing datafusion-physical-plan v54.1.0 (baseline)
Parsed [ 0.141s] (baseline)
Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.593s] 223 checks: 222 pass, 1 fail, 0 warn, 30 skip
--- failure type_method_marked_deprecated: type method #[deprecated] added ---
Description:
A type method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.49.0/src/lints/type_method_marked_deprecated.ron
Failed in:
method datafusion_physical_plan::analyze::AnalyzeExec::verbose in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/analyze.rs:145
method datafusion_physical_plan::analyze::AnalyzeExec::show_statistics in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/analyze.rs:154
method datafusion_physical_plan::async_func::AsyncFuncExec::async_exprs in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/async_func.rs:113
method datafusion_physical_plan::unnest::UnnestExec::list_column_indices in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/unnest.rs:190
method datafusion_physical_plan::unnest::UnnestExec::struct_column_indices in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/unnest.rs:199
Summary semver requires new minor version: 0 major and 1 minor checks failed
Finished [ 77.885s] datafusion-physical-plan
Building datafusion-proto v54.1.0 (current)
Built [ 59.962s] (current)
Parsing datafusion-proto v54.1.0 (current)
Parsed [ 0.019s] (current)
Building datafusion-proto v54.1.0 (baseline)
Built [ 59.775s] (baseline)
Parsing datafusion-proto v54.1.0 (baseline)
Parsed [ 0.020s] (baseline)
Checking datafusion-proto v54.1.0 -> v54.1.0 (no change; assume patch)
Checked [ 0.230s] 223 checks: 223 pass, 30 skip
Summary no semver update required
Finished [ 121.148s] 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] chore(proto): deprecate `AsyncFuncExec::async_exprs`, which only existed for proto serialization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #24168:
URL: https://github.com/apache/datafusion/pull/24168#issuecomment-5231835073
Good catch, I un-deprecated those methods.
--
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] chore(proto): deprecate accessors that only existed for proto serialization [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on code in PR #24168:
URL: https://github.com/apache/datafusion/pull/24168#discussion_r3743945227
##########
datafusion/physical-plan/src/analyze.rs:
##########
@@ -143,6 +143,10 @@ impl AnalyzeExec {
}
/// Access to show_statistics
+ #[deprecated(
Review Comment:
will deprecating these affect users like
https://github.com/openobserve/openobserve/blob/575e8ea4d5fd4b0e630aa013a1a31b1da56708e9/src/search/src/datafusion/optimizer/physical_optimizer/distribute_analyze.rs#L31-L37
##########
datafusion/physical-plan/src/unnest.rs:
##########
@@ -183,11 +183,19 @@ impl UnnestExec {
}
/// Indices of the list-typed columns in the input schema
+ #[deprecated(
Review Comment:
same
https://github.com/goldsky-io/streamling/blob/8d85af926b034085aece04233722c508ac80f2d0/crates/streamling-core/src/operators/unnest.rs#L85-L99
--
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] chore(proto): deprecate accessors that only existed for proto serialization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on PR #24168:
URL: https://github.com/apache/datafusion/pull/24168#issuecomment-5231548556
@kumarUjjawal could you take a look please?
--
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] chore(proto): deprecate `AsyncFuncExec::async_exprs`, which only existed for proto serialization [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb merged PR #24168:
URL: https://github.com/apache/datafusion/pull/24168
--
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