Skip to content

Consolidate optimizer passes to improve planning speed #15045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
alamb opened this issue Mar 6, 2025 · 3 comments
Open

Consolidate optimizer passes to improve planning speed #15045

alamb opened this issue Mar 6, 2025 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@alamb
Copy link
Contributor

alamb commented Mar 6, 2025

Is your feature request related to a problem or challenge?

@jayzhan211 removed the UnwrapCastInComparison optimizer (and combined it with the simplifier) In

Doing so seemed to make a non trivial difference for planning speed:

I think the improvement came from reducing the number of Optimizer passes (and this rewrites/copies of the plan and all expressions) that happened

Here were my mesurements about speed
Image

Describe the solution you'd like

I would like to make planning faster by potentially combining other passes from this list:

let rules: Vec<Arc<dyn OptimizerRule + Sync + Send>> = vec![
Arc::new(EliminateNestedUnion::new()),
Arc::new(SimplifyExpressions::new()),
Arc::new(UnwrapCastInComparison::new()),
Arc::new(ReplaceDistinctWithAggregate::new()),
Arc::new(EliminateJoin::new()),
Arc::new(DecorrelatePredicateSubquery::new()),
Arc::new(ScalarSubqueryToJoin::new()),
Arc::new(ExtractEquijoinPredicate::new()),
Arc::new(EliminateDuplicatedExpr::new()),
Arc::new(EliminateFilter::new()),
Arc::new(EliminateCrossJoin::new()),
Arc::new(CommonSubexprEliminate::new()),
Arc::new(EliminateLimit::new()),
Arc::new(PropagateEmptyRelation::new()),
// Must be after PropagateEmptyRelation
Arc::new(EliminateOneUnion::new()),
Arc::new(FilterNullJoinKeys::default()),
Arc::new(EliminateOuterJoin::new()),
// Filters can't be pushed down past Limits, we should do PushDownFilter after PushDownLimit
Arc::new(PushDownLimit::new()),
Arc::new(PushDownFilter::new()),
Arc::new(SingleDistinctToGroupBy::new()),
// The previous optimizations added expressions and projections,
// that might benefit from the following rules
Arc::new(SimplifyExpressions::new()),
Arc::new(UnwrapCastInComparison::new()),
Arc::new(CommonSubexprEliminate::new()),
Arc::new(EliminateGroupByConstant::new()),
Arc::new(OptimizeProjections::new()),

Describe alternatives you've considered

Some potential candidates to try consolidating:

  • EliminateNestedUnion + EliminateOneUnion
  • EliminateJoin and EliminateJoin

You can run the planning benchmarks like

cargo bench --bench sql_planner

Additional context

No response

@alamb alamb added the enhancement New feature or request label Mar 6, 2025
@jayzhan211
Copy link
Contributor

jayzhan211 commented Mar 8, 2025

If we can move type coercion into LogicalPlan building stage #14618, I think Union logic is able to be resolved at that time #14380, then we don't need EliminateNestedUnion or EliminateOneUnion anymore.

But before that, we can merge these rules

@clflushopt
Copy link
Contributor

This seems like an interesting task, I'd like to work on it !

@alamb
Copy link
Contributor Author

alamb commented Mar 13, 2025

This seems like an interesting task, I'd like to work on it !

Thanks @clflushopt

Figuring out how to avoid the two distinct union passes would be great.

Maybe we could just update

pub fn union(left_plan: LogicalPlan, right_plan: LogicalPlan) -> Result<LogicalPlan> {
Ok(LogicalPlan::Union(Union::try_new_with_loose_types(vec![
Arc::new(left_plan),
Arc::new(right_plan),
])?))
}

To check if the existing plan was already a union

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants