Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning) by alamb · Pull Request #9954 · apache/datafusion

alamb · 2024-04-04T20:16:47Z

Note: A substantial number of the changes in this PR are removing & in tests. The actual logic change is quite small

Which issue does this PR close?

Part of #9637

Rationale for this change

The less copying in the planner, the less time it takes to plan a query. Profiling from thesql_planner benchmark in #9637 shows almost 25% of the time is spent in ExprSimplifier, so let's make that faster.

The current API of OptimizerRule s requires a copy of the LogicalPlan to be made. Thus we need a new API that allows for in place rewrites.

What changes are included in this PR?

Add new OptimizerRule::rewrite API for in place rewrites
Update Optimizer to use the new API
Update ExprSimplifier to implement rewrite rather than try_optimize

Open questions:

should we deprecate the old OptimizerRule API (I think so -- maybe we can do this as a follow on PR)

Are these changes tested?

Yes by existing tests

Performance benchmark: 22% faster TPC-H planning, 26% faster TPC-DS planning

Details

++ critcmp main simplify_exprs_no_copy
group main simplify_exprs_no_copy
----- ---- ----------------------
logical_aggregate_with_join 1.00 1189.1±11.74µs ? ?/sec 1.00 1186.6±10.71µs ? ?/sec
logical_plan_tpcds_all 1.00 155.7±0.85ms ? ?/sec 1.00 155.6±0.98ms ? ?/sec
logical_plan_tpch_all 1.00 16.7±0.18ms ? ?/sec 1.00 16.7±0.15ms ? ?/sec
logical_select_all_from_1000 1.06 19.3±0.11ms ? ?/sec 1.00 18.2±0.56ms ? ?/sec
logical_select_one_from_700 1.00 782.8±19.70µs ? ?/sec 1.00 780.6±8.10µs ? ?/sec
logical_trivial_join_high_numbered_columns 1.00 732.4±18.61µs ? ?/sec 1.00 732.4±9.34µs ? ?/sec
logical_trivial_join_low_numbered_columns 1.00 714.8±6.80µs ? ?/sec 1.00 716.0±6.69µs ? ?/sec
physical_plan_tpcds_all 1.26 1847.2±3.05ms ? ?/sec 1.00 1470.4±3.99ms ? ?/sec
physical_plan_tpch_all 1.22 119.8±0.74ms ? ?/sec 1.00 98.2±0.66ms ? ?/sec
physical_plan_tpch_q1 1.37 7.3±0.07ms ? ?/sec 1.00 5.4±0.02ms ? ?/sec
physical_plan_tpch_q10 1.20 5.5±0.03ms ? ?/sec 1.00 4.6±0.03ms ? ?/sec
physical_plan_tpch_q11 1.20 4.8±0.03ms ? ?/sec 1.00 4.0±0.02ms ? ?/sec
physical_plan_tpch_q12 1.19 3.9±0.03ms ? ?/sec 1.00 3.3±0.02ms ? ?/sec
physical_plan_tpch_q13 1.18 2.6±0.01ms ? ?/sec 1.00 2.2±0.01ms ? ?/sec
physical_plan_tpch_q14 1.18 3.4±0.02ms ? ?/sec 1.00 2.9±0.01ms ? ?/sec
physical_plan_tpch_q16 1.23 4.9±0.03ms ? ?/sec 1.00 4.0±0.02ms ? ?/sec
physical_plan_tpch_q17 1.24 4.6±0.03ms ? ?/sec 1.00 3.8±0.02ms ? ?/sec
physical_plan_tpch_q18 1.22 5.0±0.03ms ? ?/sec 1.00 4.1±0.02ms ? ?/sec
physical_plan_tpch_q19 1.21 9.4±0.15ms ? ?/sec 1.00 7.8±0.15ms ? ?/sec
physical_plan_tpch_q2 1.24 10.5±0.07ms ? ?/sec 1.00 8.5±0.06ms ? ?/sec
physical_plan_tpch_q20 1.26 6.1±0.04ms ? ?/sec 1.00 4.8±0.03ms ? ?/sec
physical_plan_tpch_q21 1.24 8.3±0.05ms ? ?/sec 1.00 6.7±0.05ms ? ?/sec
physical_plan_tpch_q22 1.23 4.4±0.03ms ? ?/sec 1.00 3.6±0.02ms ? ?/sec
physical_plan_tpch_q3 1.18 3.9±0.03ms ? ?/sec 1.00 3.3±0.02ms ? ?/sec
physical_plan_tpch_q4 1.20 2.9±0.02ms ? ?/sec 1.00 2.4±0.03ms ? ?/sec
physical_plan_tpch_q5 1.16 5.6±0.04ms ? ?/sec 1.00 4.8±0.03ms ? ?/sec
physical_plan_tpch_q6 1.15 1996.9±10.25µs ? ?/sec 1.00 1739.6±86.99µs ? ?/sec
physical_plan_tpch_q7 1.22 7.5±0.11ms ? ?/sec 1.00 6.2±0.07ms ? ?/sec
physical_plan_tpch_q8 1.20 9.6±0.05ms ? ?/sec 1.00 8.0±0.07ms ? ?/sec
physical_plan_tpch_q9 1.21 7.2±0.04ms ? ?/sec 1.00 6.0±0.03ms ? ?/sec
physical_select_all_from_1000 1.47 128.4±0.49ms ? ?/sec 1.00 87.1±0.31ms ? ?/sec
physical_select_one_from_700 1.10 4.0±0.03ms ? ?/sec 1.00 3.7±0.02ms ? ?/sec

Are there any user-facing changes?

Faster planning 🚀

…lifyExprs` to avoid copies

alamb · 2024-04-10T13:33:38Z

+ simplifier.simplify(e)
+}?;

- plan.with_new_exprs(exprs, new_inputs)


The whole point of this PR is to remove the calls to expressions and new_with_exprs (which requires cloning all the expressions)

alamb · 2024-04-10T13:33:40Z

 };

- let exprs = plan
- .expressions()


Calling expressions() clones all the expressions

alamb · 2024-04-10T19:26:35Z

@jackwener and @jayzhan211 here the next PR in the "don't copy so much in the optimizer"

jayzhan211 · 2024-04-10T23:55:27Z

- /// If a rule use default None, it should traverse recursively plan inside itself
+ /// If returns `None`, the default, the rule must handle recursion itself
 fn apply_order(&self) -> Option<ApplyOrder> {
 None


I think most of the cases are BottomUp by default?

I think most of the cases are BottomUp by default?
we can't assume it.

I agree most of the cases probably should be bottom up (but not all). I'll keep an eye open for this as I work through the other optimizer rules

jayzhan211

👍

alamb · 2024-04-11T10:22:28Z

Thanks @jackwener and @jayzhan211 -- I assumed it would take some time to review these PRs but you are so efficient!

I plan to spend a little time planning today to write down the remaining tasks (aka rewriting each OptimizerRule to use the new APIs) and then start cranking on them. CommonSubExprEliminate will be a tricky one but another major win I think

…lifyExprs` to avoid copies (apache#9954)

github-actionsBot added logical-expr Logical plan and expressions optimizer Optimizer rules core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Apr 4, 2024

alamb force-pushed the alamb/simplify_exprs_no_copy branch from 4f99bfc to 6be96c2Compare April 5, 2024 10:06

alamb changed the title ~~Introduce OptimizerRule::try_optimize_owned to rewrite in place~~Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifierApr 5, 2024

alamb changed the title ~~Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifier~~Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifierApr 5, 2024

This was referenced Apr 5, 2024
[EPIC] Stop copying LogicalPlan during OptimizerPasses #9637
Closed
Consistent LogicalPlan subquery handling in TreeNode::apply and TreeNode::visit #9913
Merged

alamb force-pushed the alamb/simplify_exprs_no_copy branch 3 times, most recently from 168f63f to dacbfa6Compare April 7, 2024 18:00

alamb changed the title ~~Introduce OptimizerRule::try_optimize_owned to rewrite in place, rewrite ExprSimplifier~~Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifierApr 10, 2024

alamb force-pushed the alamb/simplify_exprs_no_copy branch from dacbfa6 to d95eca4Compare April 10, 2024 11:01

github-actionsBot removed the logical-expr Logical plan and expressions label Apr 10, 2024

alamb force-pushed the alamb/simplify_exprs_no_copy branch from d95eca4 to 6d31211Compare April 10, 2024 11:20

alamb changed the title ~~Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifier~~Introduce OptimizerRule::rewrite to rewrite in place, rewrite ExprSimplifier (20% faster planning)Apr 10, 2024

Introduce OptimizerRule::rewrite to rewrite in place, Rewrite `Simp…
e31a358
…lifyExprs` to avoid copies

alamb force-pushed the alamb/simplify_exprs_no_copy branch from c9ff49b to e31a358Compare April 10, 2024 13:35

github-actionsBot removed the sqllogictest SQL Logic Tests (.slt) label Apr 10, 2024

alamb commented Apr 10, 2024
View reviewed changes

alamb marked this pull request as ready for review April 10, 2024 13:50

jayzhan211 reviewed Apr 10, 2024
View reviewed changes

jayzhan211 approved these changes Apr 10, 2024
View reviewed changes

jackwener approved these changes Apr 11, 2024
View reviewed changes

jackwener merged commit 58e0b59 into apache:mainApr 11, 2024

alamb deleted the alamb/simplify_exprs_no_copy branch April 11, 2024 10:19

appletreeisyellow pushed a commit to influxdata/arrow-datafusion that referenced this pull request Apr 24, 2024
Introduce OptimizerRule::rewrite to rewrite in place, Rewrite `Simp…
37c22e4
…lifyExprs` to avoid copies (apache#9954)

backkem mentioned this pull request Aug 23, 2024
Migrate SQLFederationOptimizerRule to OptimizerRule::rewrite datafusion-contrib/datafusion-federation#46
Closed

Navigation Menu

Uh oh!

Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning) - #9954

Introduce `OptimizerRule::rewrite` to rewrite in place, rewrite `ExprSimplifier` (20% faster planning)#9954
jackwener merged 1 commit into
apache:mainfrom
alamb:alamb/simplify_exprs_no_copy

alamb commented Apr 4, 2024•
edited
Loading

Uh oh!

alambApr 10, 2024

Uh oh!

alambApr 10, 2024•
edited
Loading

Uh oh!

alamb commented Apr 10, 2024

Uh oh!

jayzhan211Apr 10, 2024

Uh oh!

jackwenerApr 11, 2024

Uh oh!

alambApr 11, 2024

Uh oh!

jayzhan211 left a comment

Uh oh!

alamb commented Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

alamb commented Apr 4, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

alambApr 10, 2024

Choose a reason for hiding this comment

Uh oh!

alambApr 10, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 10, 2024

Uh oh!

jayzhan211Apr 10, 2024

Choose a reason for hiding this comment

Uh oh!

jackwenerApr 11, 2024

Choose a reason for hiding this comment

Uh oh!

alambApr 11, 2024

Choose a reason for hiding this comment

Uh oh!

jayzhan211 left a comment

Choose a reason for hiding this comment

Uh oh!

alamb commented Apr 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alamb commented Apr 4, 2024•
edited
Loading

alambApr 10, 2024•
edited
Loading