adriangb opened a new issue, #24171:
URL: https://github.com/apache/datafusion/issues/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).
--
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]
