adriangb opened a new issue, #24611:
URL: https://github.com/apache/datafusion/issues/24611
## 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
31 un-destructured hooks in this EPIC's scope (16 encoders, 15 decoders)
across 22 files.
| group | files | sub-issue |
|---|---|---|
| leaf and unary expressions | 7 | A1 |
| binary and pattern expressions | 3 | A2 |
| cast expressions | 2 | A3 |
| CASE and IN-list | 2 | A4 |
| lambda expressions | 2 | A5 |
| dynamic filter and scalar subquery | 2 | A6 |
| sort expressions and partitioning | 3 | A7 |
## 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.
--
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]
