Extract CSE logic to `datafusion_common` #13002

peter-toth · 2024-10-18T13:02:42Z

Which issue does this PR close?

Part of #12599.

Rationale for this change

This PR moves common subexpression elimination logic to datafusion_common to be able to share it with physical expressions.

What changes are included in this PR?

This PR:

Moves CSE logic to datafusion_common::cse. The logic is splited to CSE struct, that does the elimination of common subtrees and CSEController trait, that can be implemented for different TreeNode types.
The PR contains 2 implementations.ExprCSEController can be used to eliminate Expr common subtrees, and TestTreeNodeCSEController is used with TestTreeNodes in tests. A new PhysicalExprCSEController will be added in a follow-up PR to eliminate common dyn PhysicalExpr subtrees.
Extracts the HashNode trait to be able to implement it for dyn PhysicalExpr in a folllow-up PR.
Makes TestTreeNode crate public for CSE tests.

Are these changes tested?

Yes, with existing but refactored tests.

Are there any user-facing changes?

No.

peter-toth · 2024-10-18T13:31:22Z

cc @alamb

alamb

Thank you @peter-toth -- this looks really nice to me. I have a few comment suggestions, and then I would like to run benchmarks to make sure this refactor doesn't accidentally slow down planning

But otherwise this looks great to me

alamb · 2024-10-18T19:28:23Z

datafusion/common/src/hash_node.rs

+use std::hash::Hasher;
+use std::sync::Arc;
+
+pub trait HashNode {


Could we please add some documentation to this trait about its purpose?

Sure, I've added it in 1cae61c.

alamb · 2024-10-18T19:29:47Z

datafusion/expr/src/expr.rs


+impl HashNode for Expr {


datafusion/common/src/cse.rs

alamb · 2024-10-18T19:33:30Z

datafusion/common/src/cse.rs

+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+


Would it be possible to add some documentation explaining what is in the module and what the main entry points are/

Something like

Suggested change

//! Common Subexpression Elimination logic: [`SubTreeNodeEliminator`]

Sure, good idea. Added i documentation in 1cae61c.

alamb · 2024-10-18T19:34:28Z

FYI @andygrove -- CSE on physical plans is happening

alamb · 2024-10-20T13:06:00Z

I ran the benchmarks and there is no change in performance 🚀

++ critcmp main extract-cse-logic
group                                         extract-cse-logic                      main
-----                                         -----------------                      ----
logical_aggregate_with_join                   1.01  1420.8±40.68µs        ? ?/sec    1.00  1408.6±14.31µs        ? ?/sec
logical_plan_tpcds_all                        1.00    194.6±1.41ms        ? ?/sec    1.00    194.1±2.83ms        ? ?/sec
logical_plan_tpch_all                         1.00     24.0±0.24ms        ? ?/sec    1.01     24.1±0.30ms        ? ?/sec
logical_select_all_from_1000                  1.01      5.4±0.06ms        ? ?/sec    1.00      5.3±0.05ms        ? ?/sec
logical_select_one_from_700                   1.00  1130.2±22.28µs        ? ?/sec    1.00  1127.6±13.80µs        ? ?/sec
logical_trivial_join_high_numbered_columns    1.00  1091.3±34.33µs        ? ?/sec    1.00  1094.4±27.40µs        ? ?/sec
logical_trivial_join_low_numbered_columns     1.01  1073.2±28.06µs        ? ?/sec    1.00  1065.6±17.13µs        ? ?/sec
physical_plan_tpcds_all                       1.01  1237.2±21.72ms        ? ?/sec    1.00  1228.5±13.18ms        ? ?/sec
physical_plan_tpch_all                        1.00     80.3±0.50ms        ? ?/sec    1.00     80.5±0.72ms        ? ?/sec
physical_plan_tpch_q1                         1.02      3.0±0.05ms        ? ?/sec    1.00      2.9±0.02ms        ? ?/sec
physical_plan_tpch_q10                        1.03      4.2±0.10ms        ? ?/sec    1.00      4.1±0.09ms        ? ?/sec
physical_plan_tpch_q11                        1.02      3.5±0.07ms        ? ?/sec    1.00      3.5±0.05ms        ? ?/sec
physical_plan_tpch_q12                        1.01      2.8±0.04ms        ? ?/sec    1.00      2.8±0.02ms        ? ?/sec
physical_plan_tpch_q13                        1.00      2.2±0.04ms        ? ?/sec    1.02      2.3±0.04ms        ? ?/sec
physical_plan_tpch_q14                        1.00      2.5±0.02ms        ? ?/sec    1.02      2.6±0.06ms        ? ?/sec
physical_plan_tpch_q16                        1.00      3.5±0.08ms        ? ?/sec    1.00      3.6±0.07ms        ? ?/sec
physical_plan_tpch_q17                        1.00      3.2±0.03ms        ? ?/sec    1.01      3.2±0.04ms        ? ?/sec
physical_plan_tpch_q18                        1.02      3.8±0.08ms        ? ?/sec    1.00      3.7±0.03ms        ? ?/sec
physical_plan_tpch_q19                        1.00      5.1±0.07ms        ? ?/sec    1.00      5.1±0.05ms        ? ?/sec
physical_plan_tpch_q2                         1.01      6.7±0.14ms        ? ?/sec    1.00      6.7±0.04ms        ? ?/sec
physical_plan_tpch_q20                        1.00      4.1±0.04ms        ? ?/sec    1.00      4.1±0.04ms        ? ?/sec
physical_plan_tpch_q21                        1.00      5.3±0.03ms        ? ?/sec    1.00      5.4±0.03ms        ? ?/sec
physical_plan_tpch_q22                        1.00      3.1±0.03ms        ? ?/sec    1.04      3.3±0.10ms        ? ?/sec
physical_plan_tpch_q3                         1.01      3.1±0.07ms        ? ?/sec    1.00      3.0±0.05ms        ? ?/sec
physical_plan_tpch_q4                         1.00      2.3±0.03ms        ? ?/sec    1.00      2.3±0.05ms        ? ?/sec
physical_plan_tpch_q5                         1.00      4.1±0.07ms        ? ?/sec    1.01      4.2±0.09ms        ? ?/sec
physical_plan_tpch_q6                         1.01  1685.3±43.27µs        ? ?/sec    1.00  1667.0±27.51µs        ? ?/sec
physical_plan_tpch_q7                         1.00      5.2±0.04ms        ? ?/sec    1.02      5.3±0.13ms        ? ?/sec
physical_plan_tpch_q8                         1.02      6.4±0.17ms        ? ?/sec    1.00      6.3±0.12ms        ? ?/sec
physical_plan_tpch_q9                         1.04      5.1±0.15ms        ? ?/sec    1.00      4.9±0.03ms        ? ?/sec
physical_select_all_from_1000                 1.01     40.6±0.19ms        ? ?/sec    1.00     40.1±0.89ms        ? ?/sec
physical_select_one_from_700                  1.00      3.6±0.03ms        ? ?/sec    1.00      3.6±0.03ms        ? ?/sec

alamb

Thank you @peter-toth

I do think some of the suggestions I had for comments would help this code, but I also think we could do it as a follow on PR

peter-toth · 2024-10-20T13:19:51Z

Thanks @alamb for the benchmark, I will address your comments tomorrow in this PR.

… shorter names for eliminator and controller, change `CSE::extract_common_nodes()` to return `Result<FoundCommonNodes<N>>` (instead of `Result<Transformed<FoundCommonNodes<N>>>`)

# Conflicts: # datafusion-cli/Cargo.lock

peter-toth · 2024-10-20T18:55:42Z

Thank you @peter-toth

I do think some of the suggestions I had for comments would help this code, but I also think we could do it as a follow on PR

I've fixed a few things in 1cae61c and addressed your comments.

alamb

Thanks @peter-toth

alamb · 2024-10-21T19:46:27Z

datafusion/common/src/cse.rs

@@ -108,7 +131,8 @@ type CommonNodes<'n, N> = IndexMap<Identifier<'n, N>, (N, String)>;

 type ChildrenList<N> = (Vec<N>, Vec<N>);

-pub trait SubTreeNodeEliminatorController {
+/// The [`TreeNode`] specific definition of elimination.
+pub trait CSEController {


👍 for the new name

peter-toth · 2024-10-22T08:15:49Z

Thanks for the review @alamb!

Extract CSE logic

807d186

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules common Related to common crate labels Oct 18, 2024

peter-toth mentioned this pull request Oct 18, 2024

Implement physical optimizer rule for common subexpression elimination #12599

Open

alamb reviewed Oct 18, 2024

View reviewed changes

alamb approved these changes Oct 20, 2024

View reviewed changes

peter-toth added 2 commits October 20, 2024 20:47

address review comments, move HashNode to datafusion_common::cse,…

1cae61c

… shorter names for eliminator and controller, change `CSE::extract_common_nodes()` to return `Result<FoundCommonNodes<N>>` (instead of `Result<Transformed<FoundCommonNodes<N>>>`)

Merge branch 'main' into extract-cse-logic

b9171e1

# Conflicts: # datafusion-cli/Cargo.lock

peter-toth mentioned this pull request Oct 21, 2024

Add Common Subexpression Elimination for PhysicalExpr trees #13046

Open

alamb approved these changes Oct 21, 2024

View reviewed changes

alamb merged commit 45a316c into apache:main Oct 21, 2024
26 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract CSE logic to `datafusion_common` #13002

Extract CSE logic to `datafusion_common` #13002

peter-toth commented Oct 18, 2024 •

edited

Loading

peter-toth commented Oct 18, 2024

alamb left a comment

alamb Oct 18, 2024

peter-toth Oct 20, 2024

alamb Oct 18, 2024

alamb Oct 18, 2024

peter-toth Oct 20, 2024

alamb commented Oct 18, 2024

alamb commented Oct 20, 2024

alamb left a comment

peter-toth commented Oct 20, 2024

peter-toth commented Oct 20, 2024

alamb left a comment

alamb Oct 21, 2024

peter-toth commented Oct 22, 2024



	//! Common Subexpression Elimination logic: [`SubTreeNodeEliminator`]

Extract CSE logic to datafusion_common #13002

Extract CSE logic to datafusion_common #13002

Conversation

peter-toth commented Oct 18, 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?

peter-toth commented Oct 18, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Oct 18, 2024

Choose a reason for hiding this comment

peter-toth Oct 20, 2024

Choose a reason for hiding this comment

alamb Oct 18, 2024

Choose a reason for hiding this comment

alamb Oct 18, 2024

Choose a reason for hiding this comment

peter-toth Oct 20, 2024

Choose a reason for hiding this comment

alamb commented Oct 18, 2024

alamb commented Oct 20, 2024

alamb left a comment

Choose a reason for hiding this comment

peter-toth commented Oct 20, 2024

peter-toth commented Oct 20, 2024

alamb left a comment

Choose a reason for hiding this comment

alamb Oct 21, 2024

Choose a reason for hiding this comment

peter-toth commented Oct 22, 2024

Extract CSE logic to `datafusion_common` #13002

Extract CSE logic to `datafusion_common` #13002

peter-toth commented Oct 18, 2024 •

edited

Loading