physical-plan: decoding an Unnest or Explain node with an unset field panics

Open Beginner friendly
  • Is something wrong?
#26,999 1 comment 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
75/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
rust
Domain
backend

Research direction

Start in datafusion/physical-plan/src/unnest.rs, in UnnestExec::try_from_proto, where recursion.input_column and recursion.output_column are unwrapped. Then look at datafusion/physical-plan/src/explain.rs, at stringified_plan_from_proto and ExplainExec::try_from_proto, where plan_type and plan_type_enum are unwrapped. Done means both paths return an internal_err naming the missing field, with a decode test for each case.

Written by the indexing model from the issue text.

Description

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

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.

Dominant language
Rust
Stars
9.6k
Forks
2.6k
Avg merge
4d 5h
Merged PRs (30d)
319

Getting set up

Open in Codespaces

Starts the project's dev container in your browser, under your own GitHub account.

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from apache/datafusion

All issues in apache/datafusion

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.