Support non-equijoin predicate for EliminateCrossJoin #4866

ygf11 · 2023-01-10T11:14:50Z

Which issue does this PR close?

Closes #4844.
Closes #4877

Rationale for this change

Currently datafusion will loss the join filter of inner join when run EliminateCrossJoin rule. Following are query and optimized logical plan:

explain verbose select t1.t1_id,t2.t2_id,t3.t3_id 
                 from t1 
                 inner join t2 on t1.t1_id > t2.t2_id 
                 cross join t3 
                 where t3.t3_int > t1.t1_int and t1.t1_int > t2.t2_int;

    Explain
      Projection: t1.t1_id, t2.t2_id, t3.t3_id
        Filter: t3.t3_int > t1.t1_int
          CrossJoin:
            Filter: t1.t1_int > t2.t2_int
              CrossJoin:
                TableScan: t1 projection=[t1_id, t1_int]
                TableScan: t2 projection=[t2_id, t2_int]
            TableScan: t3 projection=[t3_id, t3_int]",

We can see the t1.t1_id > t2.t2_id(join filter) is lost.

This is because EliminateCrossJoin only consider equijoin predicate.
This pr will rewrite EliminateCrossJoin, and choose the right input of join based on both equijoin and non-equijoin predicate. After this pr, the logical plan will be:

      Projection: t1.t1_id, t2.t2_id, t3.t3_id
        Inner Join:  Filter: t3.t3_int > t1.t1_int
          Inner Join:  Filter: t1.t1_int > t2.t2_int AND t1.t1_id > t2.t2_id
            TableScan: t1 projection=[t1_id, t1_int]
            TableScan: t2 projection=[t2_id, t2_int]
          TableScan: t3 projection=[t3_id, t3_int]

The join filter does not lost.

What changes are included in this PR?

Are these changes tested?

Yes.

Are there any user-facing changes?

jackwener · 2023-01-11T04:16:53Z

datafusion/optimizer/src/eliminate_cross_join.rs

-                left_input.schema().clone(),
-                right_input.schema().clone(),
-            )?;
+        let predicate_schemas =


we should find column compare pair. like t1.a > t2.a or t1.a > t2.a + 1.
If left or right don't contains column, we shouldn't put them into join_filter.

ygf11 · 2023-01-11T06:53:40Z

datafusion/optimizer/src/eliminate_cross_join.rs

+                        is_valid_join_predicate(expr, &predicate_schemas)?;
+                    if is_join_filter {
+                        join_filters.push(expr.clone());
+                    }


@jackwener, this try_fold will filter join filters.

Suppose the possible_join_predicates:

t1.a > 10

t1.c = t2.c

t3.b > t2.b

for t1 join t2, the try_fold will return t1.a > 1 and t1.c = t2.c, because all columns of these exprs are from t1 and t2.

This logic is the same as you comment.

I will improve the code readability after the CI fix.

I think the try_fold result is not correct, t1.a > 1 should not be treated as join filters(Inner join/Cross join cases).

jackwener · 2023-01-11T07:19:57Z

datafusion/core/tests/sql/predicates.rs

-        "          TableScan: lineitem projection=[l_partkey, l_quantity], partial_filters=[lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) OR lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) OR lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2)] [l_partkey:Int64, l_quantity:Decimal128(15, 2)]",
-        "        Filter: (part.p_brand = Utf8(\"Brand#12\") AND part.p_size <= Int32(5) OR part.p_brand = Utf8(\"Brand#23\") AND part.p_size <= Int32(10) OR part.p_brand = Utf8(\"Brand#34\") AND part.p_size <= Int32(15)) AND part.p_size >= Int32(1) [p_partkey:Int64, p_brand:Utf8, p_size:Int32]",
-        "          TableScan: part projection=[p_partkey, p_brand, p_size], partial_filters=[part.p_size >= Int32(1), part.p_brand = Utf8(\"Brand#12\") AND part.p_size <= Int32(5) OR part.p_brand = Utf8(\"Brand#23\") AND part.p_size <= Int32(10) OR part.p_brand = Utf8(\"Brand#34\") AND part.p_size <= Int32(15)] [p_partkey:Int64, p_brand:Utf8, p_size:Int32]",
+        "    Inner Join: lineitem.l_partkey = part.p_partkey Filter: part.p_brand = Utf8(\"Brand#12\") AND lineitem.l_quantity >= Decimal128(Some(100),15,2) AND lineitem.l_quantity <= Decimal128(Some(1100),15,2) AND part.p_size <= Int32(5) OR part.p_brand = Utf8(\"Brand#23\") AND lineitem.l_quantity >= Decimal128(Some(1000),15,2) AND lineitem.l_quantity <= Decimal128(Some(2000),15,2) AND part.p_size <= Int32(10) OR part.p_brand = Utf8(\"Brand#34\") AND lineitem.l_quantity >= Decimal128(Some(2000),15,2) AND lineitem.l_quantity <= Decimal128(Some(3000),15,2) AND part.p_size <= Int32(15) [l_partkey:Int64, l_quantity:Decimal128(15, 2), p_partkey:Int64, p_brand:Utf8, p_size:Int32]",


Like part.p_brand = Utf8(\"Brand#12\").
left or right don't contain column, should keep in filter

@ygf11 .

This logic is the same as you comment.

Look like it exists problem.

Sorry, it's different from you comment.

I think both are correct, because the above expression is only related to part(part is one of the input).
But I prefer join filter, because our physical join executor support it, although the above expression will push down.

Do you have any concern? could you explain more?

The new plan looks not an optimized plan, we should differ join conditions and filter conditions.

mingmwang · 2023-01-16T07:56:27Z

datafusion/optimizer/src/rewrite_disjunctive_predicate.rs

+                    ) => {
+                        l_op == r_op && ((ll == rl && lr == rr) || (ll == rr && lr == rl))
+                    }
+                    _ => false,


Should we allow 'A < B' eq 'B > A' ?

ozankabak · 2023-01-21T03:06:17Z

Any thoughts on how to make progress on this?

ygf11 · 2023-01-22T12:26:57Z

Any thoughts on how to make progress on this?

Thanks for asking @ozankabak . It is blocking by #5022, seems NestedLoopJoin has bug, we need fix it first.

epsio-banay · 2023-09-10T12:34:16Z

Hey @ygf11, I saw the blocking PR was merged, do you think you'll get back to it soon?
Thanks anyway, this PR is really helpful

alamb · 2024-04-08T21:04:36Z

Since this has been open for more than a year, closing it down. Feel free to reopen if/when you keep working on it.

Support non-equijoin predicate for EliminateCrossJoin

17a1270

github-actions bot added core Core DataFusion crate logical-expr Logical plan and expressions optimizer Optimizer rules labels Jan 10, 2023

resolve merge conflict

52585f1

github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Jan 10, 2023

ygf11 added 5 commits January 10, 2023 07:29

add tests

a95ee41

remove unused file

e4c7621

fix carg fmt

2ddf931

fix cargo clippy

5ed47b8

remove comment

a7b0e2a

ygf11 mentioned this pull request Jan 10, 2023

Incorrect results for join condition against current master branch #4844

Closed

jackwener reviewed Jan 11, 2023

View reviewed changes

ygf11 commented Jan 11, 2023

View reviewed changes

jackwener reviewed Jan 11, 2023

View reviewed changes

mingmwang reviewed Jan 16, 2023

View reviewed changes

ygf11 mentioned this pull request Jan 22, 2023

NestedLoopJoin will panic when right child contains RepartitionExec #5022

Closed

epsio-banay mentioned this pull request Sep 12, 2023

Incorrect results for nested joins after applying EliminateCrossJoin rule #7530

Closed

alamb closed this Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support non-equijoin predicate for EliminateCrossJoin #4866

Support non-equijoin predicate for EliminateCrossJoin #4866

ygf11 commented Jan 10, 2023 •

edited by alamb

Loading

jackwener Jan 11, 2023

ygf11 Jan 11, 2023

mingmwang Jan 16, 2023

jackwener Jan 11, 2023

jackwener Jan 11, 2023 •

edited

Loading

ygf11 Jan 11, 2023 •

edited

Loading

mingmwang Jan 16, 2023

mingmwang Jan 16, 2023

ozankabak commented Jan 21, 2023

ygf11 commented Jan 22, 2023 •

edited

Loading

epsio-banay commented Sep 10, 2023

alamb commented Apr 8, 2024

Support non-equijoin predicate for EliminateCrossJoin #4866

Support non-equijoin predicate for EliminateCrossJoin #4866

Conversation

ygf11 commented Jan 10, 2023 • edited by alamb 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?

jackwener Jan 11, 2023

Choose a reason for hiding this comment

ygf11 Jan 11, 2023

Choose a reason for hiding this comment

mingmwang Jan 16, 2023

Choose a reason for hiding this comment

jackwener Jan 11, 2023

Choose a reason for hiding this comment

jackwener Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

ygf11 Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

mingmwang Jan 16, 2023

Choose a reason for hiding this comment

mingmwang Jan 16, 2023

Choose a reason for hiding this comment

ozankabak commented Jan 21, 2023

ygf11 commented Jan 22, 2023 • edited Loading

epsio-banay commented Sep 10, 2023

alamb commented Apr 8, 2024

ygf11 commented Jan 10, 2023 •

edited by alamb

Loading

jackwener Jan 11, 2023 •

edited

Loading

ygf11 Jan 11, 2023 •

edited

Loading

ygf11 commented Jan 22, 2023 •

edited

Loading