Improve simplify_expressions rule

[xudong963]

Which issue does this PR close?

• Closes #.

Rationale for this change

Before:

[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::utils] Optimizer input (pass 0):
    Projection: t.a
      Filter: CAST(t.a AS Int64) BETWEEN Int64(3) AND Int64(3)
        TableScan: t
    
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::optimizer] Plan unchanged by optimizer rule 'eliminate_nested_union' (pass 0)
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::utils] simplify_expressions:
    Projection: t.a
      Filter: t.a = Int32(3)
        TableScan: t
    
...
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::utils] push_down_filter:
    Projection: t.a
      Filter: t.a = Int32(3)
        TableScan: t
    
...
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::utils] optimize_projections:
    Filter: t.a = Int32(3)
      TableScan: t projection=[a]
    
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::utils] Optimized plan (pass 0):
    Filter: t.a = Int32(3)
      TableScan: t projection=[a]
    
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::utils] Optimizer input (pass 1):
    Filter: t.a = Int32(3)
      TableScan: t projection=[a]
    
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::optimizer] Plan unchanged by optimizer rule 'eliminate_nested_union' (pass 1)
[2025-04-16T10:30:23Z DEBUG datafusion_optimizer::utils] simplify_expressions:
    Filter: t.a = Int32(3)
      TableScan: t projection=[a]
    
...

Now

[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::utils] Optimizer input (pass 0):
    Projection: t.a
      Filter: CAST(t.a AS Int64) BETWEEN Int64(3) AND Int64(3)
        TableScan: t
    
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::optimizer] Plan unchanged by optimizer rule 'eliminate_nested_union' (pass 0)
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::utils] simplify_expressions:
    Projection: t.a
      Filter: t.a = Int32(3)
        TableScan: t
    
...
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::utils] push_down_filter:
    Projection: t.a
      Filter: t.a = Int32(3)
        TableScan: t
    
...
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::utils] optimize_projections:
    Filter: t.a = Int32(3)
      TableScan: t projection=[a]
    
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::utils] Optimized plan (pass 0):
    Filter: t.a = Int32(3)
      TableScan: t projection=[a]
    
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::utils] Optimizer input (pass 1):
    Filter: t.a = Int32(3)
      TableScan: t projection=[a]
    
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::optimizer] Plan unchanged by optimizer rule 'eliminate_nested_union' (pass 1)
[2025-04-16T10:46:09Z DEBUG datafusion_optimizer::optimizer] Plan unchanged by optimizer rule 'simplify_expressions' (pass 1)

What changes are included in this PR?

Check if expr is simplified, if not, return Transformed::no

Are these changes tested?

By existing tests.

For specific influence, it's difficult to check by test, so I put the log to check

Are there any user-facing changes?

[jayzhan211]

I think what we need to do is return Transformed for simplify_with_cycle_count. Your change introduce additional clone

datafusion/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

Lines 201 to 231 in 3818b7a

	pub fn simplify_with_cycle_count(&self, mut expr: Expr) -> Result<(Expr, u32)> {
	let mut simplifier = Simplifier::new(&self.info);
	let mut const_evaluator = ConstEvaluator::try_new(self.info.execution_props())?;
	let mut shorten_in_list_simplifier = ShortenInListSimplifier::new();
	let mut guarantee_rewriter = GuaranteeRewriter::new(&self.guarantees);
	if self.canonicalize {
	expr = expr.rewrite(&mut Canonicalizer::new()).data()?
	}
	// Evaluating constants can enable new simplifications and
	// simplifications can enable new constant evaluation
	// see `Self::with_max_cycles`
	let mut num_cycles = 0;
	loop {
	let Transformed {
	data, transformed, ..
	} = expr
	.rewrite(&mut const_evaluator)?
	.transform_data(|expr| expr.rewrite(&mut simplifier))?
	.transform_data(|expr| expr.rewrite(&mut guarantee_rewriter))?;
	expr = data;
	num_cycles += 1;
	if !transformed || num_cycles >= self.max_simplifier_cycles {
	break;
	}
	}
	// shorten inlist should be started after other inlist rules are applied
	expr = expr.rewrite(&mut shorten_in_list_simplifier).data()?;
	Ok((expr, num_cycles))
	}

[xudong963]

I think what we need to do is return Transformed for simplify_with_cycle_count. Your change introduce additional clone

datafusion/datafusion/optimizer/src/simplify_expressions/expr_simplifier.rs

Lines 201 to 231 in 3818b7a

	pub fn simplify_with_cycle_count(&self, mut expr: Expr) -> Result<(Expr, u32)> {
	let mut simplifier = Simplifier::new(&self.info);
	let mut const_evaluator = ConstEvaluator::try_new(self.info.execution_props())?;
	let mut shorten_in_list_simplifier = ShortenInListSimplifier::new();
	let mut guarantee_rewriter = GuaranteeRewriter::new(&self.guarantees);
	if self.canonicalize {
	expr = expr.rewrite(&mut Canonicalizer::new()).data()?
	}
	// Evaluating constants can enable new simplifications and
	// simplifications can enable new constant evaluation
	// see `Self::with_max_cycles`
	let mut num_cycles = 0;
	loop {
	let Transformed {
	data, transformed, ..
	} = expr
	.rewrite(&mut const_evaluator)?
	.transform_data(|expr| expr.rewrite(&mut simplifier))?
	.transform_data(|expr| expr.rewrite(&mut guarantee_rewriter))?;
	expr = data;
	num_cycles += 1;
	if !transformed || num_cycles >= self.max_simplifier_cycles {
	break;
	}
	}
	// shorten inlist should be started after other inlist rules are applied
	expr = expr.rewrite(&mut shorten_in_list_simplifier).data()?;
	Ok((expr, num_cycles))
	}

Sounds good, will give a try

[xudong963]

Only changed the simplify_with_cycle_count API, keep the simplify API to avoid two API changes.

[alamb]

Maybe after we merge this one in, it would be worth considering changing the simplify API as well -- but I agree that would be good as a follow on pR

[alamb]

Thank you @xudong963 -- this looks great to me. I'll try and run some planning benchmarks on this too

cc @erratic-pattern as you wrote some of this code initially

[alamb]

Maybe after we merge this one in, it would be worth considering changing the simplify API as well -- but I agree that would be good as a follow on pR

[alamb]

BTW I tried to run the planning benchmarks to see if this made things better, but sadly I found a bug:

• Cargo bench --bench sql_planner is failing #15753

[alamb]

Thanks @xudong963