Stop copying LogicalPlan and Exprs in `PushDownFilter` (4%-6% faster planning) #10444

alamb · 2024-05-10T15:28:15Z

~~Draft as it depends on~~

Which issue does this PR close?

Closes #10291

Rationale for this change

Follow on to #10366 from @dmitrybugakov ❤️

Make planning faster by removing clones in filter pushdown

What changes are included in this PR?

Rewrite the PushDownFilter to not copy things

Are these changes tested?

Functionally it is covered by existing tests (specifically there are unit tests in the module, the sqllogictest and tpcds planner tests)

Are there any user-facing changes?

There is no functional change

Performance improvement shows 4-6% faster for TPCH/TPDS planning

Details

++ critcmp main push_down_filter
group                                         main                                   push_down_filter
-----                                         ----                                   ----------------
logical_aggregate_with_join                   1.00  1212.9±18.64µs        ? ?/sec    1.00  1217.1±10.78µs        ? ?/sec
logical_plan_tpcds_all                        1.00    159.3±1.73ms        ? ?/sec    1.00    158.7±1.79ms        ? ?/sec
logical_plan_tpch_all                         1.01     17.0±0.19ms        ? ?/sec    1.00     16.9±0.18ms        ? ?/sec
logical_select_all_from_1000                  1.01     18.8±0.14ms        ? ?/sec    1.00     18.7±0.09ms        ? ?/sec
logical_select_one_from_700                   1.00    811.6±7.82µs        ? ?/sec    1.01   820.1±29.24µs        ? ?/sec
logical_trivial_join_high_numbered_columns    1.00    760.1±8.61µs        ? ?/sec    1.00   762.9±13.46µs        ? ?/sec
logical_trivial_join_low_numbered_columns     1.00    744.4±7.41µs        ? ?/sec    1.01    749.5±8.29µs        ? ?/sec
physical_plan_tpcds_all                       1.06   1359.1±7.59ms        ? ?/sec    1.00   1284.5±8.00ms        ? ?/sec
physical_plan_tpch_all                        1.04     92.1±1.28ms        ? ?/sec    1.00     88.3±1.23ms        ? ?/sec
physical_plan_tpch_q1                         1.09      5.1±0.08ms        ? ?/sec    1.00      4.7±0.08ms        ? ?/sec
physical_plan_tpch_q10                        1.05      4.4±0.09ms        ? ?/sec    1.00      4.2±0.08ms        ? ?/sec
physical_plan_tpch_q11                        1.07      4.0±0.08ms        ? ?/sec    1.00      3.7±0.07ms        ? ?/sec
physical_plan_tpch_q12                        1.05      3.1±0.05ms        ? ?/sec    1.00      2.9±0.05ms        ? ?/sec
physical_plan_tpch_q13                        1.04      2.1±0.04ms        ? ?/sec    1.00      2.1±0.04ms        ? ?/sec
physical_plan_tpch_q14                        1.10      2.8±0.05ms        ? ?/sec    1.00      2.5±0.05ms        ? ?/sec
physical_plan_tpch_q16                        1.05      3.8±0.09ms        ? ?/sec    1.00      3.6±0.07ms        ? ?/sec
physical_plan_tpch_q17                        1.01      3.6±0.06ms        ? ?/sec    1.00      3.5±0.08ms        ? ?/sec
physical_plan_tpch_q18                        1.04      4.0±0.06ms        ? ?/sec    1.00      3.8±0.09ms        ? ?/sec
physical_plan_tpch_q19                        1.11      6.3±0.07ms        ? ?/sec    1.00      5.7±0.08ms        ? ?/sec
physical_plan_tpch_q2                         1.05      7.9±0.06ms        ? ?/sec    1.00      7.6±0.07ms        ? ?/sec
physical_plan_tpch_q20                        1.01      4.5±0.09ms        ? ?/sec    1.00      4.5±0.09ms        ? ?/sec
physical_plan_tpch_q21                        1.05      6.2±0.07ms        ? ?/sec    1.00      6.0±0.09ms        ? ?/sec
physical_plan_tpch_q22                        1.04      3.4±0.07ms        ? ?/sec    1.00      3.3±0.07ms        ? ?/sec
physical_plan_tpch_q3                         1.05      3.2±0.06ms        ? ?/sec    1.00      3.0±0.05ms        ? ?/sec
physical_plan_tpch_q4                         1.05      2.4±0.05ms        ? ?/sec    1.00      2.2±0.04ms        ? ?/sec
physical_plan_tpch_q5                         1.04      4.5±0.06ms        ? ?/sec    1.00      4.4±0.07ms        ? ?/sec
physical_plan_tpch_q6                         1.07  1615.0±89.07µs        ? ?/sec    1.00  1505.8±20.04µs        ? ?/sec
physical_plan_tpch_q7                         1.03      5.8±0.07ms        ? ?/sec    1.00      5.6±0.07ms        ? ?/sec
physical_plan_tpch_q8                         1.05      7.4±0.08ms        ? ?/sec    1.00      7.1±0.09ms        ? ?/sec
physical_plan_tpch_q9                         1.06      5.7±0.08ms        ? ?/sec    1.00      5.4±0.07ms        ? ?/sec
physical_select_all_from_1000                 1.01     61.8±1.40ms        ? ?/sec    1.00     61.1±0.27ms        ? ?/sec
physical_select_one_from_700                  1.00      3.7±0.04ms        ? ?/sec    1.00      3.7±0.05ms        ? ?/sec```

</p>
</details>

datafusion/expr/src/expr.rs

alamb · 2024-05-10T18:26:45Z

datafusion/optimizer/src/push_down_filter.rs

-    join_plan: &LogicalPlan,
-    left: &LogicalPlan,
-    right: &LogicalPlan,
+    inferred_join_predicates: Vec<Expr>,


This is refactored so that the Join is passed in, rather than a LogicalPlan and the embedded Join Plan as well as a reference to the embedded pieces

alamb · 2024-05-10T18:27:03Z

datafusion/optimizer/src/push_down_filter.rs

-                };
+    // Are there any new join predicates that can be inferred from the filter expressions?
+    let inferred_join_predicates =
+        infer_join_predicates(&join, &predicates, &on_filters)?;


pulled out to a function to make it easier to see what is going on -- the logic was not changed

alamb · 2024-05-10T18:27:53Z

datafusion/optimizer/src/push_down_filter.rs

        };

-        let child_plan = filter.input.as_ref();


Here is the core change -- instead of looking at the input by ref the PR now takes its ownership and then updates the accounting to avoid cloneing

alamb · 2024-05-13T13:14:31Z

datafusion/optimizer/src/push_down_filter.rs

-        },
-        LogicalPlan::CrossJoin(_) => Ok((true, true)),
-        _ => internal_err!("lr_is_preserved only valid for JOIN nodes"),
+fn lr_is_preserved(join_type: JoinType) -> Result<(bool, bool)> {


the plan was always a join so rather than having to re-check the plan, I changed to simply take the JoinType

alamb · 2024-05-13T13:15:58Z

datafusion/optimizer/src/push_down_filter.rs

+    predicates: &[Expr],
+    on_filters: &[Expr],
+) -> Result<Vec<Expr>> {
+    if join.join_type != JoinType::Inner {


this logic was refactored from above

alamb · 2024-05-15T18:58:53Z

@jackwener I know this is a fairly large PR, but I think you were the one who did the last major rework of this code. Do you happen to have any time to review this PR?

jackwener

A very nice job to me! Thank you @alamb .
Besides enhancing performance, I am pleased to see that the readability of this part of the code has significantly improved. During the PR review process, I found the entire workflow to be much clearer.

jackwener · 2024-05-16T02:48:00Z

datafusion/optimizer/src/push_down_filter.rs

-        },
-        LogicalPlan::CrossJoin(_) => Ok((true, true)),
-        _ => internal_err!("lr_is_preserved only valid for JOIN nodes"),
+fn lr_is_preserved(join_type: JoinType) -> Result<(bool, bool)> {


jackwener · 2024-05-16T02:54:59Z

datafusion/optimizer/src/push_down_filter.rs

+fn insert_below(
+    plan: LogicalPlan,
+    new_child: LogicalPlan,
+) -> Result<Transformed<LogicalPlan>> {


It's a great common method to me👍

Maybe I should pull it into optimizer/utils 🤔

alamb

Thank you very much for the review @jackwener 🙏

alamb · 2024-05-16T18:46:56Z

datafusion/optimizer/src/push_down_filter.rs

+fn insert_below(
+    plan: LogicalPlan,
+    new_child: LogicalPlan,
+) -> Result<Transformed<LogicalPlan>> {


Maybe I should pull it into optimizer/utils 🤔

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules labels May 10, 2024

alamb force-pushed the alamb/push_down_filter branch from 4c204e6 to a618a2e Compare May 10, 2024 17:10

alamb mentioned this pull request May 10, 2024

Add Expr::try_as_col, deprecate Expr::try_into_col (speed up optimizer) #10448

Merged

alamb changed the title ~~Stop copying LogicalPlan and Exprs in PushDownFilter~~ Stop copying LogicalPlan and Exprs in PushDownFilter (4%-6% faster planning) May 10, 2024

alamb force-pushed the alamb/push_down_filter branch from a618a2e to 79cafcb Compare May 10, 2024 19:16

Stop copying LogicalPlan and Exprs in PushDownFilter

891d415

alamb force-pushed the alamb/push_down_filter branch from 79cafcb to 891d415 Compare May 13, 2024 13:13

alamb marked this pull request as ready for review May 13, 2024 13:17

alamb commented May 14, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into alamb/push_down_filter

a44fbc5

alamb requested a review from jackwener May 15, 2024 18:58

jackwener approved these changes May 16, 2024

View reviewed changes

alamb commented May 16, 2024

View reviewed changes

Merge remote-tracking branch 'apache/main' into alamb/push_down_filter

7e02ae1

alamb merged commit 87169f0 into apache:main May 17, 2024
23 checks passed

findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024

Stop copying LogicalPlan and Exprs in PushDownFilter (apache#10444)

c706286

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop copying LogicalPlan and Exprs in `PushDownFilter` (4%-6% faster planning) #10444

Stop copying LogicalPlan and Exprs in `PushDownFilter` (4%-6% faster planning) #10444

alamb commented May 10, 2024 •

edited

Loading

alamb May 10, 2024

alamb May 10, 2024

alamb May 10, 2024

alamb May 13, 2024

jackwener May 16, 2024

alamb May 13, 2024

alamb commented May 15, 2024

jackwener left a comment

jackwener May 16, 2024

jackwener May 16, 2024

alamb May 16, 2024

alamb left a comment

alamb May 16, 2024

Stop copying LogicalPlan and Exprs in PushDownFilter (4%-6% faster planning) #10444

Stop copying LogicalPlan and Exprs in PushDownFilter (4%-6% faster planning) #10444

Conversation

alamb commented May 10, 2024 • edited Loading

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?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented May 15, 2024

jackwener left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stop copying LogicalPlan and Exprs in `PushDownFilter` (4%-6% faster planning) #10444

Stop copying LogicalPlan and Exprs in `PushDownFilter` (4%-6% faster planning) #10444

alamb commented May 10, 2024 •

edited

Loading