Skip to content

Commit 69828a8

Browse files
adriangbclaude
andcommitted
refactor(proto): destructure leaf and pass-through plans in serde hooks
Exhaustive destructures for `CooperativeExec`, `BufferExec`, `EmptyExec`, `PlaceholderRowExec`, `ExplainExec` and `ScalarSubqueryExec` on both sides, plus `ScalarSubqueryLink` on the encode side (its `index` is positional). `EmptyExec`/`PlaceholderRowExec` now read the serialized partition count from the `partitions` field instead of via `cache`; the two are kept in sync by `with_partitions`, so the encoded value is unchanged. Wire format unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent f6dceb6 commit 69828a8

6 files changed

Lines changed: 118 additions & 40 deletions

File tree

‎datafusion/physical-plan/src/buffer.rs‎

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -305,13 +305,24 @@ impl ExecutionPlan for BufferExec {
305305
ctx: &crate::proto::ExecutionPlanEncodeCtx<'_>,
306306
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
307307
use datafusion_proto_models::protobuf;
308-
let input = ctx.encode_child(self.input())?;
308+
// Destructure exhaustively (no `..`) so that adding a field to
309+
// `BufferExec` is a compile error here until it is either serialized or
310+
// explicitly documented as not needing to be.
311+
let Self {
312+
input,
313+
// Derived from the input's properties at construction time.
314+
properties: _,
315+
capacity,
316+
// Runtime metrics, not part of the plan shape.
317+
metrics: _,
318+
} = self;
319+
let input = ctx.encode_child(input)?;
309320
Ok(Some(protobuf::PhysicalPlanNode {
310321
physical_plan_type: Some(
311322
protobuf::physical_plan_node::PhysicalPlanType::Buffer(Box::new(
312323
protobuf::BufferExecNode {
313324
input: Some(Box::new(input)),
314-
capacity: self.capacity() as u64,
325+
capacity: *capacity as u64,
315326
},
316327
)),
317328
),
@@ -336,9 +347,11 @@ impl BufferExec {
336347
protobuf::physical_plan_node::PhysicalPlanType::Buffer,
337348
"BufferExec",
338349
);
339-
let input =
340-
ctx.decode_required_child(buffer.input.as_deref(), "BufferExec", "input")?;
341-
Ok(Arc::new(BufferExec::new(input, buffer.capacity as usize)))
350+
// Destructure exhaustively so that a new field on `BufferExecNode` is a
351+
// compile error here rather than a silently dropped field.
352+
let protobuf::BufferExecNode { input, capacity } = &**buffer;
353+
let input = ctx.decode_required_child(input.as_deref(), "BufferExec", "input")?;
354+
Ok(Arc::new(BufferExec::new(input, *capacity as usize)))
342355
}
343356
}
344357

‎datafusion/physical-plan/src/coop.rs‎

Lines changed: 15 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -376,7 +376,15 @@ impl ExecutionPlan for CooperativeExec {
376376
ctx: &crate::proto::ExecutionPlanEncodeCtx<'_>,
377377
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
378378
use datafusion_proto_models::protobuf;
379-
let input = ctx.encode_child(self.input())?;
379+
// Destructure exhaustively (no `..`) so that adding a field to
380+
// `CooperativeExec` is a compile error here until it is either
381+
// serialized or explicitly documented as not needing to be.
382+
let Self {
383+
input,
384+
// Derived from the input's properties at construction time.
385+
properties: _,
386+
} = self;
387+
let input = ctx.encode_child(input)?;
380388
Ok(Some(protobuf::PhysicalPlanNode {
381389
physical_plan_type: Some(
382390
protobuf::physical_plan_node::PhysicalPlanType::Cooperative(Box::new(
@@ -406,11 +414,12 @@ impl CooperativeExec {
406414
protobuf::physical_plan_node::PhysicalPlanType::Cooperative,
407415
"CooperativeExec",
408416
);
409-
let input = ctx.decode_required_child(
410-
cooperative.input.as_deref(),
411-
"CooperativeExec",
412-
"input",
413-
)?;
417+
// Destructure exhaustively so that a new field on
418+
// `CooperativeExecNode` is a compile error here rather than a silently
419+
// dropped field.
420+
let protobuf::CooperativeExecNode { input } = &**cooperative;
421+
let input =
422+
ctx.decode_required_child(input.as_deref(), "CooperativeExec", "input")?;
414423
Ok(Arc::new(CooperativeExec::new(input)))
415424
}
416425
}

‎datafusion/physical-plan/src/empty.rs‎

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -192,16 +192,22 @@ impl ExecutionPlan for EmptyExec {
192192
_ctx: &crate::proto::ExecutionPlanEncodeCtx<'_>,
193193
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
194194
use datafusion_proto_models::protobuf;
195-
let schema = self.schema().as_ref().try_into()?;
195+
// Destructure exhaustively (no `..`) so that adding a field to
196+
// `EmptyExec` is a compile error here until it is either serialized or
197+
// explicitly documented as not needing to be.
198+
let Self {
199+
schema,
200+
partitions,
201+
// Derived from `schema` and `partitions`, recomputed on decode.
202+
cache: _,
203+
} = self;
204+
let schema = schema.as_ref().try_into()?;
196205
Ok(Some(protobuf::PhysicalPlanNode {
197206
physical_plan_type: Some(
198207
protobuf::physical_plan_node::PhysicalPlanType::Empty(
199208
protobuf::EmptyExecNode {
200209
schema: Some(schema),
201-
partitions: self
202-
.properties()
203-
.output_partitioning()
204-
.partition_count() as u32,
210+
partitions: *partitions as u32,
205211
},
206212
),
207213
),
@@ -222,15 +228,18 @@ impl EmptyExec {
222228
protobuf::physical_plan_node::PhysicalPlanType::Empty,
223229
"EmptyExec",
224230
);
225-
let schema = empty.schema.as_ref().ok_or_else(|| {
231+
// Destructure exhaustively so that a new field on `EmptyExecNode` is a
232+
// compile error here rather than a silently dropped field.
233+
let protobuf::EmptyExecNode { schema, partitions } = empty;
234+
let schema = schema.as_ref().ok_or_else(|| {
226235
datafusion_common::internal_datafusion_err!(
227236
"EmptyExec is missing required field 'schema'"
228237
)
229238
})?;
230239
let schema = Arc::new(arrow::datatypes::Schema::try_from(schema)?);
231240
// A zero (absent) partition count comes from a plan encoded before the
232241
// field existed, which always meant a single partition.
233-
let partitions = empty.partitions.max(1) as usize;
242+
let partitions = (*partitions).max(1) as usize;
234243
Ok(Arc::new(EmptyExec::new(schema).with_partitions(partitions)))
235244
}
236245
}

‎datafusion/physical-plan/src/explain.rs‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -193,17 +193,26 @@ impl ExecutionPlan for ExplainExec {
193193
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
194194
use datafusion_proto_models::protobuf;
195195

196+
// Destructure exhaustively (no `..`) so that adding a field to
197+
// `ExplainExec` is a compile error here until it is either serialized
198+
// or explicitly documented as not needing to be.
199+
let Self {
200+
schema,
201+
stringified_plans,
202+
verbose,
203+
// Derived from `schema`, recomputed on decode.
204+
cache: _,
205+
} = self;
196206
Ok(Some(protobuf::PhysicalPlanNode {
197207
physical_plan_type: Some(
198208
protobuf::physical_plan_node::PhysicalPlanType::Explain(
199209
protobuf::ExplainExecNode {
200-
schema: Some(self.schema().as_ref().try_into()?),
201-
stringified_plans: self
202-
.stringified_plans()
210+
schema: Some(schema.as_ref().try_into()?),
211+
stringified_plans: stringified_plans
203212
.iter()
204213
.map(stringified_plan_to_proto)
205214
.collect(),
206-
verbose: self.verbose(),
215+
verbose: *verbose,
207216
},
208217
),
209218
),
@@ -225,19 +234,25 @@ impl ExplainExec {
225234
protobuf::physical_plan_node::PhysicalPlanType::Explain,
226235
"ExplainExec",
227236
);
228-
let schema = explain.schema.as_ref().ok_or_else(|| {
237+
// Destructure exhaustively so that a new field on `ExplainExecNode` is
238+
// a compile error here rather than a silently dropped field.
239+
let protobuf::ExplainExecNode {
240+
schema,
241+
stringified_plans,
242+
verbose,
243+
} = explain;
244+
let schema = schema.as_ref().ok_or_else(|| {
229245
datafusion_common::internal_datafusion_err!(
230246
"ExplainExec is missing required field 'schema'"
231247
)
232248
})?;
233249
Ok(Arc::new(ExplainExec::new(
234250
Arc::new(arrow::datatypes::Schema::try_from(schema)?),
235-
explain
236-
.stringified_plans
251+
stringified_plans
237252
.iter()
238253
.map(stringified_plan_from_proto)
239254
.collect(),
240-
explain.verbose,
255+
*verbose,
241256
)))
242257
}
243258
}

‎datafusion/physical-plan/src/placeholder_row.rs‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -193,16 +193,22 @@ impl ExecutionPlan for PlaceholderRowExec {
193193
_ctx: &crate::proto::ExecutionPlanEncodeCtx<'_>,
194194
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
195195
use datafusion_proto_models::protobuf;
196-
let schema = self.schema().as_ref().try_into()?;
196+
// Destructure exhaustively (no `..`) so that adding a field to
197+
// `PlaceholderRowExec` is a compile error here until it is either
198+
// serialized or explicitly documented as not needing to be.
199+
let Self {
200+
schema,
201+
partitions,
202+
// Derived from `schema` and `partitions`, recomputed on decode.
203+
cache: _,
204+
} = self;
205+
let schema = schema.as_ref().try_into()?;
197206
Ok(Some(protobuf::PhysicalPlanNode {
198207
physical_plan_type: Some(
199208
protobuf::physical_plan_node::PhysicalPlanType::PlaceholderRow(
200209
protobuf::PlaceholderRowExecNode {
201210
schema: Some(schema),
202-
partitions: self
203-
.properties()
204-
.output_partitioning()
205-
.partition_count() as u32,
211+
partitions: *partitions as u32,
206212
},
207213
),
208214
),
@@ -223,15 +229,19 @@ impl PlaceholderRowExec {
223229
protobuf::physical_plan_node::PhysicalPlanType::PlaceholderRow,
224230
"PlaceholderRowExec",
225231
);
226-
let schema = placeholder.schema.as_ref().ok_or_else(|| {
232+
// Destructure exhaustively so that a new field on
233+
// `PlaceholderRowExecNode` is a compile error here rather than a
234+
// silently dropped field.
235+
let protobuf::PlaceholderRowExecNode { schema, partitions } = placeholder;
236+
let schema = schema.as_ref().ok_or_else(|| {
227237
datafusion_common::internal_datafusion_err!(
228238
"PlaceholderRowExec is missing required field 'schema'"
229239
)
230240
})?;
231241
let schema = Arc::new(Schema::try_from(schema)?);
232242
// A zero (absent) partition count comes from a plan encoded before the
233243
// field existed, which always meant a single partition.
234-
let partitions = placeholder.partitions.max(1) as usize;
244+
let partitions = (*partitions).max(1) as usize;
235245
Ok(Arc::new(
236246
PlaceholderRowExec::new(schema).with_partitions(partitions),
237247
))

‎datafusion/physical-plan/src/scalar_subquery.rs‎

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -262,10 +262,29 @@ impl ExecutionPlan for ScalarSubqueryExec {
262262
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
263263
use datafusion_proto_models::protobuf;
264264

265-
let input = ctx.encode_child(self.input())?;
265+
// Destructure exhaustively (no `..`) so that adding a field to
266+
// `ScalarSubqueryExec` is a compile error here until it is either
267+
// serialized or explicitly documented as not needing to be.
268+
let Self {
269+
input,
270+
subqueries,
271+
// Execution-time one-shot future, created on `execute()`.
272+
subquery_future: _,
273+
// Runtime results container, rebuilt (empty) on decode and shared
274+
// with the input's `ScalarSubqueryExpr` nodes.
275+
results: _,
276+
// Copied from the input's properties, recomputed on decode.
277+
cache: _,
278+
} = self;
279+
let input = ctx.encode_child(input)?;
266280
// Subquery indices are positional and recovered during decoding.
267-
let subqueries =
268-
ctx.encode_children(self.subqueries().iter().map(|subquery| &subquery.plan))?;
281+
let subqueries = ctx.encode_children(subqueries.iter().map(
282+
|ScalarSubqueryLink {
283+
plan,
284+
// Positional: recovered from the element's position on decode.
285+
index: _,
286+
}| plan,
287+
))?;
269288
Ok(Some(protobuf::PhysicalPlanNode {
270289
physical_plan_type: Some(
271290
protobuf::physical_plan_node::PhysicalPlanType::ScalarSubquery(Box::new(
@@ -293,17 +312,20 @@ impl ScalarSubqueryExec {
293312
protobuf::physical_plan_node::PhysicalPlanType::ScalarSubquery,
294313
"ScalarSubqueryExec",
295314
);
296-
let results = ScalarSubqueryResults::new(scalar_subquery.subqueries.len());
297-
let input_node = scalar_subquery.input.as_deref().ok_or_else(|| {
315+
// Destructure exhaustively so that a new field on
316+
// `ScalarSubqueryExecNode` is a compile error here rather than a
317+
// silently dropped field.
318+
let protobuf::ScalarSubqueryExecNode { input, subqueries } = &**scalar_subquery;
319+
let results = ScalarSubqueryResults::new(subqueries.len());
320+
let input_node = input.as_deref().ok_or_else(|| {
298321
datafusion_common::internal_datafusion_err!(
299322
"ScalarSubqueryExec is missing required field 'input'"
300323
)
301324
})?;
302325
// The input's ScalarSubqueryExpr nodes must share this results container.
303326
let input =
304327
ctx.decode_child_with_scalar_subquery_results(input_node, results.clone())?;
305-
let subqueries = scalar_subquery
306-
.subqueries
328+
let subqueries = subqueries
307329
.iter()
308330
.enumerate()
309331
.map(|(index, plan)| {

0 commit comments

Comments
 (0)