kosiew commented on code in PR #24167:
URL: https://github.com/apache/datafusion/pull/24167#discussion_r3830330074
##########
datafusion/physical-plan/src/filter.rs:
##########
@@ -850,28 +850,43 @@ impl ExecutionPlan for FilterExec {
ctx: &crate::proto::ExecutionPlanEncodeCtx<'_>,
) -> Result<Option<datafusion_proto_models::protobuf::PhysicalPlanNode>> {
use datafusion_proto_models::protobuf;
- let input = ctx.encode_child(self.input())?;
- let expr = ctx.encode_expr(self.predicate())?;
+ // Destructure exhaustively (no `..`) so that adding a field to
+ // `FilterExec` is a compile error here until it is either serialized
or
+ // explicitly documented as not needing to be.
+ let Self {
+ predicate,
+ input,
+ // Runtime metrics, not part of the plan shape.
+ metrics: _,
+ default_selectivity,
+ // Derived plan properties, recomputed on decode.
+ cache: _,
+ projection,
+ batch_size,
+ fetch,
+ } = self;
+ let input_node = ctx.encode_child(input)?;
+ let expr = ctx.encode_expr(predicate)?;
// Preserve the exact wire format: `None` (full projection) is
serialized
Review Comment:
Small non-blocking suggestion on this pre-existing wording: `None` is
encoded as the identity projection, but an explicit identity projection is also
decoded back to `None`, so the two forms aren't actually distinguishable on
decode. Could we reword this to describe the identity projection as the
canonical wire representation for a full projection instead?
This is separate from the roundtrip coverage and omitted-field follow-ups
already being discussed.
--
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]
