Skip to content

chore(proto): deprecate AsyncFuncExec::async_exprs, which only existed for proto serialization - #24168

Merged
adriangb merged 2 commits into
apache:mainfrom
pydantic:refactor/deprecate-proto-only-accessors
Aug 9, 2026
Merged

chore(proto): deprecate AsyncFuncExec::async_exprs, which only existed for proto serialization#24168
adriangb merged 2 commits into
apache:mainfrom
pydantic:refactor/deprecate-proto-only-accessors

Conversation

@adriangb

@adriangb adriangb commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

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_proto hook — 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:

Method Introduced by Callers in DataFusion Known downstream caller This PR
AsyncFuncExec::async_exprs "[Proto]: Serialization support for AsyncFuncExec" (#19118) none none found deprecated
AnalyzeExec::verbose "Implement protobuf serialization for AnalyzeExec" (#7574) none datafusion-distributed, openobserve kept as-is
AnalyzeExec::show_statistics "Implement protobuf serialization for AnalyzeExec" (#7574) none openobserve kept as-is
UnnestExec::list_column_indices "Support encoding and decoding UnnestExec" (#12344) none goldsky streamling kept as-is
UnnestExec::struct_column_indices "Support encoding and decoding UnnestExec" (#12344) none goldsky streamling kept as-is

An 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 = "...")] to AsyncFuncExec::async_exprs. Nothing is removed, no behavior changes, and the other four accessors are untouched.

The note is honest that there is no replacement: AsyncFuncExec serializes itself through AsyncFuncExec::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 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").

AsyncFuncExec::async_exprs already had zero callers before #24166; its try_to_proto hook 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 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 — 219 passed, 0 failed
  • cargo test -p datafusion-physical-plan — passed, 0 failed

Downstream 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 of AnalyzeExec / 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.

Repo Uses any of the five? Which
datafusion-distributed (45bd823) Yes AnalyzeExec::verbose
openobserve (575e8ea) Yes AnalyzeExec::verbose, AnalyzeExec::show_statistics
goldsky streamling (8d85af9) Yes UnnestExec::list_column_indices, UnnestExec::struct_column_indices
datafusion-comet (c706360) No
datafusion-ballista (06f8f1d) No

datafusion-distributed — verbose. src/explain_analyze.rs:34 builds a DistributedAnalyzeExec from analyze_exec.verbose(), driven by a planner that downcasts a planned AnalyzeExec (src/distributed_planner/distributed_query_planner.rs:96). It does not read show_statisticsDistributedAnalyzeExec doesn't carry that flag.

openobserve — verbose and show_statistics. src/search/src/datafusion/optimizer/physical_optimizer/distribute_analyze.rs:31-37 does the same rewrite as datafusion-distributed, but reads both flags:

if let Some(analyze) = plan.downcast_ref::<AnalyzeExec>() {
    let distribute_analyze = Arc::new(DistributeAnalyzeExec::new(
        analyze.verbose(),
        analyze.show_statistics(),
        analyze.input().clone(),
    )) as Arc<dyn ExecutionPlan>;

goldsky streamling — both unnest accessors. crates/streamling-core/src/operators/unnest.rs:85-99, in StreamingUnnestExec::from_original, rebuilds a DataFusion UnnestExec as its own streaming operator and reads both index lists to do it:

let list_column_indices = original_unnest
    .list_column_indices()
    .iter()
    .map(|idx| ListUnnest { index_in_input_schema: idx.index_in_input_schema, depth: idx.depth })
    .collect();
let struct_column_indices = original_unnest.struct_column_indices().to_vec();

datafusion-comet — no usage. It constructs UnnestExec::new(...) in native/core/src/execution/planner.rs:2081 but 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:148 reads analyze.verbose, but that is the public field on the logical LogicalPlan::Analyze node, not the physical accessor; ballista/scheduler/src/state/distributed_explain.rs:155 calls UnnestExec::new(...), construction only.

AsyncFuncExec::async_exprs. Code search for async_exprs and for the literal async_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 mention AsyncFuncExecapache/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 change label applies.

Downstream users who call AsyncFuncExec::async_exprs will 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_indices and UnnestExec::struct_column_indices are 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.

@adriangb adriangb added the api change Changes the API exposed to users of the crate label Aug 7, 2026
@github-actions github-actions Bot added proto Related to proto crate physical-plan Changes to the physical-plan crate labels Aug 7, 2026
@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

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
     Cloning apache/main
    Building datafusion-physical-plan v54.1.0 (current)
       Built [  38.314s] (current)
     Parsing datafusion-physical-plan v54.1.0 (current)
      Parsed [   0.141s] (current)
    Building datafusion-physical-plan v54.1.0 (baseline)
       Built [  37.756s] (baseline)
     Parsing datafusion-physical-plan v54.1.0 (baseline)
      Parsed [   0.150s] (baseline)
    Checking datafusion-physical-plan v54.1.0 -> v54.1.0 (no change; assume patch)
     Checked [   0.677s] 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::async_func::AsyncFuncExec::async_exprs in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/async_func.rs:113

     Summary semver requires new minor version: 0 major and 1 minor checks failed
    Finished [  78.902s] datafusion-physical-plan

@github-actions github-actions Bot added the auto detected api change Auto detected API change label Aug 7, 2026
@codecov-commenter

codecov-commenter commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.04%. Comparing base (918013e) to head (6e312b6).
⚠️ Report is 1 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@adriangb
adriangb force-pushed the refactor/deprecate-proto-only-accessors branch 2 times, most recently from 3ef75ee to 4e92ef8 Compare August 9, 2026 12:10
…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>
@adriangb
adriangb force-pushed the refactor/deprecate-proto-only-accessors branch from 4e92ef8 to d17806f Compare August 9, 2026 12:34
@adriangb
adriangb requested a review from kumarUjjawal August 9, 2026 12:34
@adriangb

adriangb commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

@kumarUjjawal could you take a look please?

@kumarUjjawal kumarUjjawal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @adriangb

Looks good. Just had one concern it's upto you to decide what's best.

Comment thread datafusion/physical-plan/src/analyze.rs Outdated
}

/// Access to show_statistics
#[deprecated(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread datafusion/physical-plan/src/unnest.rs Outdated
}

/// Indices of the list-typed columns in the input schema
#[deprecated(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@adriangb adriangb changed the title chore(proto): deprecate accessors that only existed for proto serialization chore(proto): deprecate AsyncFuncExec::async_exprs, which only existed for proto serialization Aug 9, 2026
@adriangb

adriangb commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Good catch, I un-deprecated those methods.

@adriangb
adriangb enabled auto-merge August 9, 2026 13:45
@adriangb
adriangb added this pull request to the merge queue Aug 9, 2026
Merged via the queue into apache:main with commit 33f3688 Aug 9, 2026
40 of 41 checks passed
@adriangb
adriangb deleted the refactor/deprecate-proto-only-accessors branch August 9, 2026 14:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api change Changes the API exposed to users of the crate auto detected api change Auto detected API change physical-plan Changes to the physical-plan crate proto Related to proto crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants