adriangb opened a new pull request, #24166: URL: https://github.com/apache/datafusion/pull/24166
## Which issue does this PR close? <!-- No dedicated issue; this is follow-up cleanup for the proto hook migration EPIC. --> - Part of #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: 1. `AggregateExec` / `protobuf::AggregateExecNode` 2. `WindowAggExec` and `BoundedWindowAggExec` / `protobuf::WindowAggExecNode` (they share one decoder) 3. `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. -- 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]
