-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add assert on hash children partition count #5768
Conversation
37a8b07
to
7a08cbb
Compare
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 @duongcongtoai -- this is a great idea. It looks like there is a conflict that needs to be resolved, but otherwise this looks great
if self.mode == PartitionMode::Partitioned | ||
&& self.left.output_partitioning() != self.right.output_partitioning() | ||
{ | ||
return Err(DataFusionError::Plan(format!( |
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 likely caused by some sort of code error, so an internal error may make more sense
return Err(DataFusionError::Plan(format!( | |
return Err(DataFusionError::Internal(format!( |
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.
The same comment applies to the code below as well
b6b7653
to
bae8335
Compare
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.
Looks good to me -- thank you @duongcongtoai
Test is failing, i'll check |
I took the liberty to push a fix for the clippy CI failure: https://github.com/apache/arrow-datafusion/actions/runs/4622652579/jobs/8194716556 to your branch in a29658b I also merged up from main |
Which issue does this PR close?
Closes #5738
Rationale for this change
Adding a small validation for join executors to prevent user from providing children inputs having different partition count, causing unexpected behavior
What changes are included in this PR?
Are these changes tested?
Are there any user-facing changes?