Add optimizer pass to reduce `left`/`right`/`full` joins to `inner` join if possible #2750

HuSen8891 · 2022-06-19T06:45:55Z

Which issue does this PR close?

Rationale for this change

try to reduce left/right/full join to inner join

for query: select ... from a left join b on ... where b.xx = 100;
if b.xx is null, and b.xx = 100 returns false, filterd those null rows.
Therefore, there is no need to produce null rows for output, we can use
inner join instead of left join.

Generally, right join/full join can also be reduced to inner join according to these rules.

What changes are included in this PR?

add reduce_outer_plan to try to reduce left/right/full join to inner join in src/planner.rs

codecov-commenter · 2022-06-19T07:17:37Z

Codecov Report

Merging #2750 (9f13704) into master (533e2b4) will increase coverage by 0.08%.
The diff coverage is 99.66%.

@@            Coverage Diff             @@
##           master    #2750      +/-   ##
==========================================
+ Coverage   85.11%   85.20%   +0.08%     
==========================================
  Files         273      274       +1     
  Lines       48240    48543     +303     
==========================================
+ Hits        41060    41361     +301     
- Misses       7180     7182       +2

Impacted Files	Coverage Δ
datafusion/optimizer/src/reduce_outer_join.rs	`99.39% <99.39%> (ø)`
datafusion/core/src/execution/context.rs	`78.56% <100.00%> (+0.02%)`	⬆️
datafusion/core/tests/sql/joins.rs	`99.31% <100.00%> (+0.20%)`	⬆️
datafusion/expr/src/logical_plan/plan.rs	`74.20% <0.00%> (-0.20%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 533e2b4...9f13704. Read the comment docs.

waynexia

I read the code and test and they look good to me. Thanks @AssHero

Comment some style things and a question.

waynexia · 2022-06-20T16:38:56Z

datafusion/sql/src/planner.rs

+/// Recursively traversese expr, if expr returns false when
+/// any inputs are null, treats columns of both sides as nonnullable columns.
+///
+/// For and/or expr, extracts from all sub exprs and merges the columns.


I guess here is

Suggested change

/// For and/or expr, extracts from all sub exprs and merges the columns.

/// For and expr, extracts from all sub exprs and merges the columns.

I guess here is

For And and Or exprs, we need to extract columns from all sub exprs.

waynexia · 2022-06-20T17:08:44Z

datafusion/sql/src/planner.rs

+fn reduce_outer_join(
+    plan: &LogicalPlan,
+    nonnullable_cols: &Vec<Column>,
+) -> Result<Option<LogicalPlan>> {


How about change this function to modify plan in place like

fn reduce_outer_join(plan: mut LogicalPlan, ...) -> Result<LogicalPlan>;

This might simplify the match

How about change this function to modify plan in place like

fn reduce_outer_join(plan: mut LogicalPlan, ...) -> Result<LogicalPlan>;

This might simplify the match

inplace update is better, and I'll check it later.

waynexia · 2022-06-20T17:31:30Z

datafusion/sql/src/planner.rs

@@ -784,6 +784,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

                let filter_expr = self.sql_to_rex(predicate_expr, &join_schema, ctes)?;

+                // reduce outer joins
+                let plans = reduce_outer_joins(plans, &filter_expr)?;


I'm not sure that whether this should be done here, or somewhere else like in optimize phase. Other parts are great ❤️ cc @alamb

I'm not sure that whether this should be done here, or somewhere else like in optimize phase. Other parts are great ❤️ cc @alamb

Here we have joins and quals from where and these make reduce possible. Please give me your suggestions, thanks!

waynexia · 2022-06-20T17:34:36Z

datafusion/sql/src/planner.rs

+                extract_nonnullable_columns(right, columns, false)
+            }
+            Operator::And => {
+                if !top_level {


Will something like Not(And(a > 10, b < 10)) hits this check and gets ignored?

Will something like Not(And(a > 10, b < 10)) hits this check and gets ignored?

YES. If not from top_level, we can handle And expr as Or expr, this is better.

andygrove · 2022-06-21T23:07:11Z

@AssHero Since this is a significant feature could you file an issue for it so that it gets included in the change logs.

andygrove · 2022-06-21T23:07:33Z

datafusion/core/tests/sql/joins.rs

+    let results = execute_to_batches(&ctx, sql).await;
+    assert_batches_sorted_eq!(expected, &results);
+
+    // could not reduce, use left join


Can we split the tests out into separate test methods?

andygrove · 2022-06-21T23:10:19Z

datafusion/sql/src/planner.rs

@@ -784,6 +784,9 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {

                let filter_expr = self.sql_to_rex(predicate_expr, &join_schema, ctes)?;

+                // reduce outer joins
+                let plans = reduce_outer_joins(plans, &filter_expr)?;


This seems like it should be implemented as an optimization rule rather than in the SQL query planner and this would mean that we would get the same optimizations regardless of whether the query was created from SQL or from the DataFrame API.

HuSen8891 · 2022-06-22T01:27:10Z

@AssHero Since this is a significant feature could you file an issue for it so that it gets included in the change logs.

yes, I'll file an issue.

HuSen8891 · 2022-06-22T04:48:17Z

Split the test cases into single test method. Implementing this as optimization rule is in progress.

alamb · 2022-06-22T13:59:32Z

Thank you -- @AssHero I will put this on my list to review carefully tomorrow. This optimization in general is quite tricky so I want to make sure I give it my full attention

HuSen8891 · 2022-06-23T15:29:31Z

Implementing reduce outer join as optimization rule. More improving is in progress.
The test part of this rule will be added later.

alamb

Thank you @AssHero -- this is looking cool. I haven't had a chance to follow it all yet but I have some suggestions.

👍

alamb · 2022-06-23T21:21:49Z

datafusion/optimizer/src/reduce_outer_join.rs

+/// For IS NOT NULL/NOT expr, always returns false for NULL input.
+///     extracts columns from these exprs.
+/// For all other exprs, fall through
+fn extract_nonnullable_columns(


What do you think about using Expr::nullable instead? That seems very similar / the same as what you are doing with this function (trying to determine if an expression can be nullable / non nullable)?

https://github.com/apache/arrow-datafusion/blob/d985c0a3b6d96b02028f3abb6edb361ea72cac14/datafusion/expr/src/expr_schema.rs#L148-L211

If Expr::nullable doesn't work correctly I think we should update that

The extract_nonnullable_columns gets all columns of these exprs which returns false if inputs are null， and we use these columns to see if they appear in null rows of output for outer join. If so, this means these null rows does not meet the conditions, we can filter null rows, reduce outer to inner. So I think the purpose of extract_nonnullable_columns is not the same as Expr::nullable.

alamb · 2022-06-23T21:23:42Z

datafusion/core/src/execution/context.rs

@@ -1229,6 +1230,7 @@ impl SessionState {
        if config.config_options.get_bool(OPT_FILTER_NULL_JOIN_KEYS) {
            rules.push(Arc::new(FilterNullJoinKeys::default()));
        }
+        rules.push(Arc::new(ReduceOuterJoin::new()));


this pass should probably be applied after FilterPushdown, so that as many predicates as possible are available

After FilterPushdown, the filters like (c1 < 100) are distributed to tables. At this time, we need to check from the bottom to up, and this need more infos to reduce outer join.

For query: select * from (select * from tt1 left join tt2 on c1 = c3) a right join tt3 on a.c2 = tt3.c5 where a.c4 < 100;
after FilterPushdown, the logical plan is

| Projection: #a.c1, #a.c2, #a.c3, #a.c4, #tt3.c5, #tt3.c6
| -Inner Join: #a.c2 = #tt3.c5
| ---Projection: #a.c1, #a.c2, #a.c3, #a.c4, alias=a
| ----Projection: #tt1.c1, #tt1.c2, #tt2.c3, #tt2.c4, alias=a
| -----Inner Join: #tt1.c1 = #tt2.c3
| ------TableScan: tt1 projection=Some([c1, c2])
| ------Filter: #tt2.c4 < Int64(100)
| -------TableScan: tt2 projection=Some([c3, c4])
| ---TableScan: tt3 projection=Some([c5, c6])

we need to walk to the tablescan, and get the filter, then back to the parent plan to reduce the outer joins, I think this is more complex and complicated to implement.

Please let me know if I miss something?

alamb · 2022-06-23T21:24:57Z

datafusion/optimizer/src/reduce_outer_join.rs

+            {
+                let mut left_nonnullable = false;
+                let mut right_nonnullable = false;
+                for col in nonnullable_cols.iter_mut() {


why iter_mut and not iter?

Let me fix this! Thanks!

alamb · 2022-06-23T21:28:18Z

datafusion/optimizer/src/reduce_outer_join.rs

+            let left_plan = reduce_outer_join(
+                _optimizer,
+                &join.left,
+                nonnullable_cols,


I wonder if it is a problem to use the same nonnullable_cols here as the call to reduce_outer_join could add new columns if a fitler is encountered somewhere down the left side (e.g. in a subquery). Maybe a clone() should be passed here (or the length remembered and truncated prior to calling reduce_outer_join on the right side)

I recognize this problem after pushing the code, use clone() here.

alamb · 2022-06-23T21:29:23Z

datafusion/optimizer/src/reduce_outer_join.rs

+                null_equals_null: join.null_equals_null,
+            }))
+        }
+        LogicalPlan::Projection(Projection {


I think Sorts can also rewrite column names?

This is for such query
select * from (select * from tt1 left join tt2 on c1 = c3) a right join tt3 on a.c2 = tt3.c5 where a.c4 < 100;

The a.c4 can be used to reduce left join to inner join, but we need to know that a.c4 is corresponding with tt2.c4.

I need to think about the case with sort.

alamb · 2022-06-23T21:31:20Z

datafusion/core/tests/sql/joins.rs

+    assert_batches_sorted_eq!(expected, &results);
+
+    Ok(())
+}


some negative tests might be good -- like that

let sql = "select * from t1 left join t2 on t1.t1_id = t2.t2_id where t2.t2_id IS NULL";

isn't rewritten to an inner join

I'll add more test cases in test part of this rule, including this.

…datafusion into reduce_outer_join

alamb

Thank you again @AssHero -- I think the test coverage in this PR is quite 👍 and we can iterate it on master as we go forward.

alamb · 2022-06-27T22:48:00Z

There was a logical conflict in this PR with #2789

I took the liberty of fixing the conflicts in d0f1f83 and 9f13704 and pushing a fix to this branch

try to reduce left/right/full join to inner join

0208a7d

github-actions bot added core Core DataFusion crate sql SQL Planner labels Jun 19, 2022

waynexia approved these changes Jun 20, 2022

View reviewed changes

waynexia reviewed Jun 20, 2022

View reviewed changes

andygrove reviewed Jun 21, 2022

View reviewed changes

split the test cases

1849604

Implementing reduce outer join as optimization rule

f8e78d5

github-actions bot added the optimizer Optimizer rules label Jun 23, 2022

Merge branch 'master' into reduce_outer_join

272eebb

alamb reviewed Jun 23, 2022

View reviewed changes

HuSen8891 added 2 commits June 25, 2022 00:57

minor improvements

362c256

Merge branch 'reduce_outer_join' of https://github.com/AssHero/arrow-…

00df944

…datafusion into reduce_outer_join

HuSen8891 requested a review from alamb June 27, 2022 12:07

alamb approved these changes Jun 27, 2022

View reviewed changes

alamb added 2 commits June 27, 2022 15:35

Merge remote-tracking branch 'apache/master' into reduce_outer_join

1b904a5

Update explain plan format

d0f1f83

Update more plans

9f13704

alamb changed the title ~~try to reduce left/right/full join to inner join~~ Add optimizer pass to reduce left/right/full joins to inner join Jun 27, 2022

alamb changed the title ~~Add optimizer pass to reduce left/right/full joins to inner join~~ Add optimizer pass to reduce left/right/full joins to inner join if possible Jun 27, 2022

alamb merged commit c8de85e into apache:master Jun 27, 2022

HuSen8891 deleted the reduce_outer_join branch June 28, 2022 01:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add optimizer pass to reduce `left`/`right`/`full` joins to `inner` join if possible #2750

Add optimizer pass to reduce `left`/`right`/`full` joins to `inner` join if possible #2750

HuSen8891 commented Jun 19, 2022 •

edited by alamb

Loading

codecov-commenter commented Jun 19, 2022 •

edited

Loading

waynexia left a comment

waynexia Jun 20, 2022

HuSen8891 Jun 21, 2022

waynexia Jun 20, 2022

HuSen8891 Jun 21, 2022

waynexia Jun 20, 2022

HuSen8891 Jun 21, 2022 •

edited

Loading

waynexia Jun 20, 2022

HuSen8891 Jun 21, 2022

andygrove commented Jun 21, 2022

andygrove Jun 21, 2022

andygrove Jun 21, 2022

HuSen8891 commented Jun 22, 2022

HuSen8891 commented Jun 22, 2022

alamb commented Jun 22, 2022

HuSen8891 commented Jun 23, 2022 •

edited

Loading

alamb left a comment

alamb Jun 23, 2022

HuSen8891 Jun 24, 2022

alamb Jun 23, 2022

HuSen8891 Jun 24, 2022 •

edited

Loading

alamb Jun 23, 2022

HuSen8891 Jun 24, 2022

alamb Jun 23, 2022

HuSen8891 Jun 24, 2022

alamb Jun 23, 2022

HuSen8891 Jun 24, 2022 •

edited

Loading

alamb Jun 23, 2022

HuSen8891 Jun 24, 2022

alamb left a comment

alamb commented Jun 27, 2022 •

edited

Loading

	/// For and/or expr, extracts from all sub exprs and merges the columns.
	/// For and expr, extracts from all sub exprs and merges the columns.

Add optimizer pass to reduce left/right/full joins to inner join if possible #2750

Add optimizer pass to reduce left/right/full joins to inner join if possible #2750

Conversation

HuSen8891 commented Jun 19, 2022 • edited by alamb Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

codecov-commenter commented Jun 19, 2022 • edited Loading

Codecov Report

waynexia 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

HuSen8891 Jun 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove commented Jun 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuSen8891 commented Jun 22, 2022

HuSen8891 commented Jun 22, 2022

alamb commented Jun 22, 2022

HuSen8891 commented Jun 23, 2022 • edited Loading

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

HuSen8891 Jun 24, 2022 • 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

HuSen8891 Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

alamb commented Jun 27, 2022 • edited Loading

Add optimizer pass to reduce `left`/`right`/`full` joins to `inner` join if possible #2750

Add optimizer pass to reduce `left`/`right`/`full` joins to `inner` join if possible #2750

HuSen8891 commented Jun 19, 2022 •

edited by alamb

Loading

codecov-commenter commented Jun 19, 2022 •

edited

Loading

HuSen8891 Jun 21, 2022 •

edited

Loading

HuSen8891 commented Jun 23, 2022 •

edited

Loading

HuSen8891 Jun 24, 2022 •

edited

Loading

HuSen8891 Jun 24, 2022 •

edited

Loading

alamb commented Jun 27, 2022 •

edited

Loading