Skip to content

Commit 0f68e23

Browse files
adriangbclaude
andauthored
test(proto): add missing physical plan round-trip coverage (#24172)
## Which issue does this PR close? - Part of #24171. This addresses items (a), (b) and (c) of that issue. It does **not** attempt (d) the colocated test tier or (e) splitting up `roundtrip_physical_plan.rs`, both of which are larger and independent, so the issue stays open. Split out of #24167 at the reviewer's prompting: @andygrove pointed out on that PR that the "covered by existing round-trip tests" claim did not hold — there is no round-trip test for `SortPreservingMergeExec`. He was right. Rather than bundle new tests with a mechanical refactor, they are here on their own so the two can be reviewed and merged independently. ## Rationale for this change `datafusion/proto/tests/cases/roundtrip_physical_plan.rs` is the safety net for physical-plan serialization. A field that is silently dropped on the wire produces a plan that still *runs* — it just returns different rows. Checking turned up two real holes: 1. **`SortPreservingMergeExec` had no round-trip test at all.** The string `SortPreservingMerge` did not appear anywhere in the file. Nothing constructed one, so nothing on the encode or decode path for that plan was exercised. 2. **`SortExec::fetch` is serialized but was never exercised.** `roundtrip_sort` and `roundtrip_sort_preserve_partitioning` both leave `fetch` as `None`, so the `Some(..)` state had no coverage. `fetch` is what makes a `SortExec` a top-k sort; dropping it on the wire silently widens the result set. Both are cases where a serialization gap changes query results rather than causing a visible failure. ## What changes are included in this PR? Two new tests in `roundtrip_physical_plan.rs`. No production code changes. - **`roundtrip_sort_preserving_merge`** — covers everything actually on the wire for `SortPreservingMergeExecNode`: the input, the sort expressions, and `fetch` in both its `None` (encoded as `-1`) and `Some(11)` states. - **`roundtrip_sort_with_fetch`** — `SortExec` with `fetch: Some(7)`, plus `fetch: Some(3)` combined with `preserve_partitioning: true`, since both live in the same proto node. ### Which tests assert on accessors rather than the helper, and why The file's `roundtrip_test` helper compares `format!("{plan:?}")` before and after. That comparison is only as good as the plan's `Debug` impl — it is blind to any field `Debug` does not print, which is how #24165 (`HashJoinExec::fetch`) survived. `SortExec` and `SortPreservingMergeExec` both currently *derive* `Debug`, so the helper does in fact observe `expr`, `fetch` and `preserve_partitioning` today. I checked rather than assumed, and the deliberate-break results below confirm it — the helper is what fires first under each break. But that coverage is incidental: it would vanish the day either plan grows a hand-written `Debug`. So both new tests go through `roundtrip_test_and_return`, downcast, and assert on `fetch()`, `expr()`, `preserve_partitioning()` and the input schema directly, with the helper's string comparison still running as a backstop. ### What is deliberately *not* asserted `SortPreservingMergeExec::enable_round_robin_repartition` is **not** serialized — `SortPreservingMergeExecNode` has only `input`, `expr` and `fetch`, so decode always restores the `true` default from `SortPreservingMergeExec::new`. A round-trip equality assertion would pass whether or not that field were on the wire, so asserting on it would advertise coverage that does not exist. The test carries a comment saying so instead. The same applies to `Global/LocalLimitExec::required_ordering`, which is set by the `enforce_sorting` rule and starts as `None` on a decoded plan. If either field *should* be on the wire, that is a separate change with a wire-format bump, not something to paper over with a test that cannot tell the difference. ### Plans I checked and decided needed nothing I went through the rest of the plans touched by #24167 looking for state that is on the wire but exercised by no test. These already have adequate coverage and I did not add to them: | Plan | Existing coverage | |---|---| | `GlobalLimitExec` | `roundtrip_global_limit` (skip 0 / limit 25) and `roundtrip_global_skip_no_limit` (skip 10 / limit `None`) — both `skip` and `fetch` states | | `LocalLimitExec` | `roundtrip_local_limit` | | `FilterExec` | `roundtrip_filter_with_fetch` already asserts `default_selectivity`, `batch_size` and `fetch` on the accessors; `roundtrip_filter_projection_states` covers the projection | | `ProjectionExec` | `roundtrip_projection_source`, `roundtrip_empty_projection` | | `RepartitionExec` | `roundtrip_repartition_preserve_order` (round-robin + `preserve_order`), `roundtrip_range_partitioning`, plus hash-partitioning cases | | `UnionExec` / `InterleaveExec` | `roundtrip_union`, `roundtrip_interleave` — nothing on the wire beyond the children | | `CoalesceBatchesExec` | `roundtrip_coalesce_batches_with_fetch` covers `target_batch_size` and `fetch` in both states | | `CoalescePartitionsExec` | `roundtrip_coalesce_partitions_with_fetch`, both `fetch` states | | `CooperativeExec` | `roundtrip_cooperative` — only the input is on the wire | | `BufferExec` | `roundtrip_buffer` asserts `capacity()` on the accessor | | `EmptyExec` / `PlaceholderRowExec` | `roundtrip_empty_with_partitions`, `roundtrip_placeholder_row_with_partitions` | | `ExplainExec` | `roundtrip_explain` asserts schema, stringified plans and `verbose` on the accessors | | `ScalarSubqueryExec` | `roundtrip_scalar_subquery_exec` and the executing variant | Per the issue, this is not a push for 100% field coverage. These two were the cases where the gap was real and the test was cheap; past them it got contrived fast. ## Are these changes tested? This PR *is* tests. To confirm they are not vacuous, I broke the encode side on purpose and checked each one fails: | Deliberate break | Result | |---|---| | `SortPreservingMergeExec` encode: hardcode `fetch: -1` | `roundtrip_sort_preserving_merge` **FAILS** | | `SortPreservingMergeExec` encode: `.take(1)` on the sort expressions | `roundtrip_sort_preserving_merge` **FAILS** | | `SortExec` encode: hardcode `fetch: -1` | `roundtrip_sort_with_fetch` **FAILS** | Under all three breaks the pre-existing `roundtrip_sort` and `roundtrip_sort_preserve_partitioning` kept passing — a direct demonstration that the gap was real. All breaks reverted; the diff here is test-only. Checks run: - `cargo fmt --all` - `cargo clippy --all-targets --workspace --features avro,integration-tests,extended_tests -- -D warnings` — clean - `cargo test -p datafusion-proto --test proto_integration` — 216 passed, 0 failed (214 before this PR) - `cargo test -p datafusion-physical-plan` — 1641 passed, 0 failed ### Why this is based on `main` rather than on #24167 These tests pass on unmodified `main` — verified, not assumed. They describe serialization behaviour that already exists, and none of them depends on #24167's changes. That gives a useful property: merged first, they pin the current wire behaviour independently, which makes #24167's "wire format unchanged" claim something CI verifies rather than something the PR description asserts. ## Are there any user-facing changes? No. Test-only; no production code touched. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 2bfdd4a commit 0f68e23

1 file changed

Lines changed: 156 additions & 0 deletions

File tree

‎datafusion/proto/tests/cases/roundtrip_physical_plan.rs‎

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@ use datafusion::physical_plan::scalar_subquery::{
8888
ScalarSubqueryExec, ScalarSubqueryLink,
8989
};
9090
use datafusion::physical_plan::sorts::sort::SortExec;
91+
use datafusion::physical_plan::sorts::sort_preserving_merge::SortPreservingMergeExec;
9192
use datafusion::physical_plan::union::{InterleaveExec, UnionExec};
9293
use datafusion::physical_plan::unnest::{ListUnnest, UnnestExec};
9394
use datafusion::physical_plan::windows::{
@@ -1199,6 +1200,161 @@ fn roundtrip_sort_preserve_partitioning() -> Result<()> {
11991200
))
12001201
}
12011202

1203+
/// `SortExec::fetch` turns a sort into a top-k sort. Losing it during serde
1204+
/// would silently widen the result set, so exercise the `Some(..)` state
1205+
/// explicitly (`roundtrip_sort` only covers `None`).
1206+
///
1207+
/// `SortExec` currently derives `Debug`, so `roundtrip_test`'s
1208+
/// `format!("{plan:?}")` comparison does observe `fetch`. The assertions below
1209+
/// go through the accessor instead so that this coverage does not silently
1210+
/// disappear if `SortExec` ever grows a hand-written `Debug` impl.
1211+
#[test]
1212+
fn roundtrip_sort_with_fetch() -> Result<()> {
1213+
let field_a = Field::new("a", DataType::Boolean, false);
1214+
let field_b = Field::new("b", DataType::Int64, false);
1215+
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
1216+
let sort_exprs: LexOrdering = [
1217+
PhysicalSortExpr {
1218+
expr: col("a", &schema)?,
1219+
options: SortOptions {
1220+
descending: true,
1221+
nulls_first: false,
1222+
},
1223+
},
1224+
PhysicalSortExpr {
1225+
expr: col("b", &schema)?,
1226+
options: SortOptions {
1227+
descending: false,
1228+
nulls_first: true,
1229+
},
1230+
},
1231+
]
1232+
.into();
1233+
1234+
let ctx = SessionContext::new();
1235+
let codec = DefaultPhysicalExtensionCodec {};
1236+
let proto_converter = DefaultPhysicalProtoConverter {};
1237+
1238+
let roundtripped = roundtrip_test_and_return(
1239+
Arc::new(
1240+
SortExec::new(
1241+
sort_exprs.clone(),
1242+
Arc::new(EmptyExec::new(Arc::clone(&schema))),
1243+
)
1244+
.with_fetch(Some(7)),
1245+
),
1246+
&ctx,
1247+
&codec,
1248+
&proto_converter,
1249+
)?;
1250+
let roundtripped = roundtripped
1251+
.downcast_ref::<SortExec>()
1252+
.expect("should decode back into a SortExec");
1253+
assert_eq!(roundtripped.fetch(), Some(7));
1254+
assert_eq!(roundtripped.expr(), &sort_exprs);
1255+
1256+
// `fetch` combined with `preserve_partitioning`, since both share the same
1257+
// proto node.
1258+
let roundtripped = roundtrip_test_and_return(
1259+
Arc::new(
1260+
SortExec::new(sort_exprs.clone(), Arc::new(EmptyExec::new(schema)))
1261+
.with_fetch(Some(3))
1262+
.with_preserve_partitioning(true),
1263+
),
1264+
&ctx,
1265+
&codec,
1266+
&proto_converter,
1267+
)?;
1268+
let roundtripped = roundtripped
1269+
.downcast_ref::<SortExec>()
1270+
.expect("should decode back into a SortExec");
1271+
assert_eq!(roundtripped.fetch(), Some(3));
1272+
assert!(roundtripped.preserve_partitioning());
1273+
Ok(())
1274+
}
1275+
1276+
/// Round trip a [`SortPreservingMergeExec`], which had no dedicated round trip
1277+
/// test at all.
1278+
///
1279+
/// Covers everything that is actually on the wire for this plan: the input, the
1280+
/// sort expressions and `fetch` in both its `None` and `Some(..)` states.
1281+
///
1282+
/// Note that `SortPreservingMergeExec::enable_round_robin_repartition` is
1283+
/// deliberately *not* asserted on here: it has no field in
1284+
/// `SortPreservingMergeExecNode`, so it is not serialized and decoding always
1285+
/// restores the `true` default from `SortPreservingMergeExec::new`. Asserting
1286+
/// round trip equality on it would give a false sense of coverage.
1287+
///
1288+
/// `SortPreservingMergeExec` derives `Debug`, so `roundtrip_test`'s
1289+
/// `format!("{plan:?}")` comparison does observe `expr` and `fetch`. The
1290+
/// assertions below use the accessors so the coverage survives a future
1291+
/// hand-written `Debug` impl.
1292+
#[test]
1293+
fn roundtrip_sort_preserving_merge() -> Result<()> {
1294+
let field_a = Field::new("a", DataType::Boolean, false);
1295+
let field_b = Field::new("b", DataType::Int64, false);
1296+
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
1297+
let sort_exprs: LexOrdering = [
1298+
PhysicalSortExpr {
1299+
expr: col("a", &schema)?,
1300+
options: SortOptions {
1301+
descending: true,
1302+
nulls_first: false,
1303+
},
1304+
},
1305+
PhysicalSortExpr {
1306+
expr: col("b", &schema)?,
1307+
options: SortOptions {
1308+
descending: false,
1309+
nulls_first: true,
1310+
},
1311+
},
1312+
]
1313+
.into();
1314+
1315+
let ctx = SessionContext::new();
1316+
let codec = DefaultPhysicalExtensionCodec {};
1317+
let proto_converter = DefaultPhysicalProtoConverter {};
1318+
1319+
// No fetch: `fetch` is encoded as -1 and must decode back to `None`.
1320+
let roundtripped = roundtrip_test_and_return(
1321+
Arc::new(SortPreservingMergeExec::new(
1322+
sort_exprs.clone(),
1323+
Arc::new(EmptyExec::new(Arc::clone(&schema))),
1324+
)),
1325+
&ctx,
1326+
&codec,
1327+
&proto_converter,
1328+
)?;
1329+
let roundtripped = roundtripped
1330+
.downcast_ref::<SortPreservingMergeExec>()
1331+
.expect("should decode back into a SortPreservingMergeExec");
1332+
assert_eq!(roundtripped.fetch(), None);
1333+
assert_eq!(roundtripped.expr(), &sort_exprs);
1334+
assert_eq!(roundtripped.input().schema(), schema);
1335+
1336+
// With a fetch: dropping it would turn a bounded merge into an unbounded
1337+
// one and change the query result.
1338+
let roundtripped = roundtrip_test_and_return(
1339+
Arc::new(
1340+
SortPreservingMergeExec::new(
1341+
sort_exprs.clone(),
1342+
Arc::new(EmptyExec::new(Arc::clone(&schema))),
1343+
)
1344+
.with_fetch(Some(11)),
1345+
),
1346+
&ctx,
1347+
&codec,
1348+
&proto_converter,
1349+
)?;
1350+
let roundtripped = roundtripped
1351+
.downcast_ref::<SortPreservingMergeExec>()
1352+
.expect("should decode back into a SortPreservingMergeExec");
1353+
assert_eq!(roundtripped.fetch(), Some(11));
1354+
assert_eq!(roundtripped.expr(), &sort_exprs);
1355+
Ok(())
1356+
}
1357+
12021358
#[test]
12031359
fn roundtrip_coalesce_batches_with_fetch() -> Result<()> {
12041360
let field_a = Field::new("a", DataType::Boolean, false);

0 commit comments

Comments
 (0)