adriangb commented on issue #23494:
URL: https://github.com/apache/datafusion/issues/23494#issuecomment-5219122892
## Post-migration cleanup
With the plan-side checklist essentially complete (only the scan sources —
#23516, #23517, #23518 — remain, and both foundations they were blocked on have
landed), here is a round of follow-up cleanup, plus one real bug the audit
turned up.
### The bug this found
**#24165 — `fix(proto): preserve HashJoinExec fetch across serialization`**
`protobuf::HashJoinExecNode` has no `fetch` field, so `HashJoinExec.fetch`
round-trips `Some(n) -> None`. It matters because `limit_pushdown` pushes the
limit *into* the join via `with_fetch` and then drops the enclosing
`GlobalLimitExec` — so after a round-trip the plan has no limit at all and a
distributed executor returns more rows than the query asked for.
The new field is presence-tracked (`optional uint64`) on purpose: a plain
proto3 scalar decodes an absent field as `0`, which under `SortExecNode`'s
negative-sentinel convention would read as "fetch 0 rows" and silently empty
out plans serialized by older versions.
Worth noting *why* no existing test caught this: `roundtrip_test` compares
`format!("{plan:?}")`, and `HashJoinExec`'s `Debug` output does not include
`fetch`. This was verified rather than assumed — with the encode side reverted,
the Debug comparison still passes and only a direct `fetch()` assertion fails.
Display-string round-trip assertions cannot catch a dropped field that `Debug`
does not print.
### Cleanup 1 — exhaustive destructuring in the serde hooks
Today the encoders read state through getters, so adding a field to a plan
struct is invisible to serialization: nothing breaks, the field is just
silently not on the wire. That is exactly how the `fetch` bug happened.
These PRs make both directions exhaustive — `try_to_proto` opens with a full
`let Self { .. } = self` destructure (no `..`, every non-serialized field bound
to `_` with a comment saying why), and `try_from_proto` destructures the prost
node struct. Adding a field to a plan struct or to a proto message then becomes
a compile error in every affected encoder/decoder. The prost-generated structs
are plain, all-`pub` and not `#[non_exhaustive]`, so this works on the decode
side too.
Split by family so each stays reviewable; all three are pure refactors with
the wire format byte-for-byte unchanged:
- **#24164** — joins: `SymmetricHashJoinExec`, `NestedLoopJoinExec`,
`CrossJoinExec`, `SortMergeJoinExec`, and the hash-join expressions
- **#24167** — core plans: sorts, limits, filter, projection, repartition,
union/interleave, coalesce/cooperative/buffer, leaves, explain, scalar subquery
(17 plans)
- **#24166** — aggregate and window: `AggregateExec`, both window execs,
unnest, async func, analyze
`HashJoinExec` itself is deliberately absent — it belongs to the bug fix
above; its destructure is a small follow-up once that merges.
### Cleanup 2 — deprecate accessors that only existed for proto
****#24168** (stacked on #24166 — its diff carries that PR's commits until
it merges)**
Several `pub fn` accessors were added purely so the central encoder could
reach private fields. Now that each plan's `try_to_proto` lives in the plan's
own module and (after cleanup 1) reads fields directly, they have no callers at
all:
| Accessor | Introduced by |
|---|---|
| `AnalyzeExec::verbose`, `::show_statistics` | #7574 "Implement protobuf
serialization for AnalyzeExec" |
| `UnnestExec::list_column_indices`, `::struct_column_indices` | #12344
"Support encoding and decoding UnnestExec" |
| `AsyncFuncExec::async_exprs` | #19118 "[Proto]: Serialization support for
`AsyncFuncExec`" |
Deprecated rather than removed, per the API health policy, on the same
`since = "55.0.0"` schedule as the `PhysicalPlanNodeExt` scaffolding.
A second tier of accessors also turns out to have no callers
(`SymmetricHashJoinExec::{left,right}_sort_exprs`,
`WindowAggExec::partition_by_sort_keys`, `ScalarSubqueryExec::subqueries`,
`CoalesceBatchesExec::target_batch_size`, `HashJoinExec::join_schema`,
`CastExpr::cast_options`), but those predate the proto work and do not meet the
"only existed for serialization" bar — happy to handle them separately if
people think they should go.
### Gaps the refactor documented but did not fix
Each of these is a field that does not survive a round-trip. They are left
behaving exactly as today, with a comment recording the state, since changing
the wire format inside a cleanup PR would be worse than the gap:
- **`AnalyzeExec::metric_types`** — no field on `AnalyzeExecNode`, and
`AnalyzeExecBuilder` unconditionally resets it to `[Summary, Dev]`, so a
non-default metric type selection is lost. This is the same class of bug as the
`fetch` one and probably wants its own fix.
- `SortPreservingMergeExec::enable_round_robin_repartition` — not on the
wire; decode restores the `true` default. Only set to `false` in tests/benches
today, so no production path is affected.
- `Global/LocalLimitExec::required_ordering` — not on the wire; set by
`enforce_sorting`, so decode yields `None`.
- `HashTableLookupExpr` — serializes none of its state by design (runtime
map, replaced with `lit(true)`). Pre-existing and intentional; it now has an
all-`_` destructure so a future field addition forces an explicit decision.
### Possible further follow-ups
- The same destructuring treatment for the `PhysicalExpr` hooks from #22418.
- There is no guard test asserting that every `PhysicalPlanType` variant
round-trips — 157 hand-written tests, no exhaustive enumeration.
--
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]
