Filed with AI assistance (Claude Code); verified against main.
Describe the bug
Decoding a physical plan that contains a ParquetScan or AvroScan node panics when datafusion-proto was built without the corresponding feature, instead of returning a DataFusionError:
PhysicalPlanType::ParquetScan(_) => {
#[cfg(feature = "parquet")]
{
ParquetSource::try_from_proto(self.node(), &decode_ctx)
}
#[cfg(not(feature = "parquet"))]
panic!(
"Unable to process a Parquet PhysicalPlan when `parquet` feature is not enabled"
)
}
The bytes being decoded here come from a peer — that is the entire point of datafusion-proto. A distributed executor built without the parquet feature will abort the process when a scheduler sends it a Parquet scan, rather than failing that one plan and staying up.
This is pre-existing behaviour that was carried over verbatim by the proto-hook migration (#24169 for Parquet, #24190 for Avro), so it is not a regression — just something worth cleaning up now that both arms have the same shape.
Expected behavior
Return an error. not_impl_err! is already the convention elsewhere in the same file for feature-gated paths, e.g.:
not_impl_err!("ParquetSink requires the `parquet` feature")
Both scan arms should follow it:
#[cfg(not(feature = "parquet"))]
not_impl_err!("Unable to process a Parquet PhysicalPlan when the `parquet` feature is not enabled")
Additional context
Sites to fix in datafusion/proto/src/physical_plan/mod.rs:
- the
PhysicalPlanType::ParquetScan decode arm
- the
PhysicalPlanType::AvroScan decode arm
- the same
panic! inside the deprecated try_into_avro_scan_physical_plan compatibility wrapper
While there: the message reads "a Avro PhysicalPlan", which should be "an Avro PhysicalPlan".
Good first issue — the change is mechanical, but note that the arms need to keep type-checking under every combination of the parquet and avro features, so please verify with cargo check -p datafusion-proto (no features), --features parquet, --features avro, and --all-features.
Noticed while reviewing #24190. Part of the epic in #23494.
Describe the bug
Decoding a physical plan that contains a
ParquetScanorAvroScannode panics whendatafusion-protowas built without the corresponding feature, instead of returning aDataFusionError:The bytes being decoded here come from a peer — that is the entire point of
datafusion-proto. A distributed executor built without theparquetfeature will abort the process when a scheduler sends it a Parquet scan, rather than failing that one plan and staying up.This is pre-existing behaviour that was carried over verbatim by the proto-hook migration (#24169 for Parquet, #24190 for Avro), so it is not a regression — just something worth cleaning up now that both arms have the same shape.
Expected behavior
Return an error.
not_impl_err!is already the convention elsewhere in the same file for feature-gated paths, e.g.:Both scan arms should follow it:
Additional context
Sites to fix in
datafusion/proto/src/physical_plan/mod.rs:PhysicalPlanType::ParquetScandecode armPhysicalPlanType::AvroScandecode armpanic!inside the deprecatedtry_into_avro_scan_physical_plancompatibility wrapperWhile there: the message reads "a Avro PhysicalPlan", which should be "an Avro PhysicalPlan".
Good first issue — the change is mechanical, but note that the arms need to keep type-checking under every combination of the
parquetandavrofeatures, so please verify withcargo check -p datafusion-proto(no features),--features parquet,--features avro, and--all-features.Noticed while reviewing #24190. Part of the epic in #23494.