Skip to content

Commit 7fa6e21

Browse files
authored
Skip map_expressions rebuild for Extension nodes with empty expressions (apache#21701)
## Which issue does this PR close? - Closes apache#21700. ## Rationale for this change When an Extension node has no expressions, `map_expressions` was still cloning all inputs and calling `with_exprs_and_inputs` to reconstruct the node — wasted work since there are no expressions to transform. This is common for Extension nodes like view matching candidates that carry multiple children but no expressions. ## What changes are included in this PR? 1. **Code change** (`datafusion/expr/src/logical_plan/tree_node.rs`): Add early return when `node.expressions()` is empty, skipping the clone + rebuild path. 2. **Micro-benchmark** (`datafusion/expr/benches/map_expressions.rs`): Criterion benchmark comparing `map_expressions` on Extension nodes with and without expressions, varying the number of children (1, 3, 5, 10). ## Are these changes tested? Yes — existing tests pass, and the new benchmark validates the optimization. Benchmark results: | Children | no_expr (optimized) | with_expr (rebuild) | Speedup | |----------|--------------------|--------------------|---------| | 1 | 24 ns | 167 ns | 7x | | 3 | 23 ns | 192 ns | 8x | | 5 | 23 ns | 181 ns | 8x | | 10 | 24 ns | 216 ns | 9x | The `no_expr` path is constant time regardless of children count. In a real optimizer pipeline (~15 rules × 5-child Extension), this saves ~2.4 us per optimization pass. ## Next step A more general optimization would be to change `UserDefinedLogicalNode::expressions()` to return references (`&[Expr]`) instead of cloned `Vec<Expr>`, and only clone + rebuild when the transform actually modifies an expression. This would avoid the clone + `with_exprs_and_inputs` rebuild even for non-empty expression lists when the transform is a no-op. Added a TODO comment in the code for this direction. This would be a larger API change, so the empty-expressions shortcut is a pragmatic first step. ## Are there any user-facing changes? No — purely internal optimization. No API changes.
1 parent 65f337d commit 7fa6e21

2 files changed

Lines changed: 68 additions & 65 deletions

File tree

Cargo.lock

Lines changed: 45 additions & 54 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

datafusion/expr/src/logical_plan/tree_node.rs

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -584,17 +584,29 @@ impl LogicalPlan {
584584
.map_elements(f)?
585585
.update_data(|expr| LogicalPlan::Sort(Sort { expr, input, fetch })),
586586
LogicalPlan::Extension(Extension { node }) => {
587-
// would be nice to avoid this copy -- maybe can
588-
// update extension to just observer Exprs
589-
let exprs = node.expressions().map_elements(f)?;
590-
let plan = LogicalPlan::Extension(Extension {
591-
node: UserDefinedLogicalNode::with_exprs_and_inputs(
592-
node.as_ref(),
593-
exprs.data,
594-
node.inputs().into_iter().cloned().collect::<Vec<_>>(),
595-
)?,
596-
});
597-
Transformed::new(plan, exprs.transformed, exprs.tnr)
587+
let raw_exprs = node.expressions();
588+
if raw_exprs.is_empty() {
589+
// No expressions to transform — skip expensive clone of
590+
// all inputs and reconstruction via with_exprs_and_inputs.
591+
Transformed::no(LogicalPlan::Extension(Extension { node }))
592+
} else {
593+
// TODO: a more general optimization would be to change
594+
// `UserDefinedLogicalNode::expressions()` to return
595+
// references (`&[Expr]`) instead of cloned `Vec<Expr>`,
596+
// and only clone + rebuild when the transform actually
597+
// modifies an expression. This would avoid the clone +
598+
// `with_exprs_and_inputs` rebuild even for non-empty
599+
// expression lists when the transform is a no-op.
600+
let exprs = raw_exprs.map_elements(f)?;
601+
let plan = LogicalPlan::Extension(Extension {
602+
node: UserDefinedLogicalNode::with_exprs_and_inputs(
603+
node.as_ref(),
604+
exprs.data,
605+
node.inputs().into_iter().cloned().collect::<Vec<_>>(),
606+
)?,
607+
});
608+
Transformed::new(plan, exprs.transformed, exprs.tnr)
609+
}
598610
}
599611
LogicalPlan::TableScan(TableScan {
600612
table_name,

0 commit comments

Comments
 (0)