gruuya commented on code in PR #25602:
URL: https://github.com/apache/datafusion/pull/25602#discussion_r4079604504
##########
datafusion/physical-plan/src/joins/hash_join/exec.rs:
##########
@@ -3094,19 +3095,41 @@ async fn collect_left_input(
.iter()
.map(|arr| arr.get_array_memory_size())
.sum::<usize>();
- if left_values.is_empty()
- || left_values[0].is_empty()
- || estimated_size >
config.optimizer.hash_join_inlist_pushdown_max_size
- || map.num_of_distinct_key()
- > config
+
+ let pushdown_inlist = !left_values.is_empty()
+ && !left_values[0].is_empty()
+ && estimated_size <=
config.optimizer.hash_join_inlist_pushdown_max_size
+ && map.num_of_distinct_key()
+ <= config
.optimizer
- .hash_join_inlist_pushdown_max_distinct_values
+ .hash_join_inlist_pushdown_max_distinct_values;
+
+ if pushdown_inlist
+ && let Some(in_list_values) =
build_struct_inlist_values(&left_values)?
{
- PushdownStrategy::Map(Arc::clone(&map))
- } else if let Some(in_list_values) =
build_struct_inlist_values(&left_values)? {
PushdownStrategy::InList(in_list_values)
} else {
- PushdownStrategy::Map(Arc::clone(&map))
+ // Past the InList threshold use a bucket bitmap for container
pruning.
+ let pruning_bitmap = match (left_values.as_slice(),
bounds.as_ref()) {
+ ([keys], Some(bounds)) if !keys.is_empty() => bounds
Review Comment:
Nice catch. I also noticed that the same problem happens for the InList path
too.
So instead of guarding just the (bit)map path with
`should_compute_dynamic_filters`, i made it guard both by extending the
`PushdownStrategy::Empty` arm with `|| !should_compute_dynamic_filters`; let me
know if you see issues with this.
Also a unit test added.
##########
datafusion/pruning/src/pruning_predicate.rs:
##########
@@ -1816,6 +1858,41 @@ fn build_predicate_expression(
return unhandled_hook.handle(expr);
}
}
+ if let Some(lookup) = expr.downcast_ref::<HashTableLookupExpr>() {
+ return build_hash_lookup_pruning_expr(lookup, schema, required_columns)
+ .unwrap_or_else(|| unhandled_hook.handle(expr));
+ }
+ // A partitioned hash join hides its per-partition filters under a `CASE`
on the
+ // repartition hash. A row takes exactly one branch, so a container may
match
+ // only if some branch may: the branches' disjunction is a sound
relaxation, and
+ // the `WHEN`s (a hash, which no statistics describe) can be dropped.
+ if let Some(case) = expr.downcast_ref::<phys_expr::CaseExpr>() {
+ // Only a Boolean `CASE` is a predicate; anything else is a value for
+ // whatever compares it to handle.
+ if !matches!(case.data_type(schema), Ok(DataType::Boolean)) {
+ return unhandled_hook.handle(expr);
+ }
+ // A missing `ELSE` yields NULL, which never matches, so it adds
nothing.
+ return case
+ .when_then_expr()
+ .iter()
+ .map(|(_, then)| then)
+ .chain(case.else_expr())
Review Comment:
Good point, added a bail-out for that case as well as an SLT.
--
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]
