Destructure proto hooks for leaf and unary physical expressions
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
### Scope
| file | type |
|---|---|
| `physical-expr/src/expressions/column.rs` | `Column` |
| `physical-expr/src/expressions/unknown_column.rs` | `UnKnownColumn` |
| `physical-expr/src/expressions/literal.rs` | `Literal` |
| `physical-expr/src/expressions/is_null.rs` | `IsNullExpr` |
| `physical-expr/src/expressions/is_not_null.rs` | `IsNotNullExpr` |
| `physical-expr/src/expressions/negative.rs` | `NegativeExpr` |
| `physical-expr/src/expressions/not.rs` | `NotExpr` |
14 hooks (7 encoders, 7 decoders).
### Field drop found in this group
**`Literal::field`** is not encoded. `Literal` holds `value: ScalarValue` and `field: FieldRef`; the encoder writes only `value`, and `Literal::try_from_proto` rebuilds the expression with `Literal::new(value)`. A literal constructed with `Literal::new_with_metadata` loses its field name and metadata on round-trip. Whether that should be carried on the wire or deliberately dropped is the decision this issue should record.
### 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.
Contributor guide
Assessment
This issue has not been assessed yet.