adriangb opened a new pull request, #24167:
URL: https://github.com/apache/datafusion/pull/24167
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and
enhancements and this helps us generate change logs for our releases. You can
link an issue to this PR using the GitHub syntax. For example `Closes #123`
indicates that this PR will close issue #123.
-->
- Follow-up cleanup for EPIC #23494; does not close an issue on its own.
## Rationale for this change
EPIC #23494 moved every built-in `ExecutionPlan` off the central
`downcast_ref`
chain in `datafusion-proto` onto per-plan hooks: a `try_to_proto` override on
the `ExecutionPlan` trait and an inherent `FooExec::try_from_proto`, both
living
in the plan's own module.
Those hooks currently read plan state through **getters** (`self.expr()`,
`self.fetch()`, ...). That means adding a field to a plan struct is
invisible to
serialization: nothing breaks, nothing warns, the field is just silently
dropped on round-trip. This is not hypothetical — it has already cost us a
real
bug: `HashJoinExec.fetch` is not serialized and is lost on round-trip (being
fixed separately).
Starting each hook with an *exhaustive* destructure turns that class of
mistake
into a compile error:
* **Encode side** — destructure `self` naming every field, no `..`. Add a
field
to `SortExec` and `SortExec::try_to_proto` stops compiling until you either
serialize it or bind it to `_` with a comment saying why it does not need
to
be (derived at construction, runtime state, recomputed on decode).
* **Decode side** — destructure the prost node struct the same way. The
generated structs are plain, all-`pub` and not `#[non_exhaustive]`, so
adding
a field to the `.proto` becomes a compile error in every decoder rather
than a
silent omission.
## What changes are included in this PR?
A mechanical, behavior-preserving refactor of the serde hooks for the core
(non-join, non-aggregate, non-window) plans in
`datafusion/physical-plan/src/`:
`SortExec`, `SortPreservingMergeExec`, `GlobalLimitExec`, `LocalLimitExec`,
`FilterExec`, `ProjectionExec`, `RepartitionExec`, `UnionExec`,
`InterleaveExec`, `CoalesceBatchesExec`, `CoalescePartitionsExec`,
`CooperativeExec`, `BufferExec`, `EmptyExec`, `PlaceholderRowExec`,
`ExplainExec`, `ScalarSubqueryExec`.
**The wire format is unchanged — byte for byte — and no behavior changes.**
The
same values are written to and read from the same proto fields; only the way
they are reached in Rust changes.
Split into four commits, one per group of plans, each of which builds green.
### Fields the refactor documented as not serialized
Writing the destructures surfaced and pinned down, in comments at the point
of
use, which fields do not survive a round-trip today. All of these are
pre-existing; **none of them are changed here**, because fixing any of them
means changing the wire format, which is a separate decision:
* `SortPreservingMergeExec::enable_round_robin_repartition` — no field on
`SortPreservingMergeExecNode`; decoding always restores the `true` default
from `SortPreservingMergeExec::new`.
* `GlobalLimitExec::required_ordering` and
`LocalLimitExec::required_ordering` —
no field on the limit nodes; they are set by the `enforce_sorting`
optimizer
rule, so a decoded plan starts with `None`.
Everything else that binds to `_` is genuinely not plan shape: runtime
metrics
(`metrics` / `metrics_set`), execution-time state (`RepartitionExec::state`,
`ScalarSubqueryExec::subquery_future` and `results`), values derived at
construction from other serialized fields (`SortExec::common_sort_prefix`),
the
positional `ScalarSubqueryLink::index`, and the `cache` / `properties` plan
properties that are recomputed on decode.
Two small notes for reviewers:
* `RepartitionExec` keeps its output partitioning *inside* `cache`
(`partitioning()` is `&self.cache.partitioning`), so `cache` is bound
rather
than `_`-ed there and is read for the serialized `partitioning` field.
* `EmptyExec` / `PlaceholderRowExec` now read the encoded partition count
from
the `partitions` field instead of going through
`properties().output_partitioning().partition_count()`. The two are kept in
sync by `with_partitions`, the only writer besides `new`, so the encoded
value
is identical.
## Are these changes tested?
Covered by the existing round-trip tests, which are the real proof that the
wire
format did not move. Locally on this branch:
* `cargo test -p datafusion-proto --test proto_integration` — 214 passed, 0
failed
* `cargo test -p datafusion-physical-plan` — 1640 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 change adds no new behavior to test, and the
compile-error property it introduces is enforced by rustc 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]
