You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jonathanc-n (via GitHub)" <gi...@apache.org> on 2026/02/09 05:02:43 UTC
[PR] feat: Push limit into hash join [datafusion]
jonathanc-n opened a new pull request, #20228:
URL: https://github.com/apache/datafusion/pull/20228
## 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.
-->
- Part of #18295.
## 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?
Push limit down into hash join using limit pushdown optimizer. Use limit pushdown optimizer to pass the limit value to Hash Join exec using `with_fetch` and passing the `fetch` value to `LimitedBatch Coalescer` to emit the batch once the limit is hit.
<!--
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?
SLT tests + unit tests
--
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] feat: Push limit into hash join [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870524230
run benchmark tpcds
--
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] feat: Push limit into hash join [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2787963863
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
I think it would be better to make those resets explicit and avoid builders with hidden semantics.
However this pattern has already propagated in the codebase, so I'd like to continue the discussion in the original PR and see whether we can clean this up or remove such patterns over time: https://github.com/apache/datafusion/pull/19893#discussion_r2787434025
--
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] feat: Push limit into hash join [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870121846
๐ค `./gh_compare_branch.sh` [gh_compare_branch.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_branch.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing push-limit-into-join (832a79c22548ff273e375910fae9ce965e5929fb) to 0fd6a40d4eef274b8661a6158c62df12bab28204 [diff](https://github.com/apache/datafusion/compare/0fd6a40d4eef274b8661a6158c62df12bab28204..832a79c22548ff273e375910fae9ce965e5929fb) using: 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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783439428
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1506,53 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ // Null-aware anti join requires seeing ALL probe rows to check for NULLs.
+ // If any probe row has NULL, the output must be empty.
+ // We can't stop early or we might miss a NULL and return wrong results.
+ if self.null_aware {
Review Comment:
If we have two tables like:
table 1 join column: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
table 2 join column: [1, 2, 3, 4, 5, NULL]
If we do a LIMIT = 5, when probing, we find 5 non matches then the limited batch coalescer will push the final batch before we hit the NULL.
--
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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783439428
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1506,53 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ // Null-aware anti join requires seeing ALL probe rows to check for NULLs.
+ // If any probe row has NULL, the output must be empty.
+ // We can't stop early or we might miss a NULL and return wrong results.
+ if self.null_aware {
Review Comment:
If we have two tables like:
table 1 join column: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
table 2 join column: [1, 2, 3, 4, 5, NULL]
If we do a LIMIT = 5, when probing, we find 5 non matches then the limited batch coalescer will push the final batch before we hit the NULL.
--
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] feat: Push limit into hash join [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2787304528
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
(This is my major concern, once clarified, I think this PR is good to go)
--
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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2789145969
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
I'm confused, should we leave the current implementation as it is then?
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
Yes that makes sense, I can change to your recommendation
--
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] feat: Push limit into hash join [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2788247999
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
Builders were added to have an ability to build `HashJoinExec` using:
1. `projection: Option<Vec<usize>>` (not break existing `try_new` API)
2. shared projection `Option<ProjectionRef>` (to share projection if exec is created from another one and the projection is preserved).
The reason to not use `impl Option<Into<ProjectionRef>>` as `try_new` arg here: it requires to specify `None` type explicitly when pass `None`. The reason to not add another `try_new_with_shared_projection`: it looks confusing as we don't want to create a new constructor each time when some arg need to be either one thing or another.
If we keep the builder, to reduce hidden semantics as much as possible, it should keep all existing fields when constructed from the existing exec, allowing customizing things that must be rest. I will make the patch for 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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783366032
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
I forgot to add it, thanks for catching this.
--
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] feat: Push limit into hash join [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 merged PR #20228:
URL: https://github.com/apache/datafusion/pull/20228
--
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] feat: Push limit into hash join [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870605760
๐ค: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and push-limit-into-join
--------------------
Benchmark tpcds_sf1.json
--------------------
โโโโโโโโโโโโโณโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ push-limit-into-join โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 1 โ 74.01 ms โ 73.64 ms โ no change โ
โ QQuery 2 โ 218.55 ms โ 214.58 ms โ no change โ
โ QQuery 3 โ 158.94 ms โ 156.40 ms โ no change โ
โ QQuery 4 โ 2087.81 ms โ 1887.41 ms โ +1.11x faster โ
โ QQuery 5 โ 276.79 ms โ 261.07 ms โ +1.06x faster โ
โ QQuery 6 โ 1517.13 ms โ 1451.95 ms โ no change โ
โ QQuery 7 โ 526.79 ms โ 507.28 ms โ no change โ
โ QQuery 8 โ 174.17 ms โ 168.47 ms โ no change โ
โ QQuery 9 โ 300.14 ms โ 295.72 ms โ no change โ
โ QQuery 10 โ 178.79 ms โ 173.65 ms โ no change โ
โ QQuery 11 โ 1408.62 ms โ 1298.06 ms โ +1.09x faster โ
โ QQuery 12 โ 73.38 ms โ 66.72 ms โ +1.10x faster โ
โ QQuery 13 โ 580.59 ms โ 544.10 ms โ +1.07x faster โ
โ QQuery 14 โ 1948.30 ms โ 1875.87 ms โ no change โ
โ QQuery 15 โ 28.63 ms โ 27.83 ms โ no change โ
โ QQuery 16 โ 66.28 ms โ 62.35 ms โ +1.06x faster โ
โ QQuery 17 โ 371.07 ms โ 360.23 ms โ no change โ
โ QQuery 18 โ 198.97 ms โ 196.22 ms โ no change โ
โ QQuery 19 โ 220.37 ms โ 216.96 ms โ no change โ
โ QQuery 20 โ 25.18 ms โ 24.18 ms โ no change โ
โ QQuery 21 โ 37.82 ms โ 35.39 ms โ +1.07x faster โ
โ QQuery 22 โ 732.48 ms โ 693.12 ms โ +1.06x faster โ
โ QQuery 23 โ 1822.17 ms โ 1785.00 ms โ no change โ
โ QQuery 24 โ 706.52 ms โ 685.62 ms โ no change โ
โ QQuery 25 โ 538.78 ms โ 523.63 ms โ no change โ
โ QQuery 26 โ 127.33 ms โ 129.05 ms โ no change โ
โ QQuery 27 โ 524.55 ms โ 514.22 ms โ no change โ
โ QQuery 28 โ 302.84 ms โ 296.28 ms โ no change โ
โ QQuery 29 โ 463.46 ms โ 448.81 ms โ no change โ
โ QQuery 30 โ 79.65 ms โ 75.00 ms โ +1.06x faster โ
โ QQuery 31 โ 314.79 ms โ 298.55 ms โ +1.05x faster โ
โ QQuery 32 โ 82.27 ms โ 81.98 ms โ no change โ
โ QQuery 33 โ 212.55 ms โ 207.65 ms โ no change โ
โ QQuery 34 โ 165.51 ms โ 159.17 ms โ no change โ
โ QQuery 35 โ 184.64 ms โ 178.59 ms โ no change โ
โ QQuery 36 โ 295.92 ms โ 289.18 ms โ no change โ
โ QQuery 37 โ 265.96 ms โ 256.91 ms โ no change โ
โ QQuery 38 โ 160.60 ms โ 151.99 ms โ +1.06x faster โ
โ QQuery 39 โ 210.19 ms โ 194.86 ms โ +1.08x faster โ
โ QQuery 40 โ 183.17 ms โ 181.02 ms โ no change โ
โ QQuery 41 โ 26.39 ms โ 25.72 ms โ no change โ
โ QQuery 42 โ 148.74 ms โ 144.53 ms โ no change โ
โ QQuery 43 โ 131.06 ms โ 126.84 ms โ no change โ
โ QQuery 44 โ 30.51 ms โ 28.29 ms โ +1.08x faster โ
โ QQuery 45 โ 86.53 ms โ 84.01 ms โ no change โ
โ QQuery 46 โ 338.79 ms โ 325.59 ms โ no change โ
โ QQuery 47 โ 1137.83 ms โ 1031.50 ms โ +1.10x faster โ
โ QQuery 48 โ 435.78 ms โ 401.61 ms โ +1.09x faster โ
โ QQuery 49 โ 384.38 ms โ 384.59 ms โ no change โ
โ QQuery 50 โ 351.58 ms โ 322.87 ms โ +1.09x faster โ
โ QQuery 51 โ 317.45 ms โ 305.58 ms โ no change โ
โ QQuery 52 โ 149.41 ms โ 144.42 ms โ no change โ
โ QQuery 53 โ 148.96 ms โ 144.65 ms โ no change โ
โ QQuery 54 โ 212.67 ms โ 202.95 ms โ no change โ
โ QQuery 55 โ 149.09 ms โ 144.06 ms โ no change โ
โ QQuery 56 โ 214.57 ms โ 205.36 ms โ no change โ
โ QQuery 57 โ 301.96 ms โ 289.26 ms โ no change โ
โ QQuery 58 โ 511.13 ms โ 496.38 ms โ no change โ
โ QQuery 59 โ 300.58 ms โ 287.80 ms โ no change โ
โ QQuery 60 โ 216.67 ms โ 210.82 ms โ no change โ
โ QQuery 61 โ 258.99 ms โ 249.33 ms โ no change โ
โ QQuery 62 โ 1336.11 ms โ 1302.56 ms โ no change โ
โ QQuery 63 โ 148.63 ms โ 145.51 ms โ no change โ
โ QQuery 64 โ 1197.94 ms โ 1185.23 ms โ no change โ
โ QQuery 65 โ 372.59 ms โ 354.06 ms โ no change โ
โ QQuery 66 โ 393.62 ms โ 392.28 ms โ no change โ
โ QQuery 67 โ 547.88 ms โ 532.76 ms โ no change โ
โ QQuery 68 โ 395.19 ms โ 378.29 ms โ no change โ
โ QQuery 69 โ 173.46 ms โ 169.22 ms โ no change โ
โ QQuery 70 โ 501.92 ms โ 492.31 ms โ no change โ
โ QQuery 71 โ 186.62 ms โ 186.64 ms โ no change โ
โ QQuery 72 โ 2150.72 ms โ 2100.26 ms โ no change โ
โ QQuery 73 โ 156.61 ms โ 155.17 ms โ no change โ
โ QQuery 74 โ 851.79 ms โ 828.27 ms โ no change โ
โ QQuery 75 โ 413.46 ms โ 412.01 ms โ no change โ
โ QQuery 76 โ 187.05 ms โ 187.06 ms โ no change โ
โ QQuery 77 โ 285.51 ms โ 279.73 ms โ no change โ
โ QQuery 78 โ 690.91 ms โ 680.51 ms โ no change โ
โ QQuery 79 โ 336.48 ms โ 324.28 ms โ no change โ
โ QQuery 80 โ 544.55 ms โ 526.71 ms โ no change โ
โ QQuery 81 โ 52.54 ms โ 52.26 ms โ no change โ
โ QQuery 82 โ 282.16 ms โ 278.39 ms โ no change โ
โ QQuery 83 โ 80.82 ms โ 78.44 ms โ no change โ
โ QQuery 84 โ 70.61 ms โ 69.90 ms โ no change โ
โ QQuery 85 โ 227.02 ms โ 221.46 ms โ no change โ
โ QQuery 86 โ 59.38 ms โ 58.81 ms โ no change โ
โ QQuery 87 โ 159.55 ms โ 153.66 ms โ no change โ
โ QQuery 88 โ 267.88 ms โ 256.25 ms โ no change โ
โ QQuery 89 โ 169.87 ms โ 166.59 ms โ no change โ
โ QQuery 90 โ 47.40 ms โ 44.54 ms โ +1.06x faster โ
โ QQuery 91 โ 98.18 ms โ 95.84 ms โ no change โ
โ QQuery 92 โ 82.17 ms โ 81.56 ms โ no change โ
โ QQuery 93 โ 291.48 ms โ 277.20 ms โ no change โ
โ QQuery 94 โ 91.19 ms โ 89.78 ms โ no change โ
โ QQuery 95 โ 247.16 ms โ 239.82 ms โ no change โ
โ QQuery 96 โ 116.96 ms โ 115.05 ms โ no change โ
โ QQuery 97 โ 201.50 ms โ 194.47 ms โ no change โ
โ QQuery 98 โ 226.07 ms โ 220.06 ms โ no change โ
โ QQuery 99 โ 14374.19 ms โ 14327.41 ms โ no change โ
โโโโโโโโโโโโโดโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 51716.01ms โ
โ Total Time (push-limit-into-join) โ 50259.21ms โ
โ Average Time (HEAD) โ 522.38ms โ
โ Average Time (push-limit-into-join) โ 507.67ms โ
โ Queries Faster โ 17 โ
โ Queries Slower โ 0 โ
โ Queries with No Change โ 82 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
```
</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] feat: Push limit into hash join [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870357808
๐ค: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and push-limit-into-join
--------------------
Benchmark clickbench_extended.json
--------------------
โโโโโโโโโโโโณโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Query โ HEAD โ push-limit-into-join โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 0 โ 2241.20 ms โ 2288.83 ms โ no change โ
โ QQuery 1 โ 916.75 ms โ 911.64 ms โ no change โ
โ QQuery 2 โ 1810.92 ms โ 1834.29 ms โ no change โ
โ QQuery 3 โ 1023.69 ms โ 1044.90 ms โ no change โ
โ QQuery 4 โ 2203.93 ms โ 2153.86 ms โ no change โ
โ QQuery 5 โ 28118.13 ms โ 28127.06 ms โ no change โ
โ QQuery 6 โ 3882.93 ms โ 3843.34 ms โ no change โ
โ QQuery 7 โ 2523.06 ms โ 2459.08 ms โ no change โ
โโโโโโโโโโโโดโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 42720.62ms โ
โ Total Time (push-limit-into-join) โ 42663.01ms โ
โ Average Time (HEAD) โ 5340.08ms โ
โ Average Time (push-limit-into-join) โ 5332.88ms โ
โ Queries Faster โ 0 โ
โ Queries Slower โ 0 โ
โ Queries with No Change โ 8 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโ
--------------------
Benchmark clickbench_partitioned.json
--------------------
โโโโโโโโโโโโโณโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ push-limit-into-join โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 0 โ 2.54 ms โ 2.67 ms โ 1.05x slower โ
โ QQuery 1 โ 51.86 ms โ 50.67 ms โ no change โ
โ QQuery 2 โ 131.49 ms โ 134.76 ms โ no change โ
โ QQuery 3 โ 149.97 ms โ 158.75 ms โ 1.06x slower โ
โ QQuery 4 โ 1008.35 ms โ 998.52 ms โ no change โ
โ QQuery 5 โ 1319.07 ms โ 1345.33 ms โ no change โ
โ QQuery 6 โ 6.48 ms โ 6.81 ms โ 1.05x slower โ
โ QQuery 7 โ 52.71 ms โ 56.17 ms โ 1.07x slower โ
โ QQuery 8 โ 1401.86 ms โ 1405.80 ms โ no change โ
โ QQuery 9 โ 1806.76 ms โ 1785.18 ms โ no change โ
โ QQuery 10 โ 348.70 ms โ 352.97 ms โ no change โ
โ QQuery 11 โ 409.11 ms โ 407.47 ms โ no change โ
โ QQuery 12 โ 1212.64 ms โ 1241.57 ms โ no change โ
โ QQuery 13 โ 1883.16 ms โ 1900.00 ms โ no change โ
โ QQuery 14 โ 1283.92 ms โ 1228.05 ms โ no change โ
โ QQuery 15 โ 1211.03 ms โ 1155.14 ms โ no change โ
โ QQuery 16 โ 2545.07 ms โ 2445.83 ms โ no change โ
โ QQuery 17 โ 2549.01 ms โ 2430.71 ms โ no change โ
โ QQuery 18 โ 5397.79 ms โ 4729.88 ms โ +1.14x faster โ
โ QQuery 19 โ 120.43 ms โ 122.74 ms โ no change โ
โ QQuery 20 โ 1915.55 ms โ 1942.62 ms โ no change โ
โ QQuery 21 โ 2187.93 ms โ 2220.30 ms โ no change โ
โ QQuery 22 โ 3899.91 ms โ 3738.43 ms โ no change โ
โ QQuery 23 โ 20144.26 ms โ 12099.83 ms โ +1.66x faster โ
โ QQuery 24 โ 220.02 ms โ 221.40 ms โ no change โ
โ QQuery 25 โ 491.10 ms โ 475.16 ms โ no change โ
โ QQuery 26 โ 226.21 ms โ 207.62 ms โ +1.09x faster โ
โ QQuery 27 โ 4957.10 ms โ 2663.48 ms โ +1.86x faster โ
โ QQuery 28 โ 23529.46 ms โ 21905.22 ms โ +1.07x faster โ
โ QQuery 29 โ 977.28 ms โ 958.81 ms โ no change โ
โ QQuery 30 โ 1296.84 ms โ 1241.97 ms โ no change โ
โ QQuery 31 โ 1352.66 ms โ 1337.50 ms โ no change โ
โ QQuery 32 โ 4227.83 ms โ 4580.63 ms โ 1.08x slower โ
โ QQuery 33 โ 5652.02 ms โ 5280.43 ms โ +1.07x faster โ
โ QQuery 34 โ 5615.60 ms โ 5654.40 ms โ no change โ
โ QQuery 35 โ 1920.91 ms โ 1885.60 ms โ no change โ
โ QQuery 36 โ 197.94 ms โ 194.01 ms โ no change โ
โ QQuery 37 โ 75.22 ms โ 74.31 ms โ no change โ
โ QQuery 38 โ 114.84 ms โ 115.47 ms โ no change โ
โ QQuery 39 โ 349.70 ms โ 338.82 ms โ no change โ
โ QQuery 40 โ 38.18 ms โ 40.07 ms โ no change โ
โ QQuery 41 โ 35.02 ms โ 34.69 ms โ no change โ
โ QQuery 42 โ 30.46 ms โ 31.67 ms โ no change โ
โโโโโโโโโโโโโดโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 102348.01ms โ
โ Total Time (push-limit-into-join) โ 89201.50ms โ
โ Average Time (HEAD) โ 2380.19ms โ
โ Average Time (push-limit-into-join) โ 2074.45ms โ
โ Queries Faster โ 6 โ
โ Queries Slower โ 5 โ
โ Queries with No Change โ 32 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโ
--------------------
Benchmark tpch_mem_sf1.json
--------------------
โโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโโ
โ Query โ HEAD โ push-limit-into-join โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 1 โ 102.78 ms โ 103.60 ms โ no change โ
โ QQuery 2 โ 32.30 ms โ 32.40 ms โ no change โ
โ QQuery 3 โ 42.97 ms โ 36.37 ms โ +1.18x faster โ
โ QQuery 4 โ 31.69 ms โ 31.28 ms โ no change โ
โ QQuery 5 โ 92.69 ms โ 93.06 ms โ no change โ
โ QQuery 6 โ 20.94 ms โ 21.13 ms โ no change โ
โ QQuery 7 โ 160.77 ms โ 164.36 ms โ no change โ
โ QQuery 8 โ 42.06 ms โ 39.68 ms โ +1.06x faster โ
โ QQuery 9 โ 104.00 ms โ 101.22 ms โ no change โ
โ QQuery 10 โ 67.94 ms โ 67.31 ms โ no change โ
โ QQuery 11 โ 19.33 ms โ 18.88 ms โ no change โ
โ QQuery 12 โ 51.81 ms โ 51.32 ms โ no change โ
โ QQuery 13 โ 50.61 ms โ 49.55 ms โ no change โ
โ QQuery 14 โ 15.18 ms โ 15.26 ms โ no change โ
โ QQuery 15 โ 30.56 ms โ 31.16 ms โ no change โ
โ QQuery 16 โ 28.43 ms โ 28.09 ms โ no change โ
โ QQuery 17 โ 144.45 ms โ 140.19 ms โ no change โ
โ QQuery 18 โ 286.86 ms โ 281.42 ms โ no change โ
โ QQuery 19 โ 40.09 ms โ 40.28 ms โ no change โ
โ QQuery 20 โ 57.62 ms โ 54.70 ms โ +1.05x faster โ
โ QQuery 21 โ 194.35 ms โ 192.32 ms โ no change โ
โ QQuery 22 โ 22.64 ms โ 23.39 ms โ no change โ
โโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 1640.06ms โ
โ Total Time (push-limit-into-join) โ 1616.98ms โ
โ Average Time (HEAD) โ 74.55ms โ
โ Average Time (push-limit-into-join) โ 73.50ms โ
โ Queries Faster โ 3 โ
โ Queries Slower โ 0 โ
โ Queries with No Change โ 19 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
```
</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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783619599
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1506,53 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
Review Comment:
It is needed, the optimizer uses it to pass the limit to the exec node. `with_fetch()` will only be called during compile time for limit pushdown so the current solution is fine. I will change it to use HashJoinExec builder
--
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] feat: Push limit into hash join [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2788247999
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
Builder was added to have an ability to build `HashJoinExec` using:
1. `projection: Option<Vec<usize>>` (not break existing `try_new` API)
2. shared projection `Option<ProjectionRef>` (to share projection if exec is created from another one and the projection is preserved).
The reason to not use `impl Option<Into<ProjectionRef>>` as `try_new` arg here: it requires to specify `None` type explicitly when pass `None`. The reason to not add an another `try_new_with_shared_projection`: it looks confusing as we don't want to create a new constructor each time when some arg need to be either one thing or another.
If we keep the builder, to reduce hidden semantics as much as possible, it should keep all existing fields when constructed from the existing exec, allowing customizing things that must be rest. I will make a patch for 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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783669951
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
Very nice point, solved
--
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] feat: Push limit into hash join [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2788247999
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
Builder was added to have an ability to build `HashJoinExec` using:
1. `projection: Option<Vec<usize>>` (not break existing `try_new` API)
2. shared projection `Option<ProjectionRef>` (to share projection if exec is created from another one and the projection is preserved).
The reason to not use `impl Option<Into<ProjectionRef>>` as `try_new` arg here: it requires to specify `None` type explicitly when pass `None`. The reason to not add an another `try_new_with_shared_projection`: it looks confusing as we don't want to create a new constructor each time when some arg need to be either one thing or another.
If we keep the builder, to avoid hidden semantics as much as possible, it should keep all existing fields when constructed from the existing exec, allowing customizing things that must be rest. I will make a patch for 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] feat: Push limit into hash join [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2781796159
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1506,53 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ // Null-aware anti join requires seeing ALL probe rows to check for NULLs.
+ // If any probe row has NULL, the output must be empty.
+ // We can't stop early or we might miss a NULL and return wrong results.
+ if self.null_aware {
Review Comment:
I don't understand this part.
The `output_buffer` will only get filled when an output entry is finalized, so this should be handled automatically?
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -258,6 +258,8 @@ pub struct HashJoinExecBuilder {
partition_mode: PartitionMode,
null_equality: NullEquality,
null_aware: bool,
+ /// Maximum number of rows to return
Review Comment:
```suggestion
/// Maximum number of rows to return
///
/// If the operator produces `< fetch` rows, it returns all available rows.
/// If it produces `>= fetch` rows, it returns exactly `fetch` rows and stops early.
```
--
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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2787874938
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
I think there should be some docs for it. Filed in #20270
--
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] feat: Push limit into hash join [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870606025
๐ค `./gh_compare_branch.sh` [gh_compare_branch.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_branch.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing push-limit-into-join (832a79c22548ff273e375910fae9ce965e5929fb) to 0fd6a40d4eef274b8661a6158c62df12bab28204 [diff](https://github.com/apache/datafusion/compare/0fd6a40d4eef274b8661a6158c62df12bab28204..832a79c22548ff273e375910fae9ce965e5929fb) using: tpch
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] feat: Push limit into hash join [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2787419748
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
@2010YOUY01 See my concern at https://github.com/apache/datafusion/pull/20228#discussion_r2782276949
--
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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783366032
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
Mishap I forgot to add it to new() method thanks for catching this.
--
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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783619599
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1506,53 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
Review Comment:
It is needed, the optimizer uses it to pass fetch to the exec node. `with_fetch()` will only be called during compile time for limit pushdown so the current solution is fine. I will change it to use HashJoinExec builder
--
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] feat: Push limit into hash join [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870528812
run benchmark tpch
--
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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783366032
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
Mishap I forgot to add it to the builder, thanks for catching this.
--
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] feat: Push limit into hash join [datafusion]
Posted by "2010YOUY01 (via GitHub)" <gi...@apache.org>.
2010YOUY01 commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2787267388
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
I don't get the purpose of this builder, and why do we have to do a round trip to set this `fetch`.
Also, the comment on `HashJoinExecBuilder` lacks further explanation. This API feels quite subtle: it resets several runtime fields (for example, the dynamic filter), which can be surprising. I would expect more documentation explaining the intended usage, invariants, and any safety considerations when using it.
Could we do directly clone like
```rust
Self {
fetch: limit,
// ...explicitly reset not cloneable runtime contents
..self.clone()
}
```
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -760,6 +777,11 @@ impl HashJoinExec {
self.null_equality
}
+ /// Get the fetch (limit) for this join
+ pub fn fetch(&self) -> Option<usize> {
Review Comment:
Is this one needed? I think we already have a equivalent one in `impl ExecutionPlan for HashJoinExec`
--
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] feat: Push limit into hash join [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2788247999
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
Builder was added to have an ability to build `HashJoinExec` using:
1. `projection: Option<Vec<usize>>` (not break existing `try_new` API)
2. shared projection `Option<ProjectionRef>` (to share projection if exec is created from another one and the projection is preserved).
The reason to not use `impl Option<Into<ProjectionRef>>` as `try_new` arg here: it requires to specify `None` type explicitly when pass `None`. The reason to not add another `try_new_with_shared_projection`: it looks confusing as we don't want to create a new constructor each time when some arg need to be either one thing or another.
If we keep the builder, to reduce hidden semantics as much as possible, it should keep all existing fields when constructed from the existing exec, allowing customizing things that must be rest. I will make the patch for 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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2789145969
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
I'm confused, should we leave the current implementation as it is then?
--
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] feat: Push limit into hash join [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870608951
๐ค: Benchmark completed
<details><summary>Details</summary>
<p>
```
Comparing HEAD and push-limit-into-join
--------------------
Benchmark tpch_sf1.json
--------------------
โโโโโโโโโโโโโณโโโโโโโโโโโโณโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโโโโ
โ Query โ HEAD โ push-limit-into-join โ Change โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ QQuery 1 โ 179.88 ms โ 179.66 ms โ no change โ
โ QQuery 2 โ 85.83 ms โ 86.53 ms โ no change โ
โ QQuery 3 โ 124.91 ms โ 120.58 ms โ no change โ
โ QQuery 4 โ 78.29 ms โ 78.10 ms โ no change โ
โ QQuery 5 โ 176.45 ms โ 170.54 ms โ no change โ
โ QQuery 6 โ 66.82 ms โ 67.87 ms โ no change โ
โ QQuery 7 โ 203.32 ms โ 204.60 ms โ no change โ
โ QQuery 8 โ 161.51 ms โ 165.91 ms โ no change โ
โ QQuery 9 โ 234.77 ms โ 223.69 ms โ no change โ
โ QQuery 10 โ 185.20 ms โ 185.05 ms โ no change โ
โ QQuery 11 โ 59.12 ms โ 64.23 ms โ 1.09x slower โ
โ QQuery 12 โ 117.63 ms โ 115.83 ms โ no change โ
โ QQuery 13 โ 222.07 ms โ 220.27 ms โ no change โ
โ QQuery 14 โ 91.58 ms โ 90.01 ms โ no change โ
โ QQuery 15 โ 121.01 ms โ 127.12 ms โ 1.05x slower โ
โ QQuery 16 โ 59.01 ms โ 60.82 ms โ no change โ
โ QQuery 17 โ 255.15 ms โ 251.88 ms โ no change โ
โ QQuery 18 โ 304.12 ms โ 304.39 ms โ no change โ
โ QQuery 19 โ 133.79 ms โ 132.96 ms โ no change โ
โ QQuery 20 โ 124.32 ms โ 128.20 ms โ no change โ
โ QQuery 21 โ 251.75 ms โ 258.60 ms โ no change โ
โ QQuery 22 โ 41.07 ms โ 42.37 ms โ no change โ
โโโโโโโโโโโโโดโโโโโโโโโโโโดโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโโโโ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโณโโโโโโโโโโโโ
โ Benchmark Summary โ โ
โกโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโฉ
โ Total Time (HEAD) โ 3277.61ms โ
โ Total Time (push-limit-into-join) โ 3279.21ms โ
โ Average Time (HEAD) โ 148.98ms โ
โ Average Time (push-limit-into-join) โ 149.05ms โ
โ Queries Faster โ 0 โ
โ Queries Slower โ 2 โ
โ Queries with No Change โ 20 โ
โ Queries with Failure โ 0 โ
โโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโโดโโโโโโโโโโโโ
```
</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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2788216053
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
Yes that makes sense, I can change to your recommendation
--
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] feat: Push limit into hash join [datafusion]
Posted by "askalt (via GitHub)" <gi...@apache.org>.
askalt commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2788247999
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1517,32 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ HashJoinExecBuilder::from(self)
Review Comment:
Builder was added to have an ability to build `HashJoinExec` using:
1. `projection: Option<Vec<usize>>` (not break existing `try_new` API)
2. shared projection `Option<ProjectionRef>` (to share projection if exec is created from another one and the projection is preserved).
The reason to not use `impl Option<Into<ProjectionRef>>` as `try_new` arg here: it requires to specify `None` type explicitly when pass `None`. The reason to not add another `try_new_with_shared_projection`: it looks confusing as we don't want to create a new constructor each time when some arg need to be either one thing or another.
If we keep the builder, to reduce hidden semantics as much as possible, it should keep all existing fields when constructed from the existing exec, allowing customizing things that must be rest. I will make a patch for 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] feat: Push limit into hash join [datafusion]
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870120641
run benchmarks
--
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] feat: Push limit into hash join [datafusion]
Posted by "jonathanc-n (via GitHub)" <gi...@apache.org>.
jonathanc-n commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2783439428
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1506,53 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
+ // Null-aware anti join requires seeing ALL probe rows to check for NULLs.
+ // If any probe row has NULL, the output must be empty.
+ // We can't stop early or we might miss a NULL and return wrong results.
+ if self.null_aware {
Review Comment:
If we have two tables like:
table 1 join column: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]
table 2 join column: [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, NULL]
If we do a LIMIT = 5, when probing, we will match 5 times then the limited batch coalescer will push the final batch before we hit the NULL.
--
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] feat: Push limit into hash join [datafusion]
Posted by "alamb-ghbot (via GitHub)" <gi...@apache.org>.
alamb-ghbot commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870526348
๐ค `./gh_compare_branch.sh` [gh_compare_branch.sh](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_branch.sh) Running
Linux aal-dev 6.14.0-1018-gcp #19~24.04.1-Ubuntu SMP Wed Sep 24 23:23:09 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing push-limit-into-join (832a79c22548ff273e375910fae9ce965e5929fb) to 0fd6a40d4eef274b8661a6158c62df12bab28204 [diff](https://github.com/apache/datafusion/compare/0fd6a40d4eef274b8661a6158c62df12bab28204..832a79c22548ff273e375910fae9ce965e5929fb) using: tpcds
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] feat: Push limit into hash join [datafusion]
Posted by "Dandandan (via GitHub)" <gi...@apache.org>.
Dandandan commented on PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#issuecomment-3870528154
(not sure if there is any query benefiting from this in tpch / tpcds, but those contain joins at least)
--
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] feat: Push limit into hash join [datafusion]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #20228:
URL: https://github.com/apache/datafusion/pull/20228#discussion_r2782276949
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1476,12 +1506,53 @@ impl ExecutionPlan for HashJoinExec {
filter: dynamic_filter,
build_accumulator: OnceLock::new(),
}),
+ fetch: self.fetch,
});
result = result.with_updated_node(new_node as Arc<dyn ExecutionPlan>);
}
}
Ok(result)
}
+
+ fn supports_limit_pushdown(&self) -> bool {
+ // Hash join execution plan does not support pushing limit down through to children
+ // because the children don't know about the join condition and can't
+ // determine how many rows to produce
+ false
+ }
+
+ fn fetch(&self) -> Option<usize> {
+ self.fetch
+ }
+
+ fn with_fetch(&self, limit: Option<usize>) -> Option<Arc<dyn ExecutionPlan>> {
Review Comment:
Is this method really needed ?
If it is not then I'd suggest to remove it.
If the HashJoinExec is already executed then setting a new limit will be confusing/inconsistent unless it is re-executed again.
Alternatively it could be implemented as:
```rust
HashJoinExecBuilder::from(self)
.with_fetch(limit)
.build()
.ok()
.map(|exec| Arc::new(exec) as _)
```
This way it won't keep the calculated state.
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
The `fetch` field is not preserved here. Is this intentional ?
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
The `fetch` field is not preserved here. Is this intentional ?
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
Review Comment:
Should the statistics take into account the `fetch`/limit when estimating ?
--
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