datafusion-proto: decoding an AvroScan node without the avro feature panics

Open Beginner friendly
  • Is something wrong?
#26,998 0 comments 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 1 day

Nobody has claimed this yet.

Assessment

Difficulty
1/5
Estimated time
Under an hour
Newbie friendliness
85/100
Issue type
Bug
Clarity
Clearly specified
Activity status
Active
Tech stack
rust
Domain
databases

Research direction

The panic is the #[cfg(not(feature = "avro"))] arm for AvroScan in datafusion/proto/src/physical_plan/mod.rs, around lines 1238-1247. Copy the ParquetScan arm just above it, which returns not_impl_err! for the same case. Done means decoding an AvroScan node with default features returns Err instead of panicking, which a new test built without the avro feature can check.

Written by the indexing model from the issue text.

Description

Describe the bug

datafusion-proto is built without avro by default, since its default features are ["parquet"]. In that build, PhysicalPlanNode::try_into_physical_plan panics on an AvroScan node:

https://github.com/apache/datafusion/blob/934691e9af8f11bf4b629f21bb17795abfb84d93/datafusion/proto/src/physical_plan/mod.rs#L1238-L1247

#[cfg(not(feature = "avro"))]
panic!(
    "Unable to process a Avro PhysicalPlan when `avro` feature is not enabled"
)

The ParquetScan arm just above it handles the same situation with not_impl_err!:

https://github.com/apache/datafusion/blob/934691e9af8f11bf4b629f21bb17795abfb84d93/datafusion/proto/src/physical_plan/mod.rs#L1228-L1237

The same code is in 55.1.0, src/physical_plan/mod.rs, lines 1100-1108.

To Reproduce

Build datafusion-proto with default features, then decode a PhysicalPlanNode whose physical_plan_type is AvroScan(Default::default()):

let node = protobuf::PhysicalPlanNode {
    physical_plan_type: Some(protobuf::physical_plan_node::PhysicalPlanType::AvroScan(Default::default())),
};
let _ = datafusion_proto::bytes::physical_plan_from_bytes(&node.encode_to_vec(), &ctx.task_ctx());

The arm is an unconditional panic!, so this panics before the node's contents are looked at.

Expected behavior

Return not_impl_err!, as the ParquetScan arm does, so a caller gets Err and keeps running.

Additional context

This matters wherever a process decodes plans it did not encode itself. One example is a distributed worker that receives plan stages from peers. A few bytes of input panic the decoding task, and under panic = "abort" they stop the whole process.

A fix is one line, plus a decode test with the feature off. I can open the PR if that is welcome.

Dominant language
Rust
Stars
9.6k
Forks
2.6k
Avg merge
3d 22h
Merged PRs (30d)
331

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.