You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "adriangb (via GitHub)" <gi...@apache.org> on 2026/08/24 16:31:48 UTC
[I] EPIC: destructure proto serde hooks in `physical-expr` [datafusion]
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: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] EPIC: destructure proto serde hooks in `physical-expr` [datafusion]
Posted by "kumarUjjawal (via GitHub)" <gi...@apache.org>.
kumarUjjawal commented on issue #24611:
URL: https://github.com/apache/datafusion/issues/24611#issuecomment-5399049947
> cc @buraksenn, @kosiew, @kumarUjjawal, @Phoenix500526 since you were instrumental in previous work on this topic
Thank you @adriangb for filing this. I will help move this forward.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] EPIC: destructure proto serde hooks in `physical-expr` [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on issue #24611:
URL: https://github.com/apache/datafusion/issues/24611#issuecomment-5398323289
cc @buraksenn, @kosiew, @kumarUjjawal, @Phoenix500526 since you were instrumental in previous work on this topic
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org