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/04/03 19:19:41 UTC
[PR] Remove CoalescePartitions insertion from Joins [datafusion]
ctsk opened a new pull request, #15570:
URL: https://github.com/apache/datafusion/pull/15570
Continuing https://github.com/apache/datafusion/pull/15476 and https://github.com/apache/datafusion/pull/15418
> ## Rationale for this change + What changes are included in this PR?
>
> The logic of HashJoin's `execute` implements differs from the other operators - The recursive call to the `execute` step of the build side are delayed until `collect_left_join` is poll'ed. This PR changes that to make it more alike to the standard implementations of `execute`.
> ## Are these changes tested?
>
> No changes in behaviour are expected.
> ## Are there any user-facing changes?
>
> No
Notable changes:
* Fixed the bug that caused test failures (partition 0 of the left side was executed multiple times)
* Applied the same changes the nested_loop_join and cross_join
--
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 Joins [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#discussion_r2028923212
##########
datafusion/physical-plan/src/joins/cross_join.rs:
##########
@@ -189,19 +188,12 @@ impl CrossJoinExec {
/// Asynchronously collect the result of the left child
async fn load_left_input(
- left: Arc<dyn ExecutionPlan>,
- context: Arc<TaskContext>,
+ stream: SendableRecordBatchStream,
metrics: BuildProbeJoinMetrics,
reservation: MemoryReservation,
) -> Result<JoinLeftData> {
// merge all left parts into a single stream
- let left_schema = left.schema();
- let merge = if left.output_partitioning().partition_count() != 1 {
- Arc::new(CoalescePartitionsExec::new(left))
- } else {
- left
- };
- let stream = merge.execute(0, context)?;
+ let left_schema = stream.schema();
Review Comment:
dead comment above
--
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 Joins [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#discussion_r2031209240
##########
datafusion/physical-plan/src/joins/cross_join.rs:
##########
@@ -282,6 +282,13 @@ impl ExecutionPlan for CrossJoinExec {
partition: usize,
context: Arc<TaskContext>,
) -> Result<SendableRecordBatchStream> {
+ if self.left.output_partitioning().partition_count() != 1 {
Review Comment:
love it
--
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 Joins [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #15570:
URL: https://github.com/apache/datafusion/pull/15570
--
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 Joins [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#issuecomment-2781420170
I wonder if there is any way to write some tests for this (perhaps via `EXPLAIN` in .slt tests to demonstrate that the unecessary exec is removed)
--
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 Joins [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#issuecomment-2781439879
๐ค: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and remove-hj-coalesce
--------------------
Benchmark clickbench_extended.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Query โ HEAD โ remove-hj-coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 0 โ 1983.64ms โ 1974.40ms โ no change โ
โ QQuery 1 โ 699.38ms โ 708.13ms โ no change โ
โ QQuery 2 โ 1421.28ms โ 1456.67ms โ no change โ
โ QQuery 3 โ 708.08ms โ 717.37ms โ no change โ
โ QQuery 4 โ 1510.24ms โ 1496.34ms โ no change โ
โ QQuery 5 โ 17349.43ms โ 17050.26ms โ no change โ
โ QQuery 6 โ 6706.21ms โ 6713.50ms โ no change โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 30378.27ms โ
โ Total Time (remove-hj-coalesce) โ 30116.68ms โ
โ Average Time (HEAD) โ 4339.75ms โ
โ Average Time (remove-hj-coalesce) โ 4302.38ms โ
โ Queries Faster โ 0 โ
โ Queries Slower โ 0 โ
โ Queries with No Change โ 7 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
--------------------
Benchmark clickbench_partitioned.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ remove-hj-coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 0 โ 1.99ms โ 1.92ms โ no change โ
โ QQuery 1 โ 35.20ms โ 34.74ms โ no change โ
โ QQuery 2 โ 91.74ms โ 91.34ms โ no change โ
โ QQuery 3 โ 102.29ms โ 101.18ms โ no change โ
โ QQuery 4 โ 744.08ms โ 740.73ms โ no change โ
โ QQuery 5 โ 877.28ms โ 865.60ms โ no change โ
โ QQuery 6 โ 1.91ms โ 1.94ms โ no change โ
โ QQuery 7 โ 40.83ms โ 39.88ms โ no change โ
โ QQuery 8 โ 936.92ms โ 953.98ms โ no change โ
โ QQuery 9 โ 1256.02ms โ 1281.07ms โ no change โ
โ QQuery 10 โ 275.47ms โ 273.32ms โ no change โ
โ QQuery 11 โ 301.56ms โ 305.70ms โ no change โ
โ QQuery 12 โ 937.22ms โ 942.74ms โ no change โ
โ QQuery 13 โ 1273.43ms โ 1275.80ms โ no change โ
โ QQuery 14 โ 881.06ms โ 881.44ms โ no change โ
โ QQuery 15 โ 1070.13ms โ 1068.40ms โ no change โ
โ QQuery 16 โ 1803.13ms โ 1794.14ms โ no change โ
โ QQuery 17 โ 1638.06ms โ 1651.44ms โ no change โ
โ QQuery 18 โ 3170.64ms โ 3178.40ms โ no change โ
โ QQuery 19 โ 85.90ms โ 87.13ms โ no change โ
โ QQuery 20 โ 1144.96ms โ 1141.99ms โ no change โ
โ QQuery 21 โ 1380.02ms โ 1353.25ms โ no change โ
โ QQuery 22 โ 2504.46ms โ 2484.34ms โ no change โ
โ QQuery 23 โ 8618.09ms โ 8678.11ms โ no change โ
โ QQuery 24 โ 483.76ms โ 477.05ms โ no change โ
โ QQuery 25 โ 393.25ms โ 397.38ms โ no change โ
โ QQuery 26 โ 550.26ms โ 543.41ms โ no change โ
โ QQuery 27 โ 1738.81ms โ 1705.99ms โ no change โ
โ QQuery 28 โ 12715.67ms โ 12666.45ms โ no change โ
โ QQuery 29 โ 530.60ms โ 550.06ms โ no change โ
โ QQuery 30 โ 868.66ms โ 848.77ms โ no change โ
โ QQuery 31 โ 913.96ms โ 899.26ms โ no change โ
โ QQuery 32 โ 2762.98ms โ 2813.21ms โ no change โ
โ QQuery 33 โ 3447.68ms โ 3417.70ms โ no change โ
โ QQuery 34 โ 3430.75ms โ 3448.18ms โ no change โ
โ QQuery 35 โ 1331.45ms โ 1316.45ms โ no change โ
โ QQuery 36 โ 159.48ms โ 154.87ms โ no change โ
โ QQuery 37 โ 88.14ms โ 85.47ms โ no change โ
โ QQuery 38 โ 149.12ms โ 153.57ms โ no change โ
โ QQuery 39 โ 227.91ms โ 228.93ms โ no change โ
โ QQuery 40 โ 77.19ms โ 79.93ms โ no change โ
โ QQuery 41 โ 76.35ms โ 72.23ms โ +1.06x faster โ
โ QQuery 42 โ 65.96ms โ 67.20ms โ no change โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 59184.35ms โ
โ Total Time (remove-hj-coalesce) โ 59154.69ms โ
โ Average Time (HEAD) โ 1376.38ms โ
โ Average Time (remove-hj-coalesce) โ 1375.69ms โ
โ Queries Faster โ 1 โ
โ Queries Slower โ 0 โ
โ Queries with No Change โ 42 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
--------------------
Benchmark tpch_mem_sf1.json
--------------------
โโโโโโโโโโโโโโโโณโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ remove-hj-coalesce โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 1 โ 121.75ms โ 123.41ms โ no change โ
โ QQuery 2 โ 24.15ms โ 24.22ms โ no change โ
โ QQuery 3 โ 35.77ms โ 36.30ms โ no change โ
โ QQuery 4 โ 21.25ms โ 21.42ms โ no change โ
โ QQuery 5 โ 57.69ms โ 58.52ms โ no change โ
โ QQuery 6 โ 8.21ms โ 8.27ms โ no change โ
โ QQuery 7 โ 110.68ms โ 104.15ms โ +1.06x faster โ
โ QQuery 8 โ 26.96ms โ 26.31ms โ no change โ
โ QQuery 9 โ 63.25ms โ 63.22ms โ no change โ
โ QQuery 10 โ 61.03ms โ 61.83ms โ no change โ
โ QQuery 11 โ 13.05ms โ 13.21ms โ no change โ
โ QQuery 12 โ 37.51ms โ 39.51ms โ 1.05x slower โ
โ QQuery 13 โ 29.16ms โ 29.59ms โ no change โ
โ QQuery 14 โ 9.79ms โ 10.05ms โ no change โ
โ QQuery 15 โ 25.66ms โ 25.54ms โ no change โ
โ QQuery 16 โ 24.00ms โ 24.33ms โ no change โ
โ QQuery 17 โ 98.05ms โ 96.97ms โ no change โ
โ QQuery 18 โ 249.89ms โ 249.44ms โ no change โ
โ QQuery 19 โ 28.19ms โ 28.38ms โ no change โ
โ QQuery 20 โ 39.71ms โ 40.64ms โ no change โ
โ QQuery 21 โ 176.95ms โ 169.94ms โ no change โ
โ QQuery 22 โ 17.44ms โ 18.33ms โ 1.05x slower โ
โโโโโโโโโโโโโโโโดโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 1280.14ms โ
โ Total Time (remove-hj-coalesce) โ 1273.58ms โ
โ Average Time (HEAD) โ 58.19ms โ
โ Average Time (remove-hj-coalesce) โ 57.89ms โ
โ Queries Faster โ 1 โ
โ Queries Slower โ 2 โ
โ Queries with No Change โ 19 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
```
</p>
</details>
--
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 Joins [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#issuecomment-2781430463
I see -- this is code simplification ๐
--
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 Joins [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#issuecomment-2781421263
๐ค `./gh_compare_branch.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking) Running
Linux aal-dev 6.8.0-1016-gcp #18-Ubuntu SMP Fri Oct 4 22:16:29 UTC 2024 x86_64 x86_64 x86_64 GNU/Linux
Comparing remove-hj-coalesce (995a95cc1e30f1a96a87a7a4eece50284a9af641) to 0b061bef32bc09345814153d6eb7c737695bea9a [diff](https://github.com/apache/datafusion/compare/0b061bef32bc09345814153d6eb7c737695bea9a..995a95cc1e30f1a96a87a7a4eece50284a9af641)
Benchmarks: tpch_mem clickbench_partitioned clickbench_extended
Results will be posted here when complete
--
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 Joins [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#issuecomment-2781422765
> > I wonder if there is any way to write some tests for this (perhaps via `EXPLAIN` in .slt tests to demonstrate that the unecessary exec is removed)
>
> I don't think a redundant exec can be removed since if the input is 1 partition, it was already not introducing a new one, and what we see is input is always 1 (enforce distribution does its job ๐๐ป), because we don't get any plan 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 Joins [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#issuecomment-2781567330
TLDR is benchmark results look good to me -- 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 Joins [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #15570:
URL: https://github.com/apache/datafusion/pull/15570#issuecomment-2781422523
> I wonder if there is any way to write some tests for this (perhaps via `EXPLAIN` in .slt tests to demonstrate that the unecessary exec is removed)
I don't think a redundant exec can be removed since if the input is 1 partition, it already does not introduce a new one, and what we see is input is always 1 (enforce distribution does its job ๐๐ป), because we don't get any plan 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