geoHeil opened a new issue, #26999: URL: https://github.com/apache/datafusion/issues/26999
### Describe the bug Two physical plan decoders in `datafusion-physical-plan` panic on a field the wire leaves unset. Their neighbours return an error in the same situation. `UnnestExec::try_from_proto` unwraps both columns of each recursion option: https://github.com/apache/datafusion/blob/934691e9af8f11bf4b629f21bb17795abfb84d93/datafusion/physical-plan/src/unnest.rs#L468-L469 ```rust input_column: recursion.input_column.as_ref().unwrap().into(), output_column: recursion.output_column.as_ref().unwrap().into(), ``` `RecursionUnnestOption.input_column` and `.output_column` are `optional` messages, so a peer can leave either one unset. A few lines earlier, the same function refuses a missing `options` with `internal_datafusion_err!`: https://github.com/apache/datafusion/blob/934691e9af8f11bf4b629f21bb17795abfb84d93/datafusion/physical-plan/src/unnest.rs#L443-L447 `ExplainExec::try_from_proto` maps each stringified plan through `stringified_plan_from_proto`. That function panics when `plan_type`, or its `plan_type_enum`, is unset: https://github.com/apache/datafusion/blob/934691e9af8f11bf4b629f21bb17795abfb84d93/datafusion/physical-plan/src/explain.rs#L357-L365 `ExplainExec::try_from_proto` itself refuses a missing `schema` with an error: https://github.com/apache/datafusion/blob/934691e9af8f11bf4b629f21bb17795abfb84d93/datafusion/physical-plan/src/explain.rs#L253-L257 The same code is in 55.1.0, at `src/unnest.rs` lines 473-474 and `src/explain.rs` lines 355-363. ### To Reproduce Decode a `PhysicalPlanNode` with `datafusion_proto::bytes::physical_plan_from_bytes`, where the node is one of: - an `Explain` node with a `schema` and one `StringifiedPlan::default()` in `stringified_plans`, so `plan_type` is `None`; - an `Unnest` node with a decodable `input` and `schema`, and `options.recursions` holding one `RecursionUnnestOption::default()`, so `input_column` is `None`. Both reach the panic without any earlier check failing. I found these by reading the decode path and have not run these exact inputs here yet. I will add the panic output once a decode test on our side has run. ### Expected behavior Return `internal_err!` naming the missing field, as the sibling checks do. The caller then gets `Err` and keeps running. ### Additional context This matters wherever a process decodes plans it did not encode itself, such as a distributed worker that receives plan stages from peers. A malformed node panics the decoding task, and under `panic = "abort"` it stops the whole process. A related case in `datafusion-proto` is #26998. The fix is two `ok_or_else` calls plus a decode test for each. I can open the PR if that is welcome. -- 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]
