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: [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]
