adriangb opened a new issue, #24614:
URL: https://github.com/apache/datafusion/issues/24614
### Scope
| file | type |
|---|---|
| `physical-expr/src/expressions/binary.rs` | `BinaryExpr` |
| `physical-expr/src/expressions/like.rs` | `LikeExpr` |
| `physical-expr/src/expressions/similar_to_pattern.rs` |
`SqlSimilarToPattern` |
6 hooks (3 encoders, 3 decoders).
### Field drop found in this group -- this one changes query results
**`BinaryExpr::fail_on_overflow` is silently dropped.**
`PhysicalBinaryExprNode` has no field for it and the encoder never reads it;
`BinaryExpr::try_from_proto` rebuilds via `BinaryExpr::new`, which sets
`fail_on_overflow: false`.
An expression built with the public
`BinaryExpr::with_fail_on_overflow(true)` returns an error on arithmetic
overflow. After a serialization round-trip the same expression wraps instead.
Unlike the other drops on this EPIC, this one changes the rows a query produces
rather than performance or formatting, so it is worth fixing ahead of the
mechanical work.
### 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]
