Improve physical plan proto round-trip test coverage (missing SortPreservingMergeExec; Debug-string assertions miss dropped fields) #24171

Is your feature request related to a problem or challenge?

While reviewing #24167, @andygrove pointed out that the physical-plan proto round-trip tests may not be as comprehensive as PR descriptions tend to assume:

The PR description says that the changes in this PR are tested by existing roundtrip tests, but I'm not sure that those are comprehensive? For example, I don't see a roundtrip test for SortPreservingMergeExec which is updated in this PR.

He's right, and checking turned up two distinct problems.

1. SortPreservingMergeExec has no round-trip coverage at all. The string SortPreservingMerge does not appear anywhere in datafusion/proto/tests/cases/roundtrip_physical_plan.rs — no test constructs one. (The other 16 plans touched by #24167 each have at least one dedicated roundtrip_* test, so this looks like an isolated gap rather than a systemic one — but it went unnoticed for a long time.)

2. The main test helper is structurally blind to a whole class of bug. roundtrip_test asserts on format!("{plan:?}"), so it cannot detect a dropped field that the plan's Debug impl doesn't print. This is not theoretical — it is exactly how #24165 (HashJoinExec::fetch silently lost during ser/de) survived: with the encode side deliberately reverted, the Debug comparison still passes and only a direct fetch() assertion fails.

So "covered by existing round-trip tests" can be true and still leave a field silently unserialized.

Describe the solution you'd like

a. Fill the SortPreservingMergeExec gap — a round-trip test covering input, sort expressions, and fetch in both Some and None states.

b. Audit for fields that are exercised by no test, and add targeted assertions where a silent serialization gap would change query results. Not a push for 100% coverage — the goal is the fields where being wrong is expensive.

c. Where the state isn't in Debug output, assert on accessors rather than the helper. Any new test should be verified to actually fail against a deliberately broken encode side; a test that passes against broken code is worse than no test.

d. Consider a second, colocated test tier. Now that the serde hooks live next to each plan (#23494), field-level tests can live there too, without any new dependency: datafusion-physical-plan already has datafusion-proto-models behind the proto feature, and the dispatch inversion lets a test supply its own encoder. There's already a working example of this in datafusion/datasource/src/file_scan_config/mod.rs (UnusedPlanEncoder + ExecutionPlanEncodeCtx::new(&encoder)).

The two tiers are complementary, not substitutes:

• Colocated tests prove a plan handles its own fields — enum conversions that must be by-name, the [u32::MAX] empty-projection sentinel, fetch presence semantics (absent → None, not Some(0)). These are the tests that rot when someone adds a field, so they belong next to the field, and they can assert on private state directly.
• Central round-trip tests prove things a colocated test structurally cannot: that the real PhysicalExtensionCodec works, that datafusion-proto's dispatch actually reaches the hook, that the deprecated shims still delegate, and that bytes survive bytes.

Colocation should supplement round-trip coverage, never replace it.

e. Possibly split the test file. roundtrip_physical_plan.rs is 5,248 lines; tests/cases/plans/{joins,sorts,aggregates,…}.rs would improve locality independently of everything above.

Describe alternatives you've considered

Moving round-trip tests wholesale into datafusion-physical-plan would require a dev-dependency on datafusion-proto, which sits above it — the first upward dependency cycle in a workspace that publishes ~30 crates in dependency order. Cargo allows it, but it complicates release tooling for little gain, so the two-tier split above seems preferable.

Additional context

Split out of #24167 so the mechanical destructuring refactor and the test work can be reviewed and merged independently. Related: #23494 (the serde migration EPIC), #24165 (the fetch bug that motivated this).