sunchao commented on code in PR #25602:
URL: https://github.com/apache/datafusion/pull/25602#discussion_r4076638507
##########
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:
[P1] Preserve the implicit NULL branch during full-match inference
Omitting the missing `ELSE` is conservative for ordinary filtering, but this
rewriter is also used to build the inverse predicate that proves an entire
Parquet row group matches. That makes this change return incorrect rows when
`datafusion.execution.parquet.pushdown_filters=true`.
I reproduced this with required Int64 columns `(a,b)`, two one-row groups
containing `(2,1)` and `(1,2)`, and:
```sql
SELECT * FROM t
WHERE NOT (CASE WHEN a = 1 THEN false END) AND b > 0
ORDER BY a;
```
Base `714956b3` returns only `(1,2)`; head `adbb3ce4` also returns `(2,1)`,
although its predicate result is NULL. With `LIMIT 1` instead of `ORDER BY`,
head returns the invalid `(2,1)`. Disabling filter pushdown restores the
correct result.
The forward rewrite falls back for `NOT CASE`, so it still permits
full-match inversion. The inverse exposes `CASE ... OR b <= 0`; dropping the
implicit NULL branch lets it prune both groups, which are then marked fully
matched and skip row filtering. Head reports two fully matched groups and
`row_filter_skipped_fully_matched=1`.
Could we keep CASE expressions without an explicit `ELSE` conservatively
unhandled here, and add an end-to-end regression test? The join-generated CASE
already has an explicit ELSE. Merely setting `has_filter_semantics_only` inside
this branch would miss the forward `NOT CASE` path, which never descends into
it.
##########
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:
[P2] Skip bitmap construction when dynamic filtering is inactive
`bounds` can exist solely for perfect-hash-join candidacy even when
`should_compute_dynamic_filters` is false. This branch still constructs and
reserves a pruning bitmap before those bounds are cleared below, so a join with
dynamic filtering disabled can now fail for memory that provides no pruning
benefit.
Using a native `CollectLeft` join with 151 Int64 build keys `i * 10_000` (`i
= 0..151`), probe keys `[0, 500_000, 1_500_000]`, and
`enable_join_dynamic_filter_pushdown=false` (otherwise default configuration),
base `714956b3` succeeds in a 100,000-byte memory pool with 6,220 bytes
reserved. Head `adbb3ce4` fails with `ResourcesExhausted` requesting another
131,072 bytes. At a 1,000,000-byte limit both return the exact expected rows,
but head reserves 137,292 bytes. I also reproduced the same failure with a Full
join.
Could we gate this bitmap construction on `should_compute_dynamic_filters`
and cover the disabled-filter case with a memory-limit regression test?
--
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]
