You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "mustafasrepo (via GitHub)" <gi...@apache.org> on 2023/12/22 15:31:54 UTC
[PR] Filter pushdown through cross join [arrow-datafusion]
mustafasrepo opened a new pull request, #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626
<!--
Before opening a PR. Make sure following points are double-checked
-- Make sure `cargo fmt` is done.
-- Make sure `bash dev/rust_lint.sh` successfully runs.
-- Make sure `cargo test` passes from all tests.
-- Make sure there are wide-ranging unit tests, integration tests, end-to-end tests for the added functionality.
-- Make sure there is no merge conflict with target branch.
-- If there are sections in the code, that can be written as function do so
Especially, if these sections are
- occur multiple times,
- line number is more than > 20 approximately.
-- The code added doesn't contain unnecessary `.clone`s.
-- The code added doesn't contain naked `unwrap`s.
-- Make sure imports are consistent. Imports should be in following order
- Standart library imports (alphabetically ordered within this group)
- Arrow/ Datafusion imports (alphabetically ordered within this group)
- #rd Party libraries (alphabetically ordered within this group)
where each group is seperated by an empty line.
-- Inspect your attributes to what needs to be public, do not expose something if it is not used elsewhere.
-- Make sure that the docstrings of the newly added code blocks are not missing.
-- Read through comments whether they are clear, and grammatically correct.
-- Look for whether for loops can be re-written as iterator. Be pro-functional style.
-- Do not use short names such as `res`, `elem`, instead use `result`, `item`, etc.
-- For short and common functions use namespace with it to be explicit.
(such as instead of `min`, `max`, use `std::cmp::min`, `std::cmp::max` etc.)
-- Add necessary license notice for the added code that will remain in the Synnada repo.
-- Make sure the PR body is understandable and summarizes the topic well.
-- You can use CHATGPT to convert code to idimatic style, to generate documentation.
-->
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
Current filters are not pushed down to the cross join (equality predicates are pushed down). For this reason we generate following plan for the query
```sql
SELECT *
FROM annotated_data as l, annotated_data as r
WHERE l.a > r.a
```
```sql
FilterExec: filter=a@0 > a@1
--CrossJoinExec:
----RepartitionExec: partitioning=RoundRobinBatch(2), input_partitions=1
------CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a0, a, b, c, d], output_ordering=[a@1 ASC, b@2 ASC NULLS LAST, c@3 ASC NULLS LAST], has_header=true
----CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a0, a, b, c, d], output_ordering=[a@1 ASC, b@2 ASC NULLS LAST, c@3 ASC NULLS LAST], has_header=true
```
However, if we were to pushdown filter into CrossJoin (by converting it into inner join with filter condition ) we could have produced following plan
```sql
NestedLoopJoinExec: join_type=Inner, filter=a@0 > a@1
--RepartitionExec: partitioning=RoundRobinBatch(2), input_partitions=1
----CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a0, a, b, c, d], output_ordering=[a@1 ASC, b@2 ASC NULLS LAST, c@3 ASC NULLS LAST], has_header=true
--CsvExec: file_groups={1 group: [[WORKSPACE_ROOT/datafusion/core/tests/data/window_2.csv]]}, projection=[a0, a, b, c, d], output_ordering=[a@1 ASC, b@2 ASC NULLS LAST, c@3 ASC NULLS LAST], has_header=true
```
which would be more memory efficient especially in large tables. This PR adds this support
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
Yes
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436091082
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
-///
impl OptimizerRule for EliminateCrossJoin {
fn try_optimize(
&self,
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
+ let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
+ let mut all_inputs: Vec<LogicalPlan> = vec![];
+ let parent_predicate = match plan {
LogicalPlan::Filter(filter) => {
- let input = filter.input.as_ref().clone();
-
- let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
- let mut all_inputs: Vec<LogicalPlan> = vec![];
- let did_flat_successfully = match &input {
+ let input = filter.input.as_ref();
+ match input {
LogicalPlan::Join(Join {
join_type: JoinType::Inner,
..
})
- | LogicalPlan::CrossJoin(_) => try_flatten_join_inputs(
- &input,
- &mut possible_join_keys,
- &mut all_inputs,
- )?,
+ | LogicalPlan::CrossJoin(_) => {
+ if !try_flatten_join_inputs(
+ input,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
+ return Ok(None);
+ }
+ extract_possible_join_keys(
+ &filter.predicate,
+ &mut possible_join_keys,
+ )?;
+ Some(&filter.predicate)
+ }
_ => {
return utils::optimize_children(self, plan, config);
}
- };
-
- if !did_flat_successfully {
+ }
+ }
+ LogicalPlan::Join(Join {
+ join_type: JoinType::Inner,
+ ..
+ }) => {
+ if !try_flatten_join_inputs(
+ plan,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
return Ok(None);
}
+ None
+ }
Review Comment:
I tried to add a simple test case for this feature. However, I couldn't write a simple test to reproduce this. However, without this reordering support [test](https://github.com/apache/arrow-datafusion/blob/main/datafusion/sqllogictest/test_files/tpch/q2.slt.part) fails. If I can come up with a simple test, I will add it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436134110
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
-///
impl OptimizerRule for EliminateCrossJoin {
fn try_optimize(
&self,
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
+ let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
+ let mut all_inputs: Vec<LogicalPlan> = vec![];
+ let parent_predicate = match plan {
LogicalPlan::Filter(filter) => {
- let input = filter.input.as_ref().clone();
-
- let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
- let mut all_inputs: Vec<LogicalPlan> = vec![];
- let did_flat_successfully = match &input {
+ let input = filter.input.as_ref();
+ match input {
LogicalPlan::Join(Join {
join_type: JoinType::Inner,
..
})
- | LogicalPlan::CrossJoin(_) => try_flatten_join_inputs(
- &input,
- &mut possible_join_keys,
- &mut all_inputs,
- )?,
+ | LogicalPlan::CrossJoin(_) => {
+ if !try_flatten_join_inputs(
+ input,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
+ return Ok(None);
+ }
+ extract_possible_join_keys(
+ &filter.predicate,
+ &mut possible_join_keys,
+ )?;
+ Some(&filter.predicate)
+ }
_ => {
return utils::optimize_children(self, plan, config);
}
- };
-
- if !did_flat_successfully {
+ }
+ }
+ LogicalPlan::Join(Join {
+ join_type: JoinType::Inner,
+ ..
+ }) => {
+ if !try_flatten_join_inputs(
+ plan,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
return Ok(None);
}
+ None
+ }
Review Comment:
Oh, I meant previously this rule doesn't cover the case of reordering joins to eliminate CrossJoin without a Filter on top of Join (because it did only match `LogicalPlan::Filter(filter)`. So if adding this new matching case `LogicalPlan::Join` is to address it, it means we don't have test case covering it previously. Then we may need to add one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435683995
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
Hmm, after `push_down_all_join`, if there are predicates that cannot be join condition, they will be into a `Filter` on top of join node. In the case, this function will skip to convert the under join to cross join?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436025576
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
This is how `push_down_all_join` handles remaining predicates in Filter. Does
```rust
if keep_predicates.is_empty() {
Ok(plan)
} else {
// wrap the join on the filter whose predicates must be kept
match conjunction(keep_predicates) {
Some(predicate) => Ok(LogicalPlan::Filter(Filter::try_new(
predicate,
Arc::new(plan),
)?)),
None => Ok(plan),
}
}
```
And after it, `convert_to_cross_join_if_beneficial` is called. Where does the Filter push down? Am I missing it?
```
let plan = push_down_all_join(
predicates,
vec![],
&join_plan,
left,
right,
vec![],
true,
)?;
convert_to_cross_join_if_beneficial(plan)?
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436021548
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
As far as I know, after `after push_down_all_join` predicates remaining in Filter will be pushed into the join filter. Hence, we are sure that top operator is `Join`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436025576
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
This is how `push_down_all_join` handles remaining predicates in Filter.
```rust
if keep_predicates.is_empty() {
Ok(plan)
} else {
// wrap the join on the filter whose predicates must be kept
match conjunction(keep_predicates) {
Some(predicate) => Ok(LogicalPlan::Filter(Filter::try_new( // Filter on top on Join
predicate,
Arc::new(plan), // this is Join
)?)),
None => Ok(plan),
}
}
```
And after it, `convert_to_cross_join_if_beneficial` is called. Where does the Filter push down? Am I missing it?
```
let plan = push_down_all_join(
predicates,
vec![],
&join_plan,
left,
right,
vec![],
true,
)?;
convert_to_cross_join_if_beneficial(plan)?
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436028217
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
-///
impl OptimizerRule for EliminateCrossJoin {
fn try_optimize(
&self,
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
+ let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
+ let mut all_inputs: Vec<LogicalPlan> = vec![];
+ let parent_predicate = match plan {
LogicalPlan::Filter(filter) => {
- let input = filter.input.as_ref().clone();
-
- let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
- let mut all_inputs: Vec<LogicalPlan> = vec![];
- let did_flat_successfully = match &input {
+ let input = filter.input.as_ref();
+ match input {
LogicalPlan::Join(Join {
join_type: JoinType::Inner,
..
})
- | LogicalPlan::CrossJoin(_) => try_flatten_join_inputs(
- &input,
- &mut possible_join_keys,
- &mut all_inputs,
- )?,
+ | LogicalPlan::CrossJoin(_) => {
+ if !try_flatten_join_inputs(
+ input,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
+ return Ok(None);
+ }
+ extract_possible_join_keys(
+ &filter.predicate,
+ &mut possible_join_keys,
+ )?;
+ Some(&filter.predicate)
+ }
_ => {
return utils::optimize_children(self, plan, config);
}
- };
-
- if !did_flat_successfully {
+ }
+ }
+ LogicalPlan::Join(Join {
+ join_type: JoinType::Inner,
+ ..
+ }) => {
+ if !try_flatten_join_inputs(
+ plan,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
return Ok(None);
}
+ None
+ }
Review Comment:
Can you expand on this so we also address it in the follow-on PR (in case you see a bug or gap here)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435682252
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
Review Comment:
Looks like this function only does "Convert cross join to join"?
"pushing down filter predicate" is done by `push_down_all_join`. Newbies might be confused by this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "ozankabak (via GitHub)" <gi...@apache.org>.
ozankabak commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436027784
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
I think you are right. We will discuss and file a follow-on PR today to fix.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#issuecomment-1868070671
I changed the title of this PR as I think it is more correct to say the filter is pushed "into" the join as the filter doesn't go below it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435671974
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
Review Comment:
This current doc (PushDownFilter) doesn't have such example, maybe update it with this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435993847
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
To be more precise filter can be pushed down below the join completely. In this case we may end up with joins empty equality predicate and empty filter condition.
As an example
```
Filter(l.a>l.b AND r.a>r.b)
--Join (on=[], filter=None)
----LeftTable(a, b)
----RightTable(a, b)
```
will be converted to the plan below after `push_down_all_join`
```
Join (on=[], filter=None)
--Filter(l.a>l.b)
----LeftTable(a, b)
--Filter(r.a>r.b)
----RightTable(a, b)
```
this util ensures that plan above is converted to the plan below
```
CrossJoin
--Filter(l.a>l.b)
----LeftTable(a, b)
--Filter(r.a>r.b)
----RightTable(a, b)
```
However, if the original plan were
```
Filter(l.a>r.b)
--Join (on=[], filter=None)
----LeftTable(a, b)
----RightTable(a, b)
```
after `push_down_all_join` we will end up with following plan
```
Join (on=[], filter=Some(l.a>r.b))
--LeftTable(a, b)
--RightTable(a, b)
```
in this case join top cannot be converted to the cross join.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435989391
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
Review Comment:
Exactly, I have updated the comment to reflect this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435989391
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
Review Comment:
Exactly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435671974
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
Review Comment:
The current doc (of PushDownFilter) doesn't have such example, maybe update it with this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435677232
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
-///
impl OptimizerRule for EliminateCrossJoin {
fn try_optimize(
&self,
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
+ let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
+ let mut all_inputs: Vec<LogicalPlan> = vec![];
+ let parent_predicate = match plan {
LogicalPlan::Filter(filter) => {
- let input = filter.input.as_ref().clone();
-
- let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
- let mut all_inputs: Vec<LogicalPlan> = vec![];
- let did_flat_successfully = match &input {
+ let input = filter.input.as_ref();
+ match input {
LogicalPlan::Join(Join {
join_type: JoinType::Inner,
..
})
- | LogicalPlan::CrossJoin(_) => try_flatten_join_inputs(
- &input,
- &mut possible_join_keys,
- &mut all_inputs,
- )?,
+ | LogicalPlan::CrossJoin(_) => {
+ if !try_flatten_join_inputs(
+ input,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
+ return Ok(None);
+ }
+ extract_possible_join_keys(
+ &filter.predicate,
+ &mut possible_join_keys,
+ )?;
+ Some(&filter.predicate)
+ }
_ => {
return utils::optimize_children(self, plan, config);
}
- };
-
- if !did_flat_successfully {
+ }
+ }
+ LogicalPlan::Join(Join {
+ join_type: JoinType::Inner,
+ ..
+ }) => {
+ if !try_flatten_join_inputs(
+ plan,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
return Ok(None);
}
+ None
+ }
Review Comment:
This looks like a missing case of reordering joins to eliminate CrossJoin, do we have a test case can cover this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435671974
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
Review Comment:
This current doc (PushDownFilter) only has equality predicates example, maybe update it with this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo merged PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436018505
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
I mean if after `push_down_all_join` there are predicates remaining in Filter, but Join has empty `on` and empty `filter`, because you match the plan by `if let LogicalPlan::Join(join) = &plan`, the Join won't be converted to a Cross Join even looks like it should be (based on the logic here).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435677232
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
-///
impl OptimizerRule for EliminateCrossJoin {
fn try_optimize(
&self,
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
+ let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
+ let mut all_inputs: Vec<LogicalPlan> = vec![];
+ let parent_predicate = match plan {
LogicalPlan::Filter(filter) => {
- let input = filter.input.as_ref().clone();
-
- let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
- let mut all_inputs: Vec<LogicalPlan> = vec![];
- let did_flat_successfully = match &input {
+ let input = filter.input.as_ref();
+ match input {
LogicalPlan::Join(Join {
join_type: JoinType::Inner,
..
})
- | LogicalPlan::CrossJoin(_) => try_flatten_join_inputs(
- &input,
- &mut possible_join_keys,
- &mut all_inputs,
- )?,
+ | LogicalPlan::CrossJoin(_) => {
+ if !try_flatten_join_inputs(
+ input,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
+ return Ok(None);
+ }
+ extract_possible_join_keys(
+ &filter.predicate,
+ &mut possible_join_keys,
+ )?;
+ Some(&filter.predicate)
+ }
_ => {
return utils::optimize_children(self, plan, config);
}
- };
-
- if !did_flat_successfully {
+ }
+ }
+ LogicalPlan::Join(Join {
+ join_type: JoinType::Inner,
+ ..
+ }) => {
+ if !try_flatten_join_inputs(
+ plan,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
return Ok(None);
}
+ None
+ }
Review Comment:
This looks like a missing case of reordering joins to eliminate Join, do we have a test case can cover this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#issuecomment-1868071001
@jackwener or @liukun4515 by any chance do you have time to review this PR?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436026666
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
And, the reason that the predicate is kept in Filter in `push_down_all_join` is because it cannot be pushed into Join filter or pushed down through Join. How does it push into/push down later?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435681572
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
Review Comment:
Is this comment put wrong place?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435671974
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
Review Comment:
This current doc only has equality predicates example, maybe update it with this change?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1436025576
##########
datafusion/optimizer/src/push_down_filter.rs:
##########
@@ -955,6 +965,36 @@ impl PushDownFilter {
}
}
+/// Convert cross join to join by pushing down filter predicate to the join condition
+fn convert_cross_join_to_inner_join(cross_join: CrossJoin) -> Result<Join> {
+ let CrossJoin { left, right, .. } = cross_join;
+ let join_schema = build_join_schema(left.schema(), right.schema(), &JoinType::Inner)?;
+ // predicate is given
+ Ok(Join {
+ left,
+ right,
+ join_type: JoinType::Inner,
+ join_constraint: JoinConstraint::On,
+ on: vec![],
+ filter: None,
+ schema: DFSchemaRef::new(join_schema),
+ null_equals_null: true,
+ })
+}
+
+/// Converts the inner join with empty equality predicate and empty filter condition to the cross join
+fn convert_to_cross_join_if_beneficial(plan: LogicalPlan) -> Result<LogicalPlan> {
+ if let LogicalPlan::Join(join) = &plan {
+ // Can be converted back to cross join
+ if join.on.is_empty() && join.filter.is_none() {
+ return LogicalPlanBuilder::from(join.left.as_ref().clone())
+ .cross_join(join.right.as_ref().clone())?
+ .build();
+ }
+ }
+ Ok(plan)
+}
Review Comment:
This is how `push_down_all_join` handles remaining predicates in Filter.
```rust
if keep_predicates.is_empty() {
Ok(plan)
} else {
// wrap the join on the filter whose predicates must be kept
match conjunction(keep_predicates) {
Some(predicate) => Ok(LogicalPlan::Filter(Filter::try_new(
predicate,
Arc::new(plan),
)?)),
None => Ok(plan),
}
}
```
And after it, `convert_to_cross_join_if_beneficial` is called. Where does the Filter push down? Am I missing it?
```
let plan = push_down_all_join(
predicates,
vec![],
&join_plan,
left,
right,
vec![],
true,
)?;
convert_to_cross_join_if_beneficial(plan)?
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "mustafasrepo (via GitHub)" <gi...@apache.org>.
mustafasrepo commented on PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#issuecomment-1868982627
@viirya I have opened a PR to fix remaining points [PR](https://github.com/apache/arrow-datafusion/pull/8648).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Filter pushdown into cross join [arrow-datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #8626:
URL: https://github.com/apache/arrow-datafusion/pull/8626#discussion_r1435677232
##########
datafusion/optimizer/src/eliminate_cross_join.rs:
##########
@@ -47,81 +48,93 @@ impl EliminateCrossJoin {
/// For above queries, the join predicate is available in filters and they are moved to
/// join nodes appropriately
/// This fix helps to improve the performance of TPCH Q19. issue#78
-///
impl OptimizerRule for EliminateCrossJoin {
fn try_optimize(
&self,
plan: &LogicalPlan,
config: &dyn OptimizerConfig,
) -> Result<Option<LogicalPlan>> {
- match plan {
+ let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
+ let mut all_inputs: Vec<LogicalPlan> = vec![];
+ let parent_predicate = match plan {
LogicalPlan::Filter(filter) => {
- let input = filter.input.as_ref().clone();
-
- let mut possible_join_keys: Vec<(Expr, Expr)> = vec![];
- let mut all_inputs: Vec<LogicalPlan> = vec![];
- let did_flat_successfully = match &input {
+ let input = filter.input.as_ref();
+ match input {
LogicalPlan::Join(Join {
join_type: JoinType::Inner,
..
})
- | LogicalPlan::CrossJoin(_) => try_flatten_join_inputs(
- &input,
- &mut possible_join_keys,
- &mut all_inputs,
- )?,
+ | LogicalPlan::CrossJoin(_) => {
+ if !try_flatten_join_inputs(
+ input,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
+ return Ok(None);
+ }
+ extract_possible_join_keys(
+ &filter.predicate,
+ &mut possible_join_keys,
+ )?;
+ Some(&filter.predicate)
+ }
_ => {
return utils::optimize_children(self, plan, config);
}
- };
-
- if !did_flat_successfully {
+ }
+ }
+ LogicalPlan::Join(Join {
+ join_type: JoinType::Inner,
+ ..
+ }) => {
+ if !try_flatten_join_inputs(
+ plan,
+ &mut possible_join_keys,
+ &mut all_inputs,
+ )? {
return Ok(None);
}
+ None
+ }
Review Comment:
This looks like a missing case of reordering joins to eliminate CrossJoin, do we have a test case can cover this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org