Skip to content

Commit

Permalink
refactor: based upon performance tests, run the maximum number of che…
Browse files Browse the repository at this point in the history
…cks without impa ct:

* assert_valid_optimization can run each optimizer pass
* remove the recursive cehck_fields, which caused the performance regression
* the full LP Invariants::Executable can only run in debug
  • Loading branch information
wiedld committed Dec 24, 2024
1 parent 1164a7b commit 7ad0b74
Show file tree
Hide file tree
Showing 2 changed files with 4 additions and 10 deletions.
8 changes: 1 addition & 7 deletions datafusion/expr/src/logical_plan/invariants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,7 @@ pub fn assert_executable_invariants(plan: &LogicalPlan) -> Result<()> {
/// This invariant is subject to change.
/// refer: <https://github.com/apache/datafusion/issues/13525#issuecomment-2494046463>
fn assert_unique_field_names(plan: &LogicalPlan) -> Result<()> {
plan.schema().check_names()?;

plan.apply_with_subqueries(|plan: &LogicalPlan| {
plan.schema().check_names()?;
Ok(TreeNodeRecursion::Continue)
})
.map(|_| ())
plan.schema().check_names()
}

/// Returns an error if the plan is not sematically valid.
Expand Down
6 changes: 3 additions & 3 deletions datafusion/optimizer/src/optimizer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,6 @@ impl Optimizer {
.skip_failed_rules
.then(|| new_plan.clone());

#[cfg(debug_assertions)]
let starting_schema = Arc::clone(new_plan.schema());

let result = match rule.apply_order() {
Expand All @@ -398,15 +397,16 @@ impl Optimizer {
None => optimize_plan_node(new_plan, rule.as_ref(), config),
}
.and_then(|tnr| {
// in debug mode, run checks are each optimer pass
#[cfg(debug_assertions)]
// run checks optimizer invariant checks, per pass
assert_valid_optimization(&tnr.data, &starting_schema)
.map_err(|e| {
DataFusionError::Context(
format!("check_optimizer_specific_invariants after optimizer pass: {}", rule.name()),
Box::new(e),
)
})?;

// run LP invariant checks only in debug
#[cfg(debug_assertions)]
tnr.data.check_invariants(InvariantLevel::Executable)
.map_err(|e| {
Expand Down

0 comments on commit 7ad0b74

Please sign in to comment.