apache / apache/datafusion

Destructure proto hooks for sort expressions and partitioning

Closed
#24,619 4 comments 0 reactions 1 assignee Claimed by @Rich-T-kid View on GitHub
proto
Dominant language
Rust
Stars
9.3k
Forks
2.4k
Avg merge
3d 7h
Merged PRs (30d)
344

Description

### Scope

| file | type |
|---|---|
| `physical-expr-common/src/sort_expr.rs` | `PhysicalSortExpr` |
| `physical-expr/src/partitioning.rs` | `Partitioning` |
| `physical-plan/src/repartition/mod.rs` | `RangeExpr` |

6 hooks (3 encoders, 3 decoders).

### Notes

`Partitioning` and `RangeExpr` are enums / small structs rather than plan nodes, so the encoder-side pattern is a `match` with exhaustive variant bindings instead of `let Self { .. }`. The decoder side still gets the prost-node destructure.

### 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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.