adriangb opened a new pull request, #24164:
URL: https://github.com/apache/datafusion/pull/24164
## 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` onto per-plan hooks: a `try_to_proto` override inside
`impl ExecutionPlan for FooExec`, and an inherent `FooExec::try_from_proto`.
Those hooks read plan state through **getters** (`self.filter()`,
`self.join_type()`, …).
That makes adding a field to a plan struct invisible to serialization —
nothing breaks
at compile time, the new field is just silently not serialized, and the bug
only shows
up as state quietly disappearing on a round-trip.
This is not hypothetical. `HashJoinExec.fetch` is dropped on round-trip
today for exactly
this reason (being fixed separately) — the field was added to the struct and
the encoder,
still calling getters, never learned about it.
Destructuring removes the failure mode. If `try_to_proto` starts with an
exhaustive
`let Self { … } = self;` with **no `..`**, adding a field to the plan struct
is a compile
error in the encoder. If `try_from_proto` destructures the prost-generated
node struct the
same way, adding a field to the `.proto` is a compile error in every
decoder. The prost
structs are plain, all-`pub`, and not `#[non_exhaustive]`, so this compiles
today.
## What changes are included in this PR?
Applies the pattern to the join plans, one commit per plan:
- `CrossJoinExec`
- `NestedLoopJoinExec`
- `SortMergeJoinExec`
- `SymmetricHashJoinExec`
- `HashExpr` / `HashTableLookupExpr`
(`joins/hash_join/partitioned_hash_eval.rs`)
Fields that genuinely are not serialized bind to `_` and carry a short
comment saying
why — derived at construction, runtime state, or recomputed by
`new`/`try_new` on decode.
For example:
```rust
let Self {
left,
right,
on,
filter,
join_type,
sort_options,
null_equality,
// derived from the children's schemas by `try_new` on decode
schema: _,
// runtime metrics, not part of the plan
metrics: _,
// recomputed from `on` and `sort_options` by `try_new` on decode
left_sort_exprs: _,
// recomputed from `on` and `sort_options` by `try_new` on decode
right_sort_exprs: _,
// recomputed by `try_new` on decode
cache: _,
} = self;
```
`HashTableLookupExpr::try_to_proto` deliberately serializes none of its
state — it holds a
runtime `Arc<Map>` and is replaced with `lit(true)`. It gets a destructure
with every field
bound to `_`, so that adding a field there forces a decision instead of
passing unnoticed.
`datafusion/physical-plan/src/joins/hash_join/exec.rs` is deliberately
**not** touched to
avoid conflicting with the in-flight `HashJoinExec.fetch` fix.
**No additional unserialized fields were found.** Every field on the four
join plans that is
not written to the proto is genuinely derived, recomputed on decode, or
runtime-only.
**This is a pure refactor. The wire format is byte-for-byte unchanged and no
behavior changes.**
The only incidental change is in `SymmetricHashJoinExec::try_from_proto`,
where three
`internal_datafusion_err!` calls switched to inlined format args now that
the field is bound
locally; the message text they produce is identical.
## Are these changes tested?
Covered by the existing round-trip tests — that is the point of the change:
if the
destructures had drifted from the encoders, the plans would fail to
round-trip.
Run locally:
- `cargo test -p datafusion-proto --test proto_integration`
- `cargo test -p datafusion-physical-plan`
- `cargo clippy -p datafusion-physical-plan --all-targets --all-features --
-D warnings`
- `cargo fmt --all`
## Are there any user-facing changes?
No.
--
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]
