zhuqi-lucas commented on PR #23701:
URL: https://github.com/apache/datafusion/pull/23701#issuecomment-5018175087
Follow-up: **replaced the tactical PostScanFilter skip with a root-cause fix
in HashJoin** (commit 836f46e2e).
The tactical fix removed dynamic filter conjuncts from `post_scan_conjuncts`
in `opener/mod.rs`. That cured TPC-H but per the tpcds bench above it also
killed real wins where the dynamic filter itself was cheap (Q72: 4.91x faster →
no change) — the tactical filter was too broad.
**Root cause**: `SharedBuildAccumulator::build_filter` for
`PartitionMode::Partitioned` emitted `CASE hash(col) % N WHEN pid THEN bounds
ELSE lit(false) END`. On the probe scan this evaluated hash + modulo + CASE
branch **per row** — the samply hotspot on Q9
(`expressions::case::PartialResultIndex::merge_n` 1.11% self-time on this PR vs
0% on main).
The CASE routing existed to keep per-partition bounds exact, but the
downstream hash lookup is *also* per-partition and exact — so the CASE was
doing work the lookup was about to do again, at higher per-row cost. On the
probe scan this is redundant.
**Fix**: when the membership gate is off (production default after the
split-membership commit), emit the union of per-partition bounds instead — same
shape as `PartitionMode::CollectLeft`:
```
col >= min(min_0, ..., min_{N-1}) AND col <= max(max_0, ..., max_{N-1})
```
A probe row can pass the union and still miss its build partition, but the
exact hash lookup drops it — no wrong results. Membership opt-in still uses the
historical CASE form (so `InListExpr` / `HashTableLookupExpr` can route to the
correct partitions build values).
With the expensive per-row form gone, `PostScanFilter` on
`pushdown_filters=false` is cheap again — the `opener/mod.rs` tactical filter
is **reverted**, and dynamic filter row-level pruning is back for cheap forms
(CollectLeft, TopK). This should restore the TPC-DS wins we lost.
Local TPC-H SF1 (release-nonlto, 3 iters, avg):
- baseline (HEAD~2, pre-#22384): 27.65 ms
- PR + tactical fix: 26.24 ms
- PR + this root fix: **26.99 ms** (still ahead of baseline, Q9 back at
baseline)
Waiting on CI benchmark run to confirm TPC-DS Q72 / Q84 / Q98 all recover.
--
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]
