Add support for non-column key for equijoin when eliminating cross join to inner join #4443

ygf11 · 2022-11-30T14:20:06Z

Which issue does this PR close?

Closes #4442.
Part of #4389.

Rationale for this change

Currently EliminateCrossJoin will only support eliminating cross join when the join keys are columns. We should support non-column join keys.
This pr is part of Proposal: Improve the join keys of logical plan #4389.

What changes are included in this PR?

Add logics to support non-column join key in EliminateCrossJoin.
Fix duplicated column name when join keys contain cast(column as xxx).
Add unit tests.

Are these changes tested?

Yes, unit tests will test it.

Are there any user-facing changes?

ygf11 · 2022-12-01T06:39:32Z

cc @alamb @jackwener @mingmwang

liukun4515 · 2022-12-01T10:26:08Z

@ygf11 thanks for your contribution, could you please add some IT in the datafusion/core/tests/sql/joins.rs file with your SQL like

select * from test0 as t0 cross join test1 as t1 where t0.a + 1 = t1.a * 2;
select * from test0 as t0 cross join test1 as t1 where t0.a * 2 = t1.a - 1;

ygf11 · 2022-12-02T09:31:32Z

thanks for your contribution, could you please add some IT in the datafusion/core/tests/sql/joins.rs file with your SQL like

@liukun4515 Added.

mingmwang · 2022-12-07T03:32:27Z

datafusion/core/tests/sql/joins.rs

+            "  Projection: t1.t1_id, t2.t2_id, t1.t1_name [t1_id:UInt32;N, t2_id:UInt32;N, t1_name:Utf8;N]",
+            "    Projection: t1.t1_id, t1.t1_name, t2.t2_id [t1_id:UInt32;N, t1_name:Utf8;N, t2_id:UInt32;N]",


Looks like these two projections are not collapsed. This is unrelated to this PR. But we need to take a look at the PushDownProjection rule.

cc @jackwener
maybe @jackwener has fixed that in #4487

This isn't related with this PR. This projection is generated due to keep the same order.
It should need do more optimization in PushDownProjection.

Recent I am doing a enhancement about PushDownProjection, it should can resolve this.

mingmwang · 2022-12-07T03:54:54Z

datafusion/expr/src/logical_plan/builder.rs

+            // The display_name() of cast expression will ignore the cast info, and show the inner expression name.
+            // If we do not add alais, it will throw same field name error in the schema when adding projection.
+            // For example:
+            //    input scan : [a, b, c],
+            //    join keys: [cast(a as int)]
+            //
+            //  then a and cast(a as int) will use the same field name - `a` in projection schema.


Why the Cast expressions ignore the cast info ? Looks like this is not the expected behavior.
I think no matter the Cast is explicit or implicit, it should display the cast info.

https://github.com/apache/arrow-datafusion/blob/cedb05aedf3cea030bfa8774b8575d8f4806a1c8/datafusion/expr/src/expr.rs#L1108-L1115

@andygrove @liukun4515 @jackwener
How do you think?

Yes, I think we need to consider to the Cast.

sql can explicitly include cast

after infer cast, it will generate cast.

But I need to time to think carefully about its detail.

mingmwang · 2022-12-07T07:26:48Z

The PR itself LGTM.

jackwener

LGTM generally.
But there may be some problem cooperate with other rules, but not related to this one.I will try to investigate and think about them.

alamb

I looked at several of the tests carefully and skimmed the rest of this PR -- I also appreciate @jackwener 's PR.

Thank you very much @ygf11 -- I'll plan to merge this PR tomorrow unless there are any additional comments

alamb · 2022-12-07T11:55:33Z

datafusion/core/tests/sql/joins.rs

+
+        // reduce to inner join, t2.t2_id will insert cast.
+        let sql =
+            "select t1.t1_id, t2.t2_id, t1.t1_name from t1 cross join t2 where t1.t1_id + 11 = cast(t2.t2_id as BIGINT)";


alamb · 2022-12-07T11:57:48Z

datafusion/optimizer/src/eliminate_cross_join.rs

+        let expected = vec![
+              "Filter: t2.c < UInt32(20) [a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32]",
+              "  Projection: t1.a, t1.b, t1.c, t2.a, t2.b, t2.c [a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32]",
+              "    Inner Join: t1.a + UInt32(100) = t2.a * UInt32(2) [a:UInt32, b:UInt32, c:UInt32, t1.a + UInt32(100):UInt32, a:UInt32, b:UInt32, c:UInt32, t2.a * UInt32(2):UInt32]",


this is very cool

alamb · 2022-12-07T11:58:39Z

datafusion/sql/src/planner.rs

@@ -3115,32 +3101,6 @@ fn extract_join_keys(
    Ok(())
 }

-/// Wrap projection for a plan, if the join keys contains normal expression.
-fn wrap_projection_for_join_if_necessary(


it is very nice to not do this in the planner anymore

liukun4515 · 2022-12-08T04:08:39Z

datafusion/expr/src/logical_plan/builder.rs

+        .iter()
+        .map(|key| {
+            // The display_name() of cast expression will ignore the cast info, and show the inner expression name.
+            // If we do not add alais, it will throw same field name error in the schema when adding projection.


good comments

liukun4515

Maybe we can add sql test in the follow up pr.

select *,t1.t1_id+10 from t1,t2 where t1.t1_id+10=t2.t2_id

Thanks @ygf11

ygf11 · 2022-12-09T10:49:56Z

Maybe we can add sql test in the follow up pr.

Thanks @liukun4515, I will add this test case in the follow up pr.

alamb · 2022-12-09T14:53:35Z

Thanks @ygf11 for the contribution and everyone for the reviews!

ygf11 added 2 commits November 30, 2022 08:24

Support non-column join key in eliminating cross join to inner join

ee58c29

Add comment

a6fd19d

github-actions bot added logical-expr Logical plan and expressions optimizer Optimizer rules sql SQL Planner labels Nov 30, 2022

Make clippy happy

799e3c4

ygf11 changed the title ~~Add support for non-column key for equijoin when eliminating cross join to inner~~ Add support for non-column key for equijoin when eliminating cross join to inner join Nov 30, 2022

ygf11 marked this pull request as ready for review November 30, 2022 15:12

ygf11 added 3 commits December 1, 2022 21:34

Add tests

60c86d1

Add alias for cast expr join keys

d148fce

Add tests

34f883c

github-actions bot added the core Core DataFusion crate label Dec 2, 2022

Add relative issue comment

85221fd

Improve test

d435733

mingmwang reviewed Dec 7, 2022

View reviewed changes

jackwener mentioned this pull request Dec 7, 2022

refactor: remove redundant build_join_schema() #4538

Merged

jackwener approved these changes Dec 7, 2022

View reviewed changes

Merge master into ygf11/more-equi-join

eda812c

alamb approved these changes Dec 7, 2022

View reviewed changes

ygf11 added 2 commits December 7, 2022 08:16

Merge master into ygf11/more-equi-join

a4db340

Improve use declarations

44d3099

liukun4515 reviewed Dec 8, 2022

View reviewed changes

liukun4515 approved these changes Dec 8, 2022

View reviewed changes

resolve merge conflicts

7453b53

alamb merged commit 097a3de into apache:master Dec 9, 2022

ygf11 deleted the more-equi-join branch December 10, 2022 02:21

ygf11 mentioned this pull request Dec 10, 2022

Minor: Add test case for reduce cross join #4577

Merged

jonahgao mentioned this pull request Aug 2, 2024

fix: Add additional required expression for natural join #11713

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for non-column key for equijoin when eliminating cross join to inner join #4443

Add support for non-column key for equijoin when eliminating cross join to inner join #4443

ygf11 commented Nov 30, 2022 •

edited

Loading

ygf11 commented Dec 1, 2022

liukun4515 commented Dec 1, 2022

ygf11 commented Dec 2, 2022

mingmwang Dec 7, 2022

liukun4515 Dec 7, 2022

jackwener Dec 7, 2022 •

edited

Loading

mingmwang Dec 7, 2022

jackwener Dec 7, 2022 •

edited

Loading

mingmwang commented Dec 7, 2022

jackwener left a comment •

edited

Loading

alamb left a comment

alamb Dec 7, 2022

alamb Dec 7, 2022

alamb Dec 7, 2022

liukun4515 Dec 8, 2022

liukun4515 left a comment •

edited

Loading

ygf11 commented Dec 9, 2022 •

edited

Loading

alamb commented Dec 9, 2022

		" Projection: t1.t1_id, t2.t2_id, t1.t1_name [t1_id:UInt32;N, t2_id:UInt32;N, t1_name:Utf8;N]",
		" Projection: t1.t1_id, t1.t1_name, t2.t2_id [t1_id:UInt32;N, t1_name:Utf8;N, t2_id:UInt32;N]",

Add support for non-column key for equijoin when eliminating cross join to inner join #4443

Add support for non-column key for equijoin when eliminating cross join to inner join #4443

Conversation

ygf11 commented Nov 30, 2022 • 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?

ygf11 commented Dec 1, 2022

liukun4515 commented Dec 1, 2022

ygf11 commented Dec 2, 2022

mingmwang Dec 7, 2022

Choose a reason for hiding this comment

liukun4515 Dec 7, 2022

Choose a reason for hiding this comment

jackwener Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

mingmwang Dec 7, 2022

Choose a reason for hiding this comment

jackwener Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

mingmwang commented Dec 7, 2022

jackwener left a comment • edited Loading

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb Dec 7, 2022

Choose a reason for hiding this comment

alamb Dec 7, 2022

Choose a reason for hiding this comment

alamb Dec 7, 2022

Choose a reason for hiding this comment

liukun4515 Dec 8, 2022

Choose a reason for hiding this comment

liukun4515 left a comment • edited Loading

Choose a reason for hiding this comment

ygf11 commented Dec 9, 2022 • edited Loading

alamb commented Dec 9, 2022

ygf11 commented Nov 30, 2022 •

edited

Loading

jackwener Dec 7, 2022 •

edited

Loading

jackwener Dec 7, 2022 •

edited

Loading

jackwener left a comment •

edited

Loading

liukun4515 left a comment •

edited

Loading

ygf11 commented Dec 9, 2022 •

edited

Loading