-
Notifications
You must be signed in to change notification settings - Fork 1.6k
fix: prevent UnionExec panic with empty inputs #17449
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: prevent UnionExec panic with empty inputs #17449
Conversation
This commit fixes a panic in UnionExec when constructed with empty inputs. Previously, UnionExec::new(vec![]) would cause an index out of bounds panic at union.rs:542 when trying to access inputs[0]. Changes: - Made UnionExec::new() return Result<Self> with proper validation - Made union_schema() return Result<SchemaRef> with empty input checks - Added descriptive error messages for empty input cases - Updated all call sites to handle the new Result return type - Added comprehensive tests for edge cases Error messages: - "UnionExec requires at least one input" - "Cannot create union schema from empty inputs" The fix maintains backward compatibility for valid inputs while preventing crashes and providing clear error messages for invalid usage. Fixes apache#17052
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @EeshanBembi
fn test_union_empty_inputs() { | ||
// Test that UnionExec::new fails with empty inputs | ||
let result = UnionExec::new(vec![]); | ||
assert!(result.is_err()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the assertion check for is_err is redundant as unwrap_err
will panic if result is not an err
@@ -101,19 +101,23 @@ pub struct UnionExec { | |||
|
|||
impl UnionExec { | |||
/// Create a new UnionExec | |||
pub fn new(inputs: Vec<Arc<dyn ExecutionPlan>>) -> Self { | |||
let schema = union_schema(&inputs); | |||
pub fn new(inputs: Vec<Arc<dyn ExecutionPlan>>) -> Result<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is technically an API change -- maybe to make it easier on others, we can make a new function called try_new
that has the error checking, and deprecate the existing new
function per https://datafusion.apache.org/contributor-guide/api-health.html#deprecation-guidelines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on the API lifecycle. On separate note, can we make the new try_new
method return Box<<dyn ExecutionPlan>>
? This would allow it to return the only child in case input vector is a singleton. There is no point keeping UnionExec(a)
in the plan.
Or maybe, the new method can simply require the input to have at least two elements?
@@ -101,19 +101,23 @@ pub struct UnionExec { | |||
|
|||
impl UnionExec { | |||
/// Create a new UnionExec | |||
pub fn new(inputs: Vec<Arc<dyn ExecutionPlan>>) -> Self { | |||
let schema = union_schema(&inputs); | |||
pub fn new(inputs: Vec<Arc<dyn ExecutionPlan>>) -> Result<Self> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point on the API lifecycle. On separate note, can we make the new try_new
method return Box<<dyn ExecutionPlan>>
? This would allow it to return the only child in case input vector is a singleton. There is no point keeping UnionExec(a)
in the plan.
Or maybe, the new method can simply require the input to have at least two elements?
} | ||
|
||
#[test] | ||
fn test_union_multiple_inputs_still_works() -> Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fn test_union_multiple_inputs_still_works() -> Result<()> { | |
fn test_union_schema_multiple_inputs() -> Result<()> { |
Not related to the PR, but it'll be better for df to have a optimizer rule to remove empty inputs from union |
@xudong963 , exactly See also #17449 (comment) |
Summary
This PR fixes a panic in
UnionExec
when constructed with empty inputs, replacing the crash with proper error handling and descriptive error messages.Fixes: #17052
Problem
When
UnionExec::new(vec![])
was called with an empty input vector, it would panic with:This occurred because
union_schema()
directly accessedinputs[0]
without checking if the array was empty.Solution
Core Changes
Made
UnionExec::new()
returnResult<Self>
:inputs.is_empty()
"UnionExec requires at least one input"
Made
union_schema()
returnResult<SchemaRef>
:inputs[0]
"Cannot create union schema from empty inputs"
Updated all call sites (7 files):
physical_planner.rs
- Core DataFusion integrationrepartition/mod.rs
- Internal dependenciesResult
return typeError Handling
Testing
Added 4 comprehensive tests:
test_union_empty_inputs()
- Verifies empty input validationtest_union_schema_empty_inputs()
- Tests schema creation with empty inputstest_union_single_input()
- Ensures single input still workstest_union_multiple_inputs_still_works()
- Verifies existing functionality unchangedTest Results:
Backward Compatibility
UnionExec::new()
now returnsResult<Self>
instead ofSelf
This is a breaking change but justified because:
Union
which requires ≥2 inputsFiles Changed
datafusion/physical-plan/src/union.rs
- Core fix + tests (main changes)datafusion/core/src/physical_planner.rs
- HandleResult
returndatafusion/physical-plan/src/repartition/mod.rs
- Update internal callsThe fix provides robust error handling while maintaining all existing functionality for valid use cases.