EPIC: destructure proto serde hooks in `physical-expr`
- Dominant language
- Rust
- Stars
- 9.3k
- Forks
- 2.4k
- Avg merge
- 3d 7h
- Merged PRs (30d)
- 344
Description
## Background
#24164 established a convention for the per-type proto hooks introduced by EPIC #23494: `try_to_proto` starts with an exhaustive `let Self { .. }` (no `..`), and `try_from_proto` destructures the prost-generated node the same way. Adding a field to the struct then becomes a compile error in the encoder, and adding a field to `datafusion.proto` becomes a compile error in every decoder.
That PR applied the convention to the join plans, and the rest of the `physical-plan` plan nodes follow it today. **`physical-expr` was never converted**: every expression hook still reads `self.field` or a getter.
## Why this matters
A hook that reads state field-by-field makes 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 see it. This is not hypothetical:
- `HashJoinExec::fetch` was dropped this way (fixed in #24165).
- `CsvSource` terminator is a live instance found by applying the convention to a single file (#24609).
- Checking each un-destructured encoder in this crate against its struct's fields turned up three more, listed in the sub-issues below.
## Inventory
42 un-destructured hooks in this EPIC's scope (21 encoders, 21 decoders) across 21 files.
| group | files | sub-issue |
|---|---|---|
| leaf and unary expressions | 7 | #24613 |
| binary and pattern expressions | 3 | #24614 |
| cast expressions | 2 | #24615 |
| CASE and IN-list | 2 | #24616 |
| lambda expressions | 2 | #24617 |
| dynamic filter and scalar subquery | 2 | #24618 |
| sort expressions and partitioning | 3 | #24619 |
## Sub-issues
- [ ] #24613 Destructure proto hooks for leaf and unary physical expressions
- [ ] #24614 Destructure proto hooks for binary and pattern physical expressions
- [ ] #24615 Destructure proto hooks for cast physical expressions
- [x] #24616 Destructure proto hooks for CASE and IN-list physical expressions -- #24700 (merged)
- [x] #24617 Destructure proto hooks for lambda physical expressions -- #24775 (merged)
- [ ] #24618 Destructure proto hooks for dynamic filter and scalar subquery expressions
- [ ] #24619 Destructure proto hooks for sort expressions and partitioning
## Field drops already confirmed
| field | set via | effect after a round-trip |
|---|---|---|
| `BinaryExpr::fail_on_overflow` | `with_fail_on_overflow(true)` | decode defaults to `false`: checked arithmetic silently becomes wrapping, **changing query results** |
| `CastExpr::cast_options` | `CastExpr::new(.., Some(opts))` | decode passes `None`, resolving to `DEFAULT_CAST_OPTIONS` |
| `CastExpr::target_field` | `new_with_target_field` | rebuilt from the data type; name and metadata lost |
| `Literal::field` | `Literal::new_with_metadata` | decode calls `Literal::new(value)`; field name and metadata lost |
`BinaryExpr::fail_on_overflow` is the one worth fixing first, since it is the only one that changes results rather than metadata or performance.
## Public API to consider deprecating alongside this work
None found in `physical-expr`. The getters these hooks call (`CastExpr::cast_type`, `CastExpr::expr`, `LambdaExpr::body`, `LambdaExpr::params`, `LambdaVariable::index`, ...) all have other callers in the workspace, so converting the hooks to destructuring does not orphan any of them.
For contrast, the equivalent review of the already-converted plan nodes did find dead accessors -- see the corresponding section on the `datasource` EPIC for the method and results.
Contributor guide
Research direction
Start in the physical-expr crate by locating its try_to_proto and try_from_proto hooks, then use unchecked sub-issues #24613, #24614, #24615, #24618, and #24619 to scope the affected expression groups. Compare each hook with its struct and the prost-generated node in datafusion.proto; done means all 42 hooks use exhaustive destructuring and preserve every field through round-tripping.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- data-engineering
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100