chore(proto): deprecate AsyncFuncExec::async_exprs, which only existed for proto serialization - #24168
Conversation
|
Thank you for opening this pull request! Reviewer note: 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #24168 +/- ##
==========================================
- Coverage 81.06% 81.04% -0.02%
==========================================
Files 1106 1106
Lines 381891 381896 +5
Branches 381891 381896 +5
==========================================
- Hits 309578 309519 -59
- Misses 54034 54095 +61
- Partials 18279 18282 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3ef75ee to
4e92ef8
Compare
…zation Four public accessors were added purely so that protobuf serialization could reach private plan fields: - AnalyzeExec::show_statistics (apache#7574) - UnnestExec::list_column_indices / UnnestExec::struct_column_indices (apache#12344) - AsyncFuncExec::async_exprs (apache#19118) Now that each plan's own try_to_proto hook reads the struct fields directly, none of them has any caller. Deprecate rather than remove, per the API health policy; removal follows the normal deprecation window. AnalyzeExec::verbose was introduced by the same serialization PR (apache#7574) but is deliberately left alone: datafusion-distributed calls it from its distributed planner, which downcasts a planned AnalyzeExec and rebuilds it as a distributed node. That is a real non-serialization consumer, so deprecating it would be wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
4e92ef8 to
d17806f
Compare
|
@kumarUjjawal could you take a look please? |
kumarUjjawal
left a comment
There was a problem hiding this comment.
Thanks @adriangb
Looks good. Just had one concern it's upto you to decide what's best.
| } | ||
|
|
||
| /// Access to show_statistics | ||
| #[deprecated( |
There was a problem hiding this comment.
| } | ||
|
|
||
| /// Indices of the list-typed columns in the input schema | ||
| #[deprecated( |
There was a problem hiding this comment.
Review turned up external callers for three of the four accessors deprecated here, so they are no longer deprecated: - AnalyzeExec::show_statistics — openobserve reads it alongside verbose() when swapping AnalyzeExec for its own DistributeAnalyzeExec - UnnestExec::list_column_indices / struct_column_indices — goldsky streamling reads both in StreamingUnnestExec::from_original, which rebuilds a DataFusion UnnestExec as its own streaming operator All three are the same shape as AnalyzeExec::verbose, which was already excluded: downstream code downcasts a planned node and needs to read its private fields to rebuild it, with no other way to get at them. That leaves AsyncFuncExec::async_exprs as the only deprecation. A search across public Rust code turned up no caller outside DataFusion forks. Also reverts the #[expect(deprecated)] on roundtrip_analyze, which is no longer needed now that show_statistics is not deprecated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AsyncFuncExec::async_exprs, which only existed for proto serialization
|
Good catch, I un-deprecated those methods. |
Which issue does this PR close?
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_protohook — which lives in the same module and can read the fields directly — the accessors have no callers inside DataFusion.But "no caller inside DataFusion" is not the same as "no caller". Of the five, only one is deprecated — the other four all turned out to have a real, non-serialization consumer downstream:
AsyncFuncExec::async_exprsAsyncFuncExec" (#19118)AnalyzeExec::verboseAnalyzeExec::show_statisticsUnnestExec::list_column_indicesUnnestExec::struct_column_indicesAn earlier revision of this PR deprecated four of the five. @kumarUjjawal's review pointed at two downstream projects I had not checked, which between them use three of those four. Those three deprecations have been reverted; see below.
Every one of the four kept accessors is the same shape: downstream code downcasts a planned node and needs to read its private fields in order to rebuild it as its own node. That is a legitimate use, and the fact that an accessor was originally added for proto doesn't make its current use wrong. Deprecating them would push a warning onto downstream projects for an API they have a real need for, with nothing to point them at instead.
What changes are included in this PR?
Adds
#[deprecated(since = "55.0.0", note = "...")]toAsyncFuncExec::async_exprs. Nothing is removed, no behavior changes, and the other four accessors are untouched.The
noteis honest that there is no replacement:AsyncFuncExecserializes itself throughAsyncFuncExec::try_to_proto, which reads the field directly, so there is nothing to point users at. It follows the existing phrasing used by the deprecated shims indatafusion/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").AsyncFuncExec::async_exprsalready had zero callers before #24166; itstry_to_protohook was written against the field from the start.Are these changes tested?
There is no new behavior to test — the real verification is that the compiler agrees the method is unused. Since
deprecatedis 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 --allcargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings(CI's exact invocation) — clean across every crate, includingdatafusion-cli,benchmarks,datafusion-examplesandsubstraitcargo test -p datafusion-proto --test proto_integration— 219 passed, 0 failedcargo test -p datafusion-physical-plan— passed, 0 failedDownstream usage check
Before deprecating, I checked the three main downstream consumers at their current
main(2026-08-09), by cloning each repo and grepping for all five accessor names plus every mention ofAnalyzeExec/UnnestExec/AsyncFuncExec. GitHub code search returned 503s and silent empty results at the time, which is exactly why the survey missed two projects — @kumarUjjawal caught both in review.45bd823)AnalyzeExec::verbose575e8ea)AnalyzeExec::verbose,AnalyzeExec::show_statistics8d85af9)UnnestExec::list_column_indices,UnnestExec::struct_column_indicesc706360)06f8f1d)datafusion-distributed —
verbose.src/explain_analyze.rs:34builds aDistributedAnalyzeExecfromanalyze_exec.verbose(), driven by a planner that downcasts a plannedAnalyzeExec(src/distributed_planner/distributed_query_planner.rs:96). It does not readshow_statistics—DistributedAnalyzeExecdoesn't carry that flag.openobserve —
verboseandshow_statistics.src/search/src/datafusion/optimizer/physical_optimizer/distribute_analyze.rs:31-37does the same rewrite as datafusion-distributed, but reads both flags:goldsky streamling — both unnest accessors.
crates/streamling-core/src/operators/unnest.rs:85-99, inStreamingUnnestExec::from_original, rebuilds a DataFusionUnnestExecas its own streaming operator and reads both index lists to do it:datafusion-comet — no usage. It constructs
UnnestExec::new(...)innative/core/src/execution/planner.rs:2081but never reads the index lists back out. Zero hits for any of the five names.datafusion-ballista — no usage. Two near-misses, both false positives:
ballista/core/src/planner.rs:148readsanalyze.verbose, but that is the public field on the logicalLogicalPlan::Analyzenode, not the physical accessor;ballista/scheduler/src/state/distributed_explain.rs:155callsUnnestExec::new(...), construction only.AsyncFuncExec::async_exprs. Code search forasync_exprsand for the literalasync_exprs()across public Rust code returns hits only in DataFusion itself and in forks/vendored copies of it (ClickHouse/rust_vendor,apache/datafusion-sandbox,Epsio-Labs/hiring-datafusion,smartdu/datafusion). The three non-fork repos that mentionAsyncFuncExec—apache/sedona-db,influxdata/datafusion-udf-wasm,goldmedal/datafusion-llm-function— have no calls to the accessor.This still covers only what public code search and these five projects show. If you know of a consumer of
AsyncFuncExec::async_exprs, say so and I'll drop the last deprecation too, on the same reasoning applied to the other four.Are there any user-facing changes?
Yes, and the
api changelabel applies.Downstream users who call
AsyncFuncExec::async_exprswill now see a deprecation warning. Nothing breaks in this release — the method still works exactly as before. Removal follows the normal deprecation window described in the API health policy (six major versions or six months, whichever is longer), consistent with the plan in EPIC #23494.AnalyzeExec::verbose,AnalyzeExec::show_statistics,UnnestExec::list_column_indicesandUnnestExec::struct_column_indicesare unchanged, so datafusion-distributed, openobserve and goldsky streamling see no new warning.There is intentionally no replacement API for
async_exprs. If you have a use case for reading that field 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.