Skip to content

Commit 406e428

Browse files
adriangbclaude
andcommitted
test: check per rule that the pushdown rules reach a fixed point
Address review: add an observer test that fails when PushDownFilter or PushDownLeafProjections changes the plan in the last optimizer pass, and correct the rule precedence wording in the optimizer guide. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f146f1d commit 406e428

2 files changed

Lines changed: 61 additions & 2 deletions

File tree

‎datafusion/optimizer/src/push_down_filter.rs‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4627,6 +4627,65 @@ mod tests {
46274627
)
46284628
}
46294629

4630+
/// Runs the default optimizer and returns, for each pass, the names of the
4631+
/// rules that changed the plan in that pass.
4632+
fn rules_that_changed_plan_per_pass(plan: LogicalPlan) -> Result<Vec<Vec<String>>> {
4633+
let optimizer = Optimizer::new();
4634+
let rules_per_pass = optimizer.rules.len();
4635+
let mut previous = plan.display_indent().to_string();
4636+
let mut calls = 0;
4637+
let mut passes: Vec<Vec<String>> = vec![];
4638+
optimizer.optimize(plan, &OptimizerContext::new(), |plan, rule| {
4639+
if calls % rules_per_pass == 0 {
4640+
passes.push(vec![]);
4641+
}
4642+
calls += 1;
4643+
let current = plan.display_indent().to_string();
4644+
if current != previous {
4645+
passes.last_mut().unwrap().push(rule.name().to_string());
4646+
previous = current;
4647+
}
4648+
})?;
4649+
Ok(passes)
4650+
}
4651+
4652+
/// `PushDownFilter` and `PushDownLeafProjections` must not undo each other
4653+
/// for a filter next to a pure extraction projection
4654+
/// (<https://github.com/apache/datafusion/issues/14540>). Neither rule may
4655+
/// change the plan in the last optimizer pass. The source does not absorb
4656+
/// filters, so the `Filter` node stays in the plan.
4657+
#[test]
4658+
fn filter_and_extraction_projection_reach_fixed_point() -> Result<()> {
4659+
let scan = || {
4660+
table_scan_with_pushdown_provider_builder(
4661+
TableProviderFilterPushDown::Unsupported,
4662+
vec![],
4663+
None,
4664+
)
4665+
};
4666+
let simple = scan()?
4667+
.filter(col("b").gt(lit(5)))?
4668+
.project(vec![leaf_udf_expr(col("a"))])?
4669+
.build()?;
4670+
// Two filters, one of them on an extracted leaf.
4671+
let two_filters = scan()?
4672+
.filter(col("b").gt(lit(5)))?
4673+
.filter(leaf_udf_expr(col("a")).eq(lit(1)))?
4674+
.project(vec![leaf_udf_expr(col("a")), col("b")])?
4675+
.build()?;
4676+
4677+
for plan in [simple, two_filters] {
4678+
let passes = rules_that_changed_plan_per_pass(plan)?;
4679+
let last = passes.last().unwrap();
4680+
assert!(
4681+
!last.iter().any(|rule| rule == "push_down_filter"
4682+
|| rule == "push_down_leaf_projections"),
4683+
"pushdown rules changed the plan in the last pass: {passes:?}"
4684+
);
4685+
}
4686+
Ok(())
4687+
}
4688+
46304689
/// A projection that mixes an extraction alias with a computed expression is
46314690
/// not a pure extraction projection, so the filter still moves below it.
46324691
#[test]

‎docs/source/library-user-guide/query-optimizer.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -164,9 +164,9 @@ the actual optimization. This approach is used in projection push down and filte
164164

165165
### Rule Precedence
166166

167-
Two rules can want the opposite order for the same pair of adjacent plan nodes. Each rule then undoes the work of the other one on every optimizer pass. The loop stops at `datafusion.optimizer.max_passes`, and the position of the two rules in the rule list decides the plan. This wastes a pass and makes the plan depend on the rule list.
167+
Two rules can want the opposite order for the same pair of adjacent plan nodes. Each rule then undoes the work of the other one on every optimizer pass. The plan does not reach a fixed point, so the optimizer runs more passes than necessary, and the position of the two rules in the rule list decides the plan. The optimizer can stop early when the plan at the end of a pass is the same as the plan at the end of an earlier pass, but that does not make the result independent of the rule order.
168168

169-
Do not let two rules compete. Give one rule precedence, make that rule yield, and record the decision in the module documentation of both rules.
169+
Do not let two rules compete. Give one rule precedence, make the competing rule yield, and record the decision in the module documentation of both rules.
170170

171171
There is one such decision today:
172172

0 commit comments

Comments
 (0)