zhuqi-lucas opened a new pull request, #23532:
URL: https://github.com/apache/datafusion/pull/23532
## Which issue does this PR close?
- Closes #.
## Rationale for this change
`DynamicFilterPhysicalExpr::current()` is invoked per batch on the RowFilter
path (via `PhysicalExpr::evaluate`). It calls `remap_children`, which does a
`transform_up` tree walk. For dynamic filters that carry a large `InListExpr` —
e.g. `HashJoinExec` pushing down a `col IN build_keys` list of ~150+ values —
that walk is dominated by `InListExpr::with_new_children` cloning the whole
list on every call.
The `inner.generation` only changes when the filter is `update`d — once per
HashJoin build, or once per TopK threshold refresh. Between updates every
`current()` call recomputes an identical remapped tree. Caching per generation
lets per-batch calls short-circuit the walk.
### Measured impact (TPCH SF1, 10 iterations)
| | HEAD `pushdown=true` | This PR `pushdown=true` | Baseline
`pushdown=false` |
|---|---|---|---|
| **Q17** | 128 ms | **53 ms** (**2.4× faster**) | 55 ms |
| Q18 | 69 ms | 56 ms | 49 ms |
| Total (22 queries) | 743 ms | 644 ms (**-13%**) | 531 ms |
Q17 (Nation-in-orders subquery) matches the `pushdown=false` baseline after
the fix — the DynamicFilter tax is fully eliminated on that shape. Row counts
identical across all 22 queries pre/post fix.
Profile before the fix: `InListExpr::with_new_children` and its `drop_glue`
sat at the top of Q17's samply flamegraph.
## What changes are included in this PR?
- Add `current_cache: Arc<RwLock<Option<(u64, Arc<dyn PhysicalExpr>)>>>` on
`DynamicFilterPhysicalExpr` (aliased as `CurrentExprCache` to keep the type
readable).
- `current()`: read the current `(expr, generation)` from `inner`, return
the cached `Arc` if the cached generation matches; otherwise recompute via
`remap_children` and store the new pair under a write lock.
- Reset semantics:
- `update()` bumps `inner.generation`, so the next `current()` misses the
cache and recomputes.
- `with_new_children` clones the outer struct with new
`remapped_children`; the derived filter gets its own fresh cache slot.
- `from_parts` (proto deserialization) starts with an empty cache.
- No public API changes.
## Are these changes tested?
Yes — three new regression tests in `expressions::dynamic_filters::test`:
- `test_current_cache_hits_within_generation` — three consecutive
`current()` calls return the same `Arc` (pointer-equal) at the same generation,
proving the remap walk did not re-run.
- `test_current_cache_invalidates_on_update` — after `update()` bumps the
generation, `current()` returns a fresh `Arc` (not pointer-equal, and stringly
distinct).
- `test_current_cache_is_per_derived_filter` — two filters derived via
`with_new_children` with different `remapped_children` produce distinct cached
`Arc`s and each hits its own cache on subsequent calls.
All 21 tests in `expressions::dynamic_filters` (18 existing + 3 new) pass.
## Are there any user-facing changes?
No. Internal caching only. No public API changes; behavior is identical
modulo the generation-tied fast path.
--
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]
