jayshrivastava commented on code in PR #24601:
URL: https://github.com/apache/datafusion/pull/24601#discussion_r3844785894
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -999,6 +999,13 @@ impl HashJoinExec {
/// Set the dynamic filter on this hash join.
///
+ /// Holding a dynamic filter is what makes the join compute and publish
+ /// build-side bounds during execution, so callers are asserting that
+ /// something will consume the filter. The filter pushdown rule only sets
one
+ /// after finding a consumer for it in the probe subtree; callers wiring a
+ /// filter up by hand (for example to carry it across a network boundary)
+ /// take on that check themselves.
+ ///
Review Comment:
```suggestion
/// Setting a dynamic filter is what makes the join compute and publish
/// build-side bounds during execution.
[`Self::handle_child_pushdown_result`]
/// sets one after finding a consumer for it in the probe side. Callers
wiring a
/// filter up by hand take on that check themselves.
```
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -1449,20 +1456,13 @@ impl ExecutionPlan for HashJoinExec {
consider using CoalescePartitionsExec or the EnforceDistribution
rule"
);
- // Only compute a dynamic filter when the probe subtree contains a
consumer.
- // Searching from `self` would always find the producer expression
owned by this join.
- let enable_dynamic_filter_pushdown = if self
+ // Whether the probe subtree contains a consumer for the dynamic
filter is
+ // decided at planning time in `handle_child_pushdown_result`: a join
that
+ // nothing listens to is rebuilt without a dynamic filter at all. So
by the
+ // time we get here the filter's presence is the answer.
+ let enable_dynamic_filter_pushdown = self
Review Comment:
```suggestion
let enable_dynamic_filter_pushdown = self
```
##########
datafusion/core/tests/physical_optimizer/filter_pushdown.rs:
##########
@@ -2964,11 +2965,12 @@ async fn
test_hashjoin_hash_table_pushdown_collect_left() {
);
}
-// Not portable to sqllogictest: verifies whether the optimized probe-side plan
-// retains the HashJoinExec's dynamic filter expression. The
with_support(false)
-// branch has no SQL analog because parquet supports filter pushdown.
+// Not portable to sqllogictest: verifies that the HashJoinExec only keeps its
+// dynamic filter when the optimized probe-side plan retains the expression,
i.e.
+// when there is something to consume it. The with_support(false) branch has no
+// SQL analog because parquet supports filter pushdown.
#[test]
-fn test_hashjoin_dynamic_filter_pushdown_is_used() {
+fn test_hashjoin_dynamic_filter_requires_probe_consumer() {
fn contains_expression_id(plan: &Arc<dyn ExecutionPlan>, expression_id:
u64) -> bool {
Review Comment:
nit: can we call `plan_contains_expression_id` here?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
