refactor(proto): destructure plan and proto structs in aggregate and window serde hooks by adriangb · Pull Request #24166 · apache/datafusion

adriangb · 2026-08-07T15:30:18Z

Which issue does this PR close?

Part of [EPIC] Port ExecutionPlan serialization to try_to_proto / try_from_proto hooks #23494.

Rationale for this change

EPIC #23494 moved every built-in ExecutionPlan off the central downcast_ref chain in datafusion-proto and onto per-plan hooks (ExecutionPlan::try_to_proto + an inherent FooExec::try_from_proto) that live in the plan's own module.

Those hooks currently read plan state through getters. That means adding a field to a plan struct is invisible to serialization: nothing breaks, the field is just silently not serialized, and the omission only shows up later as a plan that quietly changes shape after a round-trip.

This is not hypothetical. HashJoinExec.fetch is dropped on round-trip today for exactly this reason (being fixed separately). The same class of bug is one commit away in every other plan.

This PR removes the failure mode for the aggregate, window, and remaining misc plans by making both directions exhaustive:

Encode side: each try_to_proto begins with an exhaustive destructure of self. Every field is named — no ... Adding a field to the plan struct is now a compile error until the author decides what happens to it.
Decode side: each try_from_proto destructures the prost-generated node struct exhaustively. Those structs are plain, all-pub and not #[non_exhaustive], so this compiles — and a newly added proto field becomes a compile error in every decoder rather than a silently ignored wire field.

Fields that genuinely are not serialized bind to _ with a short comment saying why: derived at construction, runtime state, or recomputed on decode.

What changes are included in this PR?

Three commits, one per plan group, each green on its own:

AggregateExec / protobuf::AggregateExecNode
WindowAggExec and BoundedWindowAggExec / protobuf::WindowAggExecNode (they share one decoder)
UnnestExec, AsyncFuncExec, AnalyzeExec and their nodes

All changes are confined to datafusion/physical-plan/src/.

The wire format is unchanged — byte for byte. No behavior changes. This is a pure refactor; the encoders build the same proto messages from the same values, just reached through destructured bindings instead of accessors.

Implicit coupling now documented

WindowAggExec::can_repartition / BoundedWindowAggExec::can_repartition have no wire field of their own. partition_keys() returns an empty vec when can_repartition is false, and the decoder recovers the flag as !partition_keys.is_empty(). That round trip was previously something you had to already know; it is now an explicit comment on both the encode and the decode side.

Unserialized fields the refactor documented

Fields bound to _ because they are legitimately reconstructed rather than transmitted:

Plan	Field(s)	Why
`AggregateExec`	`schema`, `required_input_ordering`, `input_order_mode`, `cache`	derived at construction
`AggregateExec`	`metrics`	runtime state
`WindowAggExec`	`schema`, `ordered_partition_by_indices`, `cache`	derived at construction
`BoundedWindowAggExec`	`schema`, `ordered_partition_by_indices`, `cache`	derived at construction
`WindowAggExec` / `BoundedWindowAggExec`	`can_repartition`	no wire field; folded into `partition_keys` (see above)
`UnnestExec`	`cache` (derived), `metrics` (runtime)
`AsyncFuncExec`	`cache` (derived), `metrics` (runtime)
`AnalyzeExec`	`cache`	derived at construction

The AggrDynFilter case is also now commented: only the shared filter expr goes on the wire; the per-accumulator bounds are runtime state repopulated during execution.

One real gap found, deliberately left alone

AnalyzeExec::metric_types is not serialized. There is no field for it on AnalyzeExecNode, and AnalyzeExecBuilder unconditionally resets it to [MetricType::Summary, MetricType::Dev], so a non-default metric type selection does not survive a round trip.

Fixing this requires a new proto field, which is a wire-format change and therefore out of scope for a cleanup PR — a refactor that silently alters the wire format would be worse than the gap it fixes. The field is left bound to _ with a TODO describing the current state, so it can be filed and fixed separately.

Are these changes tested?

Covered by the existing round-trip test suite, which is the actual proof that the wire format did not move:

cargo test -p datafusion-proto --test proto_integration — 214 passed, 0 failed
cargo test -p datafusion-physical-plan --all-features — 1648 + 9 passed, 0 failed
cargo clippy -p datafusion-physical-plan --all-targets --all-features -- -D warnings — clean
cargo fmt --all

No new tests are added: the refactor introduces no new behavior to test, and its safety property (a forgotten field becomes a compile error) is enforced by the compiler rather than by a test.

Are there any user-facing changes?

No. No public API changes, no wire-format changes, no behavior changes.

codecov-commenter · 2026-08-07T16:02:13Z

Codecov Report

❌ Patch coverage is 82.55034% with 26 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.05%. Comparing base (92f4e8f) to head (3588946).

Files with missing lines	Patch %	Lines
datafusion/physical-plan/src/analyze.rs	71.87%	4 Missing and 5 partials ⚠️
datafusion/physical-plan/src/aggregates/mod.rs	87.50%	0 Missing and 7 partials ⚠️
datafusion/physical-plan/src/unnest.rs	82.60%	0 Missing and 4 partials ⚠️
datafusion/physical-plan/src/async_func.rs	81.25%	0 Missing and 3 partials ⚠️
...usion/physical-plan/src/windows/window_agg_exec.rs	86.66%	0 Missing and 2 partials ⚠️
...ysical-plan/src/windows/bounded_window_agg_exec.rs	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24166      +/-   ##
==========================================
- Coverage   81.05%   81.05%   -0.01%     
==========================================
  Files        1107     1107              
  Lines      381555   381582      +27     
  Branches   381555   381582      +27     
==========================================
+ Hits       309277   309284       +7     
- Misses      54020    54032      +12     
- Partials    18258    18266       +8

☔ 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.

…erde hooks Start `AggregateExec::try_to_proto` with an exhaustive destructure of `Self` and `AggregateExec::try_from_proto` with an exhaustive destructure of `protobuf::AggregateExecNode`, then build from those bindings instead of calling getters / field paths. Adding a field to either struct without deciding how it is serialized is now a compile error in both directions rather than a silently dropped value. Fields that are intentionally not on the wire bind to `_` with a comment explaining why (derived at construction, runtime state, or recomputed on decode). Pure refactor: the wire format is byte-for-byte unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…rde hooks `WindowAggExec::try_to_proto` and `BoundedWindowAggExec::try_to_proto` now start with an exhaustive destructure of `Self`, and the shared `WindowAggExec::try_from_proto` decoder destructures `protobuf::WindowAggExecNode` exhaustively. This documents the implicit coupling around `can_repartition`: it has no wire field of its own, because `partition_keys()` returns an empty vec when it is false and the decoder recovers it as `!partition_keys.is_empty()`. That is now an explicit comment on both sides instead of tribal knowledge. Adding a field to either struct without deciding how it is serialized is now a compile error in both directions. Pure refactor: the wire format is byte-for-byte unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

…c in serde hooks Apply the same exhaustive-destructure pattern to the remaining misc plans: `try_to_proto` destructures `Self` and `try_from_proto` destructures the prost node struct, so a newly added field on either side is a compile error rather than a silent round-trip gap. The refactor surfaced one pre-existing gap, left as-is and documented with a TODO: `AnalyzeExec::metric_types` has no proto field, and `AnalyzeExecBuilder` resets it to `[Summary, Dev]`, so a non-default metric type selection does not survive a round-trip. Fixing that needs a new wire field and is deliberately out of scope here. Pure refactor: the wire format is byte-for-byte unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

kumarUjjawal

Thansk @adriangb

This looks good!

kumarUjjawal · 2026-08-09T05:11:17Z

+            filter_expr,
+            limit_options,
+            input,
+            // Derived at construction by `create_schema` from `input_schema`,


I don't think schema is always safely derived here. with_new_aggr_exprs and with_new_children preserve the existing schema through try_new_with_schema specifically because rebuilding it after rewriting aggregate expressions can change field names. However, deserialization calls AggregateExec::try_new which rebuilds the schema. Is this an already existing issue?

pre-existing. i will open an issue to track

@kumarUjjawal

…ted for proto serialization (apache#24168) ## Which issue does this PR close? - Follow-up cleanup for the proto hook migration EPIC apache#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 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`" (apache#19118) | none | none found | **deprecated** | | `AnalyzeExec::verbose` | "Implement protobuf serialization for AnalyzeExec" (apache#7574) | none | datafusion-distributed, openobserve | kept as-is | | `AnalyzeExec::show_statistics` | "Implement protobuf serialization for AnalyzeExec" (apache#7574) | none | openobserve | kept as-is | | `UnnestExec::list_column_indices` | "Support encoding and decoding UnnestExec" (apache#12344) | none | goldsky streamling | kept as-is | | `UnnestExec::struct_column_indices` | "Support encoding and decoding UnnestExec" (apache#12344) | none | goldsky streamling | kept as-is | An earlier revision of this PR deprecated four of the five. @kumarUjjawal's [review](apache#24168 (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](#downstream-usage-check). 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 apache#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](https://github.com/datafusion-contrib/datafusion-distributed) (`45bd823`) | Yes | `AnalyzeExec::verbose` | | [openobserve](https://github.com/openobserve/openobserve) (`575e8ea`) | Yes | `AnalyzeExec::verbose`, `AnalyzeExec::show_statistics` | | [goldsky streamling](https://github.com/goldsky-io/streamling) (`8d85af9`) | Yes | `UnnestExec::list_column_indices`, `UnnestExec::struct_column_indices` | | [datafusion-comet](https://github.com/apache/datafusion-comet) (`c706360`) | No | — | | [datafusion-ballista](https://github.com/apache/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_statistics` — `DistributedAnalyzeExec` doesn't carry that flag. **openobserve — `verbose` *and* `show_statistics`.** [`src/search/src/datafusion/optimizer/physical_optimizer/distribute_analyze.rs:31-37`](https://github.com/openobserve/openobserve/blob/575e8ea4d5fd4b0e630aa013a1a31b1da56708e9/src/search/src/datafusion/optimizer/physical_optimizer/distribute_analyze.rs#L31-L37) does the same rewrite as datafusion-distributed, but reads both flags: ```rust 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`](https://github.com/goldsky-io/streamling/blob/8d85af926b034085aece04233722c508ac80f2d0/crates/streamling-core/src/operators/unnest.rs#L85-L99), in `StreamingUnnestExec::from_original`, rebuilds a DataFusion `UnnestExec` as its own streaming operator and reads both index lists to do it: ```rust 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 `AsyncFuncExec` — `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 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](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 apache#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. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>

github-actions Bot added the physical-plan Changes to the physical-plan crate label Aug 7, 2026

This was referenced Aug 7, 2026

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

Merged

[EPIC] Port ExecutionPlan serialization to try_to_proto / try_from_proto hooks #23494

Open

adriangb and others added 3 commits August 7, 2026 11:31

adriangb force-pushed the refactor/proto-destructure-agg-window branch from 9c955d6 to 3588946 Compare August 7, 2026 16:38

kumarUjjawal approved these changes Aug 9, 2026

View reviewed changes

adriangb added this pull request to the merge queue Aug 9, 2026

Merged via the queue into apache:main with commit ddb0250 Aug 9, 2026
40 checks passed

adriangb deleted the refactor/proto-destructure-agg-window branch August 9, 2026 11:10

adriangb mentioned this pull request Aug 9, 2026

AggregateExec physical-plan serde rebuilds the output schema, losing optimizer-preserved field names (and is_reversed) #24202

Open

Navigation Menu

Uh oh!

refactor(proto): destructure plan and proto structs in aggregate and window serde hooks - #24166

refactor(proto): destructure plan and proto structs in aggregate and window serde hooks#24166
adriangb merged 3 commits into
apache:mainfrom
pydantic:refactor/proto-destructure-agg-window

adriangb commented Aug 7, 2026

Uh oh!

codecov-commenter commented Aug 7, 2026 •

edited

Loading

Uh oh!

kumarUjjawal left a comment

Uh oh!

kumarUjjawal Aug 9, 2026

Uh oh!

adriangb Aug 9, 2026

Uh oh!

adriangb Aug 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

adriangb commented Aug 7, 2026

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Implicit coupling now documented

Unserialized fields the refactor documented

One real gap found, deliberately left alone

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Aug 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kumarUjjawal left a comment

Choose a reason for hiding this comment

Uh oh!

kumarUjjawal Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

adriangb Aug 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Aug 7, 2026 •

edited

Loading