adriangb opened a new issue, #24618:
URL: https://github.com/apache/datafusion/issues/24618
### Scope
| file | type |
|---|---|
| `physical-expr/src/expressions/dynamic_filters/mod.rs` |
`DynamicFilterPhysicalExpr` |
| `physical-expr/src/scalar_subquery.rs` | `ScalarSubqueryExpr` |
4 hooks (2 encoders, 2 decoders).
### Notes
These two carry the most runtime state of any expression, so they benefit
most from having it written down explicitly:
- `DynamicFilterPhysicalExpr`: `current_cache`, `state_watch` (runtime),
`data_type`, `nullable` (cached and recomputable) are all unmentioned by the
encoder today.
- `ScalarSubqueryExpr::results` is runtime state filled in during execution.
Each should become a `_` binding with a one-line reason rather than an
omission.
### Why
Serde hooks that read state through getters or `self.field` make an added
field invisible to serialization: nothing breaks at compile time, the field
simply stops round-tripping, and `Debug`-comparing round-trip tests do not
notice. `HashJoinExec::fetch` was lost exactly this way (#24165), and #24609 is
a second live instance found by applying the convention to one file.
#24164 established the fix -- exhaustive destructuring in both directions --
and applied it to the join plans. The `physical-plan` plan nodes are done.
`physical-expr` and the `datasource*` crates were never converted.
### What to do
For each hook in scope:
1. In `try_to_proto`, start with an exhaustive `let Self {{ .. }}` -- **no
`..` rest pattern**. Fields that are genuinely not serialized bind to `_` with
a short comment saying why (derived at construction, runtime state, recomputed
on decode, carried by a parent message).
2. In `try_from_proto`, destructure the prost-generated node struct the same
way, so adding a field to `datafusion.proto` is a compile error in every
decoder.
3. If the destructure turns up a field that *should* round-trip but has no
wire representation, add it to the message and cover it with a test that fails
without the fix.
### Definition of done
- [ ] Every field of the plan/expression struct is either serialized or
bound to `_` with a reason.
- [ ] Every field of the prost node is destructured in the decoder.
- [ ] Any newly serialized field has a round-trip test, verified to fail
before the fix.
- Part of #24611.
--
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]
