You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "ctsk (via GitHub)" <gi...@apache.org> on 2025/03/28 13:20:54 UTC
[PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
ctsk opened a new pull request, #15476:
URL: https://github.com/apache/datafusion/pull/15476
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
- Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2764497395
This PR appears to have caused CI failures for some reason so @goldmedal has a PR to revert it:
- https://github.com/apache/datafusion/pull/15496
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "ctsk (via GitHub)" <gi...@apache.org>.
ctsk commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2762368030
I've amended the PR so that `Executionplan::execute` fails if one tries to execute such a problematic plan.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan merged PR #15476:
URL: https://github.com/apache/datafusion/pull/15476
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "ctsk (via GitHub)" <gi...@apache.org>.
ctsk commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2762302514
Before this PR, if someone hand-wired a CollectLeft HashJoin where the left child has more than one output partition, the HashJoin would automatically add a CoalescePartitions exec. This behaviour never triggers for plans that are constructed by datafusion, because the EnforceDistribution pass makes sure that that CoalescePartitions exist.
After this PR, if someone hand-wires a CollectLeft HashJoin and does not use the EnforceDistribution pass and provides a left child that has more than 1 output partition, the resulting plan, when executed will return a wrong result (because the hash table will only built on partition 0).
Now that I've written it out. I believe it is strictly better to return a plan_err! if one constructs such a plan rather than return a wrong result.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2763855444
Thanks @ctsk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "ctsk (via GitHub)" <gi...@apache.org>.
ctsk commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2761891586
Note that this does break for users of HashJoinExec that
- Use the CollectLeft mode, with >1 partition on the build side AND
- Construct their physical plan without running EnforceDistribution
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "ctsk (via GitHub)" <gi...@apache.org>.
ctsk commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2772417654
> > Alright, so what went wrong here is that for CollectLeft joins, the left ExecutionPlan gets executed for every input partition.
> The issue is now CollectLeft should always receive only 1 partition, by EnforceDistribution, right? Then, we can re-apply these changes
I typo'd there - I meant to say *for every output partition*.
The bug is not in the removal of the CoalescePartitions, but the changes that I did prior to that (lifting the `execute` of the build side out of the `OnceFut` - this commit: https://github.com/apache/datafusion/pull/15476/commits/da06eff8feaa557b9508553ea97c029254c1ef76)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2772106637
> This was not caught by the tests 2 weeks ago, because most plans can have a partition executed multiple times - but repartition can not.
As the signature of execute() is not self consuming, what I believe is all execute()'s should be idempotent. If repartition breaks this, should we create a ticket for that?
> I suspect there is no case where a CollectLeft join is used and a RepartitionExec is under the build side.
Even if this case is not generated in tests at all, we should be prepared for that.
> Alright, so what went wrong here is that for CollectLeft joins, the left ExecutionPlan gets executed _for every input partition_.
The issue is now CollectLeft should always receive only 1 partition, by EnforceDistribution, right? Then, we can re-apply these changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "ctsk (via GitHub)" <gi...@apache.org>.
ctsk commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2764514013
Sorry about that! Thanks for tracking it down @goldmedal.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "ctsk (via GitHub)" <gi...@apache.org>.
ctsk commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2769354999
Alright, so what went wrong here is that for CollectLeft joins, the left ExecutionPlan gets executed *for every input partition*.
This was not caught by the tests 2 weeks ago, because most plans can have a partition executed multiple times - but repartition can not. I suspect there is no case where a CollectLeft join is used and a RepartitionExec is under the build side.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "goldmedal (via GitHub)" <gi...@apache.org>.
goldmedal commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2764514075
> This PR appears to have caused CI failures for some reason so @goldmedal has a PR to revert it:
>
> * [Revert #15476 to fix the datafusion-examples CI fail #15496](https://github.com/apache/datafusion/pull/15496)
It can be reproduced in the local by the following steps:
- Checkout to the commit in the man branch: `git checkout 7e0738a979054e95d1935d6e3e58b27385679031`
- run the datafame example: `cargo run --package datafusion-examples --example dataframe`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Remove CoalescePartitions insertion from HashJoinExec [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #15476:
URL: https://github.com/apache/datafusion/pull/15476#issuecomment-2762263935
> Note that this does break for users of HashJoinExec that
>
> * Use the CollectLeft mode, with >1 partition on the build side AND
> * Construct their physical plan without running EnforceDistribution
Thanks @ctsk what exactly is broken?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org