Move the extract_join_keys to optimizer #4711

ygf11 · 2022-12-22T11:41:15Z

Which issue does this PR close?

Closes #4710.

Rationale for this change

See #4710

What changes are included in this PR?

Remove the usage of extract join keys in sql planner.
Add ExtractEquijoinPredicate to optimizer.

Are these changes tested?

Yes.

Are there any user-facing changes?

alamb

Thanks @ygf11 -- this PR looks very nice 👌

alamb · 2022-12-23T21:18:31Z

datafusion/sql/src/planner.rs

@@ -806,7 +805,7 @@ impl<'a, S: ContextProvider> SqlToRel<'a, S> {
    ) -> Result<LogicalPlan> {
        match constraint {
            JoinConstraint::On(sql_expr) => {
-                let mut keys: Vec<(Expr, Expr)> = vec![];
+                // let mut keys: Vec<(Expr, Expr)> = vec![];


maybe a leftover?

Sorry, I forget it. Removed now.

alamb · 2022-12-23T21:19:48Z

datafusion/optimizer/src/extract_equijoin_predicate.rs

+
+                Ok(plan)
+            }
+            _ => Ok(Some(optimize_children(self, plan, config)?)),


@jackwener has been removing direct recursion in optimizer rules -- for example see #4687

Perhaps we could do the same with this rule too

Yes, we can do it.

Utilizing the traverse of optimizer now.

alamb · 2022-12-23T21:20:32Z

datafusion/optimizer/src/extract_equijoin_predicate.rs

+            )?
+            .build()?;
+        let expected = vec![
+            "Left Join: t1.a = t2.a [a:UInt32, b:UInt32, c:UInt32, a:UInt32, b:UInt32, c:UInt32]",


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

datafusion/optimizer/src/extract_equijoin_predicate.rs

+                Some(binary_expr(
+                    (col("t1.a") + lit(10i64)).gt_eq(col("t2.a") * lit(2u32)),
+                    Operator::And,
+                    col("t1.b").lt(lit(100i32)),
+                )),


I thunk you can use .and here if you want (untested)

Suggested change

Some(binary_expr(

(col("t1.a") + lit(10i64)).gt_eq(col("t2.a") * lit(2u32)),

Operator::And,

col("t1.b").lt(lit(100i32)),

)),

Some(

col("t1.a") + lit(10i64)).gt_eq(col("t2.a") * lit(2u32))

.and(col("t1.b").lt(lit(100i32)))

)),

It is short and clear, fixed.

alamb · 2022-12-23T21:22:40Z

datafusion/optimizer/src/extract_equijoin_predicate.rs

+}
+
+#[cfg(test)]
+mod tests {


👍 nice tests

jackwener

A nice job to me👍 except a possible problem.

jackwener · 2022-12-24T07:00:51Z

datafusion/sql/src/planner.rs

-                // expression that didn't match equi-join pattern
-                let mut filter = vec![];
-
-                // extract join keys


Happy to see them are removed from planner

jackwener · 2022-12-24T07:09:48Z

I'm not sure if some special case will meet problem. like:

A join (B join C on B.id = C.id) on A.id = B.id and B.key = C.key.

B.key = C.key will be pushdown in PushdownFilter. But now this rule is front of it. Can this rule ignore B.key = C.key? or we need reorder rule?

jackwener · 2022-12-24T07:41:24Z

I'm not sure if some special case will meet problem. like:

A join (B join C on B.id = C.id) on A.id = B.id and B.key = C.key.

B.key = C.key will be pushdown in PushdownFilter. But now this rule is front of it. Can this rule ignore B.key = C.key? or we need reorder rule?

I think carefully it's ok, because it's OK when it exist in planner, it don't cause problem to just move it into optimizer.

and @ygf11 added a new test for it.

So LGTM.

ygf11 · 2022-12-24T07:44:47Z

@jackwener Thanks for review. I think it is ok. Following up are the reason.

But now this rule is previous. Can this rule ignore B.key = C.key? or we need reorder rule?

No, this rule will ignore B.key = C.key.

Because this rule will treat eq expression as equijoin only when each side of eq belong to one input.
For B.key = C.key, B.key and C.key are both belong to B join C, no key is belong to A so this predicate will be added to the filter.

liukun4515 · 2022-12-26T07:27:47Z

datafusion/optimizer/src/optimizer.rs

@@ -237,6 +238,7 @@ impl Optimizer {
        let rules: Vec<Arc<dyn OptimizerRule + Sync + Send>> = vec![
            Arc::new(InlineTableScan::new()),
            Arc::new(TypeCoercion::new()),
+            Arc::new(ExtractEquijoinPredicate::new()),


Some subquery will be converted to the join in the following up SubqueryFilterToJoin rule., and I am not sure if this rule is right in this position

cc @ygf11 @jackwener

Yes, this rule should be placed behind SubqueryFilterToJoin rule. @jackwener also suggested about it in #4725 (comment).

I think it is ok now, because SubqueryFilterToJoin and other similar rule directly specify the equijoin predicate, so they do not depends ExtractEquijoinPredicate currently.

I plan to do this movement after #4725 merged.

Yes, this rule should be placed behind SubqueryFilterToJoin rule. @jackwener also suggested about it in #4725 (comment).

I think it is ok now, because SubqueryFilterToJoin and other similar rule directly specify the equijoin predicate, so they do not depends ExtractEquijoinPredicate currently.

Yes, other rule will convert query to join with specified equal-join predicate and non-equal-join predicate.

I plan to do this movement after #4725 merged.

LGTM

liukun4515

LGTM
If there is no other comments until tomorrow, i will merge it.

Thanks @ygf11

ursabot · 2022-12-27T10:02:07Z

Benchmark runs are scheduled for baseline = 9331ee3 and contender = 734c211. 734c211 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

Move extract_join_keys to optimizer

017454e

github-actions bot added optimizer Optimizer rules sql SQL Planner labels Dec 22, 2022

ygf11 added 2 commits December 22, 2022 06:49

rename ExtractEquijoinExpr to ExtractEquijoinPredicate

dc69726

fix cargo clippy

1d0a2ed

ygf11 marked this pull request as ready for review December 22, 2022 13:15

alamb approved these changes Dec 23, 2022

View reviewed changes

utilize the optimizer to traverse the plan tree

4f33596

jackwener approved these changes Dec 24, 2022

View reviewed changes

add a new test

36ea387

ygf11 mentioned this pull request Dec 26, 2022

Support non-tuple expression for in-subquery to join #4725

Closed

liukun4515 reviewed Dec 26, 2022

View reviewed changes

liukun4515 approved these changes Dec 26, 2022

View reviewed changes

liukun4515 merged commit 734c211 into apache:master Dec 27, 2022

ygf11 deleted the extract-join-key branch December 27, 2022 11:26

liukun4515 mentioned this pull request Dec 30, 2022

Refactor extract_join_keys and move the ExtractEquijoinPredicate rule #4760

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move the extract_join_keys to optimizer #4711

Move the extract_join_keys to optimizer #4711

ygf11 commented Dec 22, 2022 •

edited

Loading

alamb left a comment

alamb Dec 23, 2022

ygf11 Dec 24, 2022

alamb Dec 23, 2022

ygf11 Dec 24, 2022

alamb Dec 23, 2022

alamb Dec 23, 2022

ygf11 Dec 24, 2022

alamb Dec 23, 2022

jackwener left a comment •

edited

Loading

jackwener Dec 24, 2022

jackwener commented Dec 24, 2022 •

edited

Loading

jackwener commented Dec 24, 2022 •

edited

Loading

ygf11 commented Dec 24, 2022 •

edited

Loading

liukun4515 Dec 26, 2022

liukun4515 Dec 26, 2022

ygf11 Dec 26, 2022 •

edited

Loading

liukun4515 Dec 26, 2022

liukun4515 left a comment

ursabot commented Dec 27, 2022

Move the extract_join_keys to optimizer #4711

Move the extract_join_keys to optimizer #4711

Conversation

ygf11 commented Dec 22, 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?

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackwener left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackwener commented Dec 24, 2022 • edited Loading

jackwener commented Dec 24, 2022 • edited Loading

ygf11 commented Dec 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygf11 Dec 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liukun4515 left a comment

Choose a reason for hiding this comment

ursabot commented Dec 27, 2022

ygf11 commented Dec 22, 2022 •

edited

Loading

jackwener left a comment •

edited

Loading

jackwener commented Dec 24, 2022 •

edited

Loading

jackwener commented Dec 24, 2022 •

edited

Loading

ygf11 commented Dec 24, 2022 •

edited

Loading

ygf11 Dec 26, 2022 •

edited

Loading