Skip to content

Commit 3f4cee9

Browse files
committed
perf(physical-expr): cache remapped expression in DynamicFilterPhysicalExpr::current()
`DynamicFilterPhysicalExpr::current()` is called per batch on the RowFilter path (via `PhysicalExpr::evaluate`). It internally calls `remap_children`, which does a `transform_up` tree walk that, for filters carrying a large `InListExpr` (HashJoin's `col IN build_keys`, ~150+ values), 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 — while `current()` fires per batch. Cache the remapped expression per generation so per-batch calls short-circuit the walk. Reset semantics: * `update()` bumps `inner.generation`, invalidating the cache lookup on the next `current()`. * `with_new_children` clones the outer struct with its own remapped children; each derived filter gets its own fresh cache slot. * `from_parts` (proto deserialization) starts with an empty cache. TPCH SF1 (10 iterations): * Q17: 128ms -> 53ms (2.4x) * Total (22 queries): 743ms -> 644ms (-13%) TPCH row counts identical across all 22 queries pre/post fix. Adds three regression tests: * cache hits within a generation (same Arc returned) * update() invalidates (new Arc returned) * derived filters via with_new_children have independent caches
1 parent 65f9726 commit 3f4cee9

1 file changed

Lines changed: 167 additions & 2 deletions

File tree

  • datafusion/physical-expr/src/expressions/dynamic_filters

datafusion/physical-expr/src/expressions/dynamic_filters/mod.rs

Lines changed: 167 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,10 @@ use datafusion_physical_expr_common::physical_expr::DynHash;
3535
mod tracker;
3636
pub use tracker::{DynamicFilterTracker, DynamicFilterTracking};
3737

38+
/// Per-generation cache of the remapped current expression for
39+
/// [`DynamicFilterPhysicalExpr::current`]. See the field docs there.
40+
type CurrentExprCache = Arc<RwLock<Option<(u64, Arc<dyn PhysicalExpr>)>>>;
41+
3842
/// State of a dynamic filter, tracking both updates and completion.
3943
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
4044
enum FilterState {
@@ -72,6 +76,16 @@ pub struct DynamicFilterPhysicalExpr {
7276
/// If any of the children were remapped / modified (e.g. to adjust for projections) we need to keep track of the new children
7377
/// so that when we update `current()` in subsequent iterations we can re-apply the replacements.
7478
remapped_children: Option<Vec<Arc<dyn PhysicalExpr>>>,
79+
/// Cache of the last (generation, remapped-expression) pair returned by
80+
/// [`Self::current`]. `current()` is hot on the per-batch RowFilter path;
81+
/// when the inner generation hasn't changed (common — updates fire once
82+
/// per HashJoin build or once per TopK threshold refresh, but `evaluate`
83+
/// is called per batch), the cache serves the remapped expression
84+
/// without re-running the `transform_up` tree walk in
85+
/// [`Self::remap_children`]. Reset on `update()` (by generation bump)
86+
/// and populated with `None` on `with_new_children` (each derived
87+
/// filter owns its own cache).
88+
current_cache: CurrentExprCache,
7589
/// The source of dynamic filters.
7690
inner: Arc<RwLock<Inner>>,
7791
/// Broadcasts filter state (updates and completion) to all waiters.
@@ -189,6 +203,7 @@ impl DynamicFilterPhysicalExpr {
189203
children,
190204
remapped_children: None, // Initially no remapped children
191205
inner: Arc::new(RwLock::new(Inner::new(inner))),
206+
current_cache: Arc::new(RwLock::new(None)),
192207
state_watch,
193208
data_type: Arc::new(RwLock::new(None)),
194209
nullable: Arc::new(RwLock::new(None)),
@@ -232,9 +247,33 @@ impl DynamicFilterPhysicalExpr {
232247
/// Get the current expression.
233248
/// This will return the current expression with any children
234249
/// remapped to match calls to [`PhysicalExpr::with_new_children`].
250+
///
251+
/// Called per batch on the RowFilter path (via
252+
/// [`PhysicalExpr::evaluate`]). The remap walk is O(tree size) and, for
253+
/// dynamic filters that carry a large `InListExpr` (join key IN list),
254+
/// dominated by `InListExpr::with_new_children` cloning the whole list.
255+
/// The inner generation only changes when [`Self::update`] fires, so we
256+
/// cache the remapped expression per generation and return it directly
257+
/// on subsequent per-batch calls.
235258
pub fn current(&self) -> Result<Arc<dyn PhysicalExpr>> {
236-
let expr = Arc::clone(self.inner.read().expr());
237-
Self::remap_children(&self.children, self.remapped_children.as_ref(), expr)
259+
// Fast path: cache hit for the current generation.
260+
let (expr, generation) = {
261+
let inner = self.inner.read();
262+
(Arc::clone(inner.expr()), inner.generation)
263+
};
264+
if let Some((cached_gen, cached_expr)) = self.current_cache.read().as_ref()
265+
&& *cached_gen == generation
266+
{
267+
return Ok(Arc::clone(cached_expr));
268+
}
269+
// Slow path: (re)compute the remap and store it under a write lock.
270+
let remapped =
271+
Self::remap_children(&self.children, self.remapped_children.as_ref(), expr)?;
272+
// A concurrent thread may have populated the cache with the same
273+
// generation; either wins the write is fine — both are semantically
274+
// equivalent because the inner generation hasn't changed.
275+
*self.current_cache.write() = Some((generation, Arc::clone(&remapped)));
276+
Ok(remapped)
238277
}
239278

240279
/// Update the current expression and notify all waiters.
@@ -411,6 +450,7 @@ impl DynamicFilterPhysicalExpr {
411450
children,
412451
remapped_children,
413452
inner: Arc::new(RwLock::new(inner)),
453+
current_cache: Arc::new(RwLock::new(None)),
414454
state_watch,
415455
data_type: Arc::new(RwLock::new(None)),
416456
nullable: Arc::new(RwLock::new(None)),
@@ -436,6 +476,9 @@ impl PhysicalExpr for DynamicFilterPhysicalExpr {
436476
remapped_children: Some(children),
437477
// Note: expression_id is preserved
438478
inner: Arc::clone(&self.inner),
479+
// Fresh cache per derived filter — remap depends on this
480+
// instance's `remapped_children`, which just changed.
481+
current_cache: Arc::new(RwLock::new(None)),
439482
state_watch: self.state_watch.clone(),
440483
data_type: Arc::clone(&self.data_type),
441484
nullable: Arc::clone(&self.nullable),
@@ -1259,4 +1302,126 @@ mod test {
12591302
"mark_complete() must not change expression_id",
12601303
);
12611304
}
1305+
1306+
/// Repeated `current()` at the same generation must return the exact same
1307+
/// `Arc` — the cache serves without re-running `remap_children`.
1308+
#[test]
1309+
fn test_current_cache_hits_within_generation() {
1310+
let table_schema =
1311+
Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
1312+
let col_a = col("a", &table_schema).unwrap();
1313+
let expr = Arc::new(BinaryExpr::new(
1314+
Arc::clone(&col_a),
1315+
datafusion_expr::Operator::Gt,
1316+
lit(10) as Arc<dyn PhysicalExpr>,
1317+
));
1318+
// Force the remap path to actually run: give the filter a distinct
1319+
// `remapped_children`. Without this, `remap_children` returns the
1320+
// input Arc unchanged and every call is trivially pointer-equal.
1321+
let filter = Arc::new(DynamicFilterPhysicalExpr::new(
1322+
vec![Arc::clone(&col_a)],
1323+
expr as Arc<dyn PhysicalExpr>,
1324+
));
1325+
let remapped_schema =
1326+
Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
1327+
let derived = reassign_expr_columns(
1328+
Arc::clone(&filter) as Arc<dyn PhysicalExpr>,
1329+
&remapped_schema,
1330+
)
1331+
.unwrap();
1332+
let derived = derived
1333+
.downcast_ref::<DynamicFilterPhysicalExpr>()
1334+
.expect("derived filter must be a DynamicFilterPhysicalExpr");
1335+
1336+
// First call populates the cache. Second and third must return the
1337+
// *same* Arc — proving `remap_children` did not run again.
1338+
let first = derived.current().unwrap();
1339+
let second = derived.current().unwrap();
1340+
let third = derived.current().unwrap();
1341+
assert!(
1342+
Arc::ptr_eq(&first, &second),
1343+
"current() should return the cached Arc within a generation",
1344+
);
1345+
assert!(Arc::ptr_eq(&second, &third));
1346+
}
1347+
1348+
/// `update()` bumps the generation; the next `current()` must return a
1349+
/// fresh remapped expression, not the stale cached one.
1350+
#[test]
1351+
fn test_current_cache_invalidates_on_update() {
1352+
let table_schema =
1353+
Arc::new(Schema::new(vec![Field::new("a", DataType::Int32, false)]));
1354+
let col_a = col("a", &table_schema).unwrap();
1355+
let filter = Arc::new(DynamicFilterPhysicalExpr::new(
1356+
vec![Arc::clone(&col_a)],
1357+
lit(10) as Arc<dyn PhysicalExpr>,
1358+
));
1359+
// Remap to force the cache path.
1360+
let derived = reassign_expr_columns(
1361+
Arc::clone(&filter) as Arc<dyn PhysicalExpr>,
1362+
&table_schema,
1363+
)
1364+
.unwrap();
1365+
let derived = derived
1366+
.downcast_ref::<DynamicFilterPhysicalExpr>()
1367+
.expect("derived filter must be a DynamicFilterPhysicalExpr");
1368+
1369+
let before = derived.current().unwrap();
1370+
// Bump the generation with a distinct expression.
1371+
filter
1372+
.update(Arc::new(BinaryExpr::new(
1373+
Arc::clone(&col_a),
1374+
datafusion_expr::Operator::Gt,
1375+
lit(42) as Arc<dyn PhysicalExpr>,
1376+
)) as Arc<dyn PhysicalExpr>)
1377+
.unwrap();
1378+
let after = derived.current().unwrap();
1379+
assert!(
1380+
!Arc::ptr_eq(&before, &after),
1381+
"current() must return a fresh Arc after update() bumps the generation",
1382+
);
1383+
assert_ne!(format!("{before:?}"), format!("{after:?}"));
1384+
}
1385+
1386+
/// `with_new_children` produces a derived filter with its own cache slot;
1387+
/// populating one filter's cache must not leak into the other.
1388+
#[test]
1389+
fn test_current_cache_is_per_derived_filter() {
1390+
let schema = Arc::new(Schema::new(vec![
1391+
Field::new("a", DataType::Int32, false),
1392+
Field::new("b", DataType::Int32, false),
1393+
Field::new("c", DataType::Int32, false),
1394+
]));
1395+
// Original expression references `a`. Each derived filter remaps `a`
1396+
// to a *different* column so remap_children returns distinct exprs
1397+
// per filter (and thus distinct cached Arcs).
1398+
let col_a = col("a", &schema).unwrap();
1399+
let expr = Arc::new(BinaryExpr::new(
1400+
Arc::clone(&col_a),
1401+
datafusion_expr::Operator::Gt,
1402+
lit(10) as Arc<dyn PhysicalExpr>,
1403+
));
1404+
let filter = Arc::new(DynamicFilterPhysicalExpr::new(
1405+
vec![Arc::clone(&col_a)],
1406+
expr as Arc<dyn PhysicalExpr>,
1407+
));
1408+
1409+
let d1 = Arc::clone(&filter)
1410+
.with_new_children(vec![col("b", &schema).unwrap()])
1411+
.unwrap();
1412+
let d2 = Arc::clone(&filter)
1413+
.with_new_children(vec![col("c", &schema).unwrap()])
1414+
.unwrap();
1415+
let d1 = d1.downcast_ref::<DynamicFilterPhysicalExpr>().unwrap();
1416+
let d2 = d2.downcast_ref::<DynamicFilterPhysicalExpr>().unwrap();
1417+
1418+
let d1_first = d1.current().unwrap();
1419+
let d2_first = d2.current().unwrap();
1420+
// Distinct remap_children paths produce distinct cached Arcs.
1421+
assert!(!Arc::ptr_eq(&d1_first, &d2_first));
1422+
assert_ne!(format!("{d1_first:?}"), format!("{d2_first:?}"));
1423+
// Subsequent calls each hit their own cache.
1424+
assert!(Arc::ptr_eq(&d1_first, &d1.current().unwrap()));
1425+
assert!(Arc::ptr_eq(&d2_first, &d2.current().unwrap()));
1426+
}
12621427
}

0 commit comments

Comments
 (0)