fix: Incorrect LEFT JOIN evaluation result on OR conditions #11203

viirya · 2024-07-01T23:16:44Z

Which issue does this PR close?

Closes #10881.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

viirya · 2024-07-01T23:39:18Z

datafusion/optimizer/src/push_down_filter.rs

@@ -441,11 +442,11 @@ fn push_down_all_join(

    // Extract from OR clause, generate new predicates for both side of join if possible.
    // We only track the unpushable predicates above.
-    if left_preserved {
+    if on_left_preserved {


keep_predicates, join_conditions are join filter but not predicates from top Filter, we should use on_lr_is_preserved instead of lr_is_preserved.

alamb

Thank you @viirya (and everyone else who reviewed and commented on this bug)

I went through the code carefully and I pushed a few more tests to this branch. I think it looks great and makes sense to me.

I will also make a new PR to tweak some comments as well

datafusion/sqllogictest/test_files/join.slt

alamb · 2024-07-02T11:31:40Z

The CI error appears to be #11210 unrelated to this PR

mustafasrepo · 2024-07-02T12:09:07Z

datafusion/optimizer/src/push_down_filter.rs

        left_push.extend(extract_or_clauses_for_join(&keep_predicates, left_schema));
        left_push.extend(extract_or_clauses_for_join(&join_conditions, left_schema));
    }
-    if right_preserved {
+    if on_right_preserved {


We have discussed the conditions, filter predicates and on expression can be pushed down with @ozankabak .
According to us, correct handling seems to be

if left_preserved { left_push.extend(extract_or_clauses_for_join(&keep_predicates, left_schema)); } if on_left_preserved { left_push.extend(extract_or_clauses_for_join(&join_conditions, left_schema)); } if right_preserved { right_push.extend(extract_or_clauses_for_join(&keep_predicates, right_schema)); } if on_right_preserved { right_push.extend(extract_or_clauses_for_join(&join_conditions, right_schema)); }

where, for predicates and join conditions flag used is different. What do you think?

I am not sure I understand what is different about your suggestion.

My understanding of the code is that to breaks the predicates into:

keep_predicates predicates that must be left as a filter (above the join)

join_conditions predicates that must be left in the join (e.g. ON clause)

left_push predicates that can be pushed down to the left input

right_push predicates that can be pushed down to the right input

I think the code, as written, does push predicates from the ON clause down correctly when possible.

So in other words, I am not sure what changes you are suggesting

We could be missing something but it seems like keep_predicates and join_conditions should be using preserved vs. on_preserved (respectively) since the former applies as a post-join filter.

For left join, left_preserved=true and on_left_preserved=false.
In current version of the code, nothing from keep_predicates and join_conditions can be inserted to the left_push since on_left_preserved flag is false. However, if we use the flag left_preserved to decide whether we can insert something from keep_predicates, into left_push. We could have inserted additional conditions to the left_push. In this sense, current code works correctly, however behaves suboptimal in some cases.

Actually we wrongly mix join filter predicates and filter predicates into join_conditions. I separate them now so only on_filter_join_conditions will be considered with on_preserved.

Thank you for the clarifications

In current version of the code, nothing from keep_predicates and join_conditions can be inserted to the left_push since on_left_preserved flag is false

I agree, but I think this is required for correctness. At least for join_conditions it is important not to push them down to the preserved side

For example, in this query I am pretty sure it is not correct to push the predicate on A.a below the join (the intuition is that all rows from A have to have at least one row out of the join). I believe this is what this PR fixes.

SELECT .. FROM A LEFT JOIN B ON (A.a > 5)

I think is correct to push predicates on B below (as any non matching rows would have been filtered in the join anyways)

SELECT .. FROM A LEFT JOIN B ON (B.b > 5)

We could have inserted additional conditions to the left_push. In this sense, current code works correctly, however behaves suboptimal in some cases.

Can you help me write an example? I can add it to this PR and then we can handle improving the case in a future PR.

I tried to add a test case with it.

EDIT: Seems it doesn't test against the corner case. 🤔

I think we found one example -- @mustafasrepo will post shortly

alamb · 2024-07-02T13:59:18Z

Looks like my comment #11203 (comment) was in progress when @viirya changed the code in 17b2d43

Can someone help me understand what additional case this would catch (and I'll go and write some tests that cover it)?

ozankabak

I think you made a typo in your last commit (left an inline comment for it), but other than that I think it is now logically equivalent to @mustafasrepo's suggestion on #11203 (comment) (although his code seems to use a single join_conditions vector and seems more concise).

ozankabak · 2024-07-02T13:57:32Z

datafusion/optimizer/src/push_down_filter.rs

        left_push.extend(extract_or_clauses_for_join(&keep_predicates, left_schema));
        left_push.extend(extract_or_clauses_for_join(&join_conditions, left_schema));
    }
-    if on_right_preserved {
+    if left_preserved {


I think you meant right_preserved here.

mustafasrepo · 2024-07-02T14:27:17Z

Looks like my comment #11203 (comment) was in progress when @viirya changed the code in 17b2d43

Can someone help me understand what additional case this would catch (and I'll go and write some tests that cover it)?

I have constructed an example that triggers this test case (query is a bit weird. I have constructed it to trigger extract_or_clauses_for_join call):

explain SELECT e.emp_id, e.name, d.dept_name
FROM employees AS e
LEFT JOIN department AS d
ON e.emp_id = d.emp_id
WHERE ((dept_name != 'Engineering' AND e.name = 'Alice') OR (name != 'Alice' AND e.name = 'Carol'));

this query generates following plan with recent changes

+   logical_plan
+   01)Filter: d.dept_name != Utf8("Engineering") AND e.name = Utf8("Alice") OR e.name != Utf8("Alice") AND e.name = Utf8("Carol")
+   02)--Projection: e.emp_id, e.name, d.dept_name
+   03)----Left Join: e.emp_id = d.emp_id
+   04)------SubqueryAlias: e
+   05)--------Filter: employees.name = Utf8("Alice") OR employees.name != Utf8("Alice") AND employees.name = Utf8("Carol")
+   06)----------TableScan: employees projection=[emp_id, name]
+   07)------SubqueryAlias: d
+   08)--------TableScan: department projection=[emp_id, dept_name]

previously it was generating

+   logical_plan
+   01)Filter: d.dept_name != Utf8("Engineering") AND e.name = Utf8("Alice") OR e.name != Utf8("Alice") AND e.name = Utf8("Carol")
+   02)--Projection: e.emp_id, e.name, d.dept_name
+   03)----Left Join: e.emp_id = d.emp_id
+   04)------SubqueryAlias: e
+   05)--------TableScan: employees projection=[emp_id, name]
+   06)------SubqueryAlias: d
+   07)--------TableScan: department projection=[emp_id, dept_name]

where both are correct. However, first plan is a bit better

viirya · 2024-07-02T14:28:47Z

I think you made a typo in your last commit (left an inline comment for it), but other than that I think it is now logically equivalent to @mustafasrepo's suggestion on #11203 (comment) (although his code seems to use a single join_conditions vector and seems more concise).

Yea, I think it is correct. I made the change to make it more obvious to more people. Because they need to figure out that join_conditions can only contains parent Filter predicates for inner join and for inner join, both sides are preserved (either preserved or on_preserved). So for join_conditions, we can simply use on_preserved.

I can use the suggestion if you think it is better.

alamb · 2024-07-02T14:32:30Z

where both are correct. However, first plan is a bit better

Thank you @mustafasrepo - that makes sense to me. Let's also add that example to the tests (I can do so in a follow on PR or if we are going to change this PR again we can do so)

mustafasrepo · 2024-07-02T14:32:50Z

I can use the suggestion if you think it is better.

As long as we have the functionality, both versions are fine.

viirya · 2024-07-02T14:33:25Z

Thanks @mustafasrepo @ozankabak @alamb . I added the test case.

alamb · 2024-07-02T15:25:06Z

datafusion/sqllogictest/test_files/join.slt

+1 Alice HR
+3 Carol Engineering
+
+# Push down OR conditions on Filter through LEFT JOIN if possible


pretty fancy 🚀

ozankabak

Great collaboration!

ozankabak · 2024-07-03T11:58:11Z

Should we go ahead and merge? Are we waiting for more feedback?

alamb · 2024-07-03T15:01:40Z

🚀 I agree it was time to merge

Thanks again everyone

…1203) * fix: Incorrect LEFT JOIN evaluation result on OR conditions * Add a few more test cases * Don't push join filter predicates into join_conditions * Add test case and fix typo * Add test case --------- Co-authored-by: Andrew Lamb <[email protected]>

fix: Incorrect LEFT JOIN evaluation result on OR conditions

7549e8b

github-actions bot added optimizer Optimizer rules sqllogictest SQL Logic Tests (.slt) labels Jul 1, 2024

viirya commented Jul 1, 2024

View reviewed changes

alamb added 2 commits July 2, 2024 07:08

Add a few more test cases

a506bed

Merge remote-tracking branch 'apache/main' into fix_join_filter_pushdown

9cc111e

alamb approved these changes Jul 2, 2024

View reviewed changes

alamb reviewed Jul 2, 2024

View reviewed changes

datafusion/sqllogictest/test_files/join.slt Show resolved Hide resolved

alamb reviewed Jul 2, 2024

View reviewed changes

datafusion/sqllogictest/test_files/join.slt Show resolved Hide resolved

alamb mentioned this pull request Jul 2, 2024

Minor: Improve documentation about pushdown join predicates #11209

Merged

alamb mentioned this pull request Jul 2, 2024

Optimize PushDownFilter to avoid recreating schema columns #11211

Merged

mustafasrepo reviewed Jul 2, 2024

View reviewed changes

Don't push join filter predicates into join_conditions

17b2d43

ozankabak reviewed Jul 2, 2024

View reviewed changes

Add test case and fix typo

68e8838

Add test case

70bf59f

alamb reviewed Jul 2, 2024

View reviewed changes

ozankabak approved these changes Jul 2, 2024

View reviewed changes

alamb merged commit 03c8db0 into apache:main Jul 3, 2024
23 checks passed

viirya deleted the fix_join_filter_pushdown branch July 3, 2024 16:17

appletreeisyellow mentioned this pull request Jul 11, 2024

WIP(iox-11398): patched df upgrade 2024-07-02 influxdata/arrow-datafusion#31

Closed

appletreeisyellow mentioned this pull request Jul 12, 2024

WIP(iox-11398): patched df upgrade 2024-07-08 influxdata/arrow-datafusion#33

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Incorrect LEFT JOIN evaluation result on OR conditions #11203

fix: Incorrect LEFT JOIN evaluation result on OR conditions #11203

viirya commented Jul 1, 2024

viirya Jul 1, 2024

alamb left a comment

alamb commented Jul 2, 2024 •

edited

Loading

mustafasrepo Jul 2, 2024

alamb Jul 2, 2024

ozankabak Jul 2, 2024

mustafasrepo Jul 2, 2024

viirya Jul 2, 2024

alamb Jul 2, 2024

viirya Jul 2, 2024 •

edited

Loading

ozankabak Jul 2, 2024

alamb commented Jul 2, 2024

ozankabak left a comment

ozankabak Jul 2, 2024 •

edited

Loading

viirya Jul 2, 2024

mustafasrepo commented Jul 2, 2024

viirya commented Jul 2, 2024

alamb commented Jul 2, 2024

mustafasrepo commented Jul 2, 2024

viirya commented Jul 2, 2024

alamb Jul 2, 2024

ozankabak left a comment

ozankabak commented Jul 3, 2024

alamb commented Jul 3, 2024

fix: Incorrect LEFT JOIN evaluation result on OR conditions #11203

fix: Incorrect LEFT JOIN evaluation result on OR conditions #11203

Conversation

viirya commented Jul 1, 2024

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

alamb left a comment

Choose a reason for hiding this comment

alamb commented Jul 2, 2024 • edited Loading

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

viirya Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jul 2, 2024

ozankabak left a comment

Choose a reason for hiding this comment

ozankabak Jul 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mustafasrepo commented Jul 2, 2024

viirya commented Jul 2, 2024

alamb commented Jul 2, 2024

mustafasrepo commented Jul 2, 2024

viirya commented Jul 2, 2024

Choose a reason for hiding this comment

ozankabak left a comment

Choose a reason for hiding this comment

ozankabak commented Jul 3, 2024

alamb commented Jul 3, 2024

alamb commented Jul 2, 2024 •

edited

Loading

viirya Jul 2, 2024 •

edited

Loading

ozankabak Jul 2, 2024 •

edited

Loading