Refactor: Change equijoin keys from column to expression in logical join #4602

ygf11 · 2022-12-13T13:43:41Z

Which issue does this PR close?

Closes #4389.

Rationale for this change

It can make the display representation of logical join more clean.

What changes are included in this PR?

Change equijoin keys from column to expression in logical join.
Remove additional projection in parse_join.
Add additional physical projection for physical join.
Adapter the expression keys in EliminateCrossJoin, FilterNullJoinKeys, PushDownFilter, SubqueryFilterToJoin optimization rules.
Change the equijoin keys of JoinNode in proto module.
Fix and add test cases.

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, the JoinNode in proto module is also changed.

ygf11 · 2022-12-14T11:35:38Z

datafusion/core/tests/sql/joins.rs

+        let expected = if repartition_joins {
+            vec![
+                "ProjectionExec: expr=[t1_id@0 as t1_id, t2_id@2 as t2_id, t1_name@1 as t1_name]",
+                "  ProjectionExec: expr=[t1_id@0 as t1_id, t1_name@1 as t1_name, t2_id@3 as t2_id]",


It seems merging projection does not consider the ordering also.

👍 I think that could be improved as a follow on PR

I will work on it.

ygf11 · 2022-12-14T11:35:46Z

datafusion/core/tests/sql/joins.rs

+                "        CoalesceBatchesExec: target_batch_size=4096",
+                "          RepartitionExec: partitioning=Hash([Column { name: \"t1.t1_id + Int64(11)\", index: 3 }], 2)",
+                "            ProjectionExec: expr=[t1_id@0 as t1_id, t1_name@1 as t1_name, t1_int@2 as t1_int, CAST(t1_id@0 AS Int64) + 11 as t1.t1_id + Int64(11)]",
+                "              RepartitionExec: partitioning=RoundRobinBatch(2)",


The name in join and projection matches now, both are t1.t1_id + Int64(11).
cc @liukun4515

ygf11 · 2022-12-14T11:36:07Z

datafusion/core/tests/sql/joins.rs

-            "        TableScan: t2 projection=[t2_id, t2_name, t2_int] [t2_id:UInt32;N, t2_name:Utf8;N, t2_int:UInt32;N]",
+            "    Inner Join: CAST(t1.t1_id AS Int64) + Int64(12) = CAST(t2.t2_id AS Int64) + Int64(1) [t1_id:UInt32;N, t1_name:Utf8;N, t1_int:UInt32;N, t2_id:UInt32;N, t2_name:Utf8;N, t2_int:UInt32;N]",
+            "      TableScan: t1 projection=[t1_id, t1_name, t1_int] [t1_id:UInt32;N, t1_name:Utf8;N, t1_int:UInt32;N]",
+            "      TableScan: t2 projection=[t2_id, t2_name, t2_int] [t2_id:UInt32;N, t2_name:Utf8;N, t2_int:UInt32;N]",


The logical plan does not need any projections now.

ygf11 · 2022-12-14T11:37:43Z

@alamb @mingmwang @jackwener @liukun4515, please take a look, thanks.

alamb · 2022-12-14T20:57:01Z

I plan to review this carefully tomorrow.

alamb

This is a really cool PR. Thank you @ygf11

I went through the code and all the test changes carefully and I think this PR is 👨‍🍳 👌 very nice

I think it is ready to go, though plan to leave it open for another day or two in case @jackwener or anyone else would like a chance to review it.

alamb · 2022-12-15T19:55:37Z

datafusion/expr/src/logical_plan/builder.rs

+        &self,
+        right: &LogicalPlan,
+        join_type: JoinType,
+        join_keys: (Vec<impl Into<Expr>>, Vec<impl Into<Expr>>),


Maybe calling this parameter equi_exprs would better reflect what it is (exprs, not column keys) 🤔

datafusion/expr/src/logical_plan/builder.rs

alamb · 2022-12-15T19:57:29Z

datafusion/expr/src/logical_plan/plan.rs

@@ -1646,8 +1650,8 @@ pub struct Join {
    pub left: Arc<LogicalPlan>,
    /// Right input
    pub right: Arc<LogicalPlan>,
-    /// Equijoin clause expressed as pairs of (left, right) join columns
-    pub on: Vec<(Column, Column)>,
+    /// Equijoin clause expressed as pairs of (left, right) join expressions


alamb · 2022-12-15T19:58:20Z

datafusion/optimizer/src/eliminate_cross_join.rs

-              "      Projection: t2.a, t2.b, t2.c, t2.a * UInt32(2) [a:UInt32, b:UInt32, c:UInt32, t2.a * UInt32(2):UInt32]",
-              "        TableScan: t2 [a:UInt32, b:UInt32, c:UInt32]",
-        ];
+            "Filter: t2.c < UInt32(20) [a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32]", 


that is quite cool to see the expressions directly in the Join without needing a projection to compute them

alamb · 2022-12-15T20:02:05Z

datafusion/proto/src/logical_plan.rs

-                        left_keys,
-                    )?,
+                    JoinConstraint::Using => {
+                        // The equijoin keys in using-join must be column.


alamb · 2022-12-15T20:03:30Z

datafusion/sql/src/planner.rs

-        \n        TableScan: person\
-        \n      Projection: orders.order_id, orders.customer_id, orders.o_item_id, orders.qty, orders.price, orders.delivered, orders.customer_id * Int64(2)\
-        \n        TableScan: orders";
+        \n  Inner Join: person.id + Int64(10) = orders.customer_id * Int64(2)\


this plan is certainly much nicer -- I don't understand where all the other columns used to come from but this is 👍

alamb · 2022-12-15T20:07:26Z

datafusion/core/tests/sql/joins.rs

+        let expected = if repartition_joins {
+            vec![
+                "ProjectionExec: expr=[t1_id@0 as t1_id, t2_id@2 as t2_id, t1_name@1 as t1_name]",
+                "  ProjectionExec: expr=[t1_id@0 as t1_id, t1_name@1 as t1_name, t2_id@3 as t2_id]",


👍 I think that could be improved as a follow on PR

alamb · 2022-12-16T22:22:48Z

Because this PR has been outstanding for some time, and it had a conflict I took the liberty to merge from master and resolve the conflicts

I am about out of time today but I plan to merge this PR first thing tomorrow.

Thanks again @ygf11 -- really great stuff.

Co-authored-by: Andrew Lamb <[email protected]>

ygf11 · 2022-12-17T09:43:06Z

Thanks @alamb.

I resolve the remaining conflict, and ci success now.

alamb · 2022-12-17T11:06:47Z

Thanks again @ygf11 -- this is great work

ursabot · 2022-12-17T11:11:50Z

Benchmark runs are scheduled for baseline = 42b3a6c and contender = 8d36529. 8d36529 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-datafusion-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

ygf11 added 4 commits December 13, 2022 04:28

Refactor: Make equi-join keys from column to expr in logical join plan

e084b30

resolve conflicts

06b9f2f

add tests

162f1db

remove unused code

ca655d1

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner labels Dec 13, 2022

ygf11 added 3 commits December 14, 2022 03:08

fix test

ee4f9d9

add test

d1cddfa

remove unused code

51a7045

ygf11 marked this pull request as ready for review December 14, 2022 11:30

ygf11 commented Dec 14, 2022

View reviewed changes

alamb approved these changes Dec 15, 2022

View reviewed changes

alamb added 3 commits December 16, 2022 17:16

Merge remote-tracking branch 'apache/master' into refactor-logical-join

e3ab8c3

fmt

1234b80

clippy

9915b82

ygf11 and others added 3 commits December 17, 2022 02:11

resolve merge conflict

7af5e02

Update datafusion/expr/src/logical_plan/builder.rs

34c5186

Co-authored-by: Andrew Lamb <[email protected]>

rename join_keys to equi_exprs

cf5d42c

alamb merged commit 8d36529 into apache:master Dec 17, 2022

ygf11 deleted the refactor-logical-join branch December 18, 2022 06:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: Change equijoin keys from column to expression in logical join #4602

Refactor: Change equijoin keys from column to expression in logical join #4602

ygf11 commented Dec 13, 2022

ygf11 Dec 14, 2022

alamb Dec 15, 2022

ygf11 Dec 17, 2022

ygf11 Dec 14, 2022

ygf11 Dec 14, 2022

ygf11 commented Dec 14, 2022

alamb commented Dec 14, 2022

alamb left a comment

alamb Dec 15, 2022

alamb Dec 15, 2022

alamb Dec 15, 2022

alamb Dec 15, 2022

alamb Dec 15, 2022

alamb Dec 15, 2022

alamb commented Dec 16, 2022

ygf11 commented Dec 17, 2022 •

edited

Loading

alamb commented Dec 17, 2022

ursabot commented Dec 17, 2022

Refactor: Change equijoin keys from column to expression in logical join #4602

Refactor: Change equijoin keys from column to expression in logical join #4602

Conversation

ygf11 commented Dec 13, 2022

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

ygf11 commented Dec 14, 2022

alamb commented Dec 14, 2022

alamb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Dec 16, 2022

ygf11 commented Dec 17, 2022 • edited Loading

alamb commented Dec 17, 2022

ursabot commented Dec 17, 2022

ygf11 commented Dec 17, 2022 •

edited

Loading