adriangb commented on PR #24167:
URL: https://github.com/apache/datafusion/pull/24167#issuecomment-5220089825
Thanks @andygrove — you were right to push on this, and it turned out better
than a spot check.
`SortPreservingMergeExec` has no round-trip test at all: the string
`SortPreservingMerge` doesn't appear anywhere in `roundtrip_physical_plan.rs`.
I checked the other 16 plans this PR touches and each has at least one
dedicated `roundtrip_*` test, so it looks like an isolated gap rather than a
systemic one — but it went unnoticed for a long time.
Digging in also surfaced a second, more structural problem: `roundtrip_test`
asserts on `format!("{plan:?}")`, so it can't detect a dropped field that a
plan's `Debug` impl doesn't print. That's exactly how the `HashJoinExec::fetch`
bug in #24165 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 while a field is silently
unserialized.
I've filed #24171 for both, and I'm splitting the test work into its own PR
based on `main` rather than adding it here, so this mechanical destructuring
change stays reviewable on its own. A side benefit: since those tests sit on
`main` *before* the refactor, they pin the current wire behaviour
independently, which makes this PR's "wire format unchanged" claim verifiable
rather than just asserted.
I've also corrected the description above — the original "covered by
existing round-trip tests" oversold it.
Do you have a preference on sequencing: land the test PR first so this one
merges against proven coverage, or does the order not matter to you?
--
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]
