You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "alihan-synnada (via GitHub)" <gi...@apache.org> on 2024/09/17 14:35:28 UTC
[PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
alihan-synnada opened a new pull request, #12504:
URL: https://github.com/apache/datafusion/pull/12504
## Which issue does this PR close?
Closes #11332.
## Rationale for this change
See the linked issue.
## What changes are included in this PR?
`NestedLoopJoinExec` now preserves the order of the right table for `Inner`, `Right`, `RightAnti` and `RightSemi` join types.
## Are these changes tested?
Tested with `join_maintains_right_order` and relevant SQL logic tests
## Are there any user-facing changes?
This PR changes the output ordering of some queries with `JOIN`, as reflected in SQL logic tests. The output values and the API remain unchanged.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #12504:
URL: https://github.com/apache/datafusion/pull/12504#discussion_r1763546352
##########
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##########
@@ -238,6 +238,19 @@ impl NestedLoopJoinExec {
PlanProperties::new(eq_properties, output_partitioning, mode)
}
+
+ fn maintains_input_order(join_type: JoinType) -> Vec<bool> {
Review Comment:
I feel it would be great to have some doc on what this method does and return. Specifically what false/true means, what is the benefit of preserving the order
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "korowa (via GitHub)" <gi...@apache.org>.
korowa commented on PR #12504:
URL: https://github.com/apache/datafusion/pull/12504#issuecomment-2359064908
This PR seems to introduce a performance regression due to the similar reasons as it was in https://github.com/apache/datafusion/pull/9830#issuecomment-2028430188 -- now `apply_batch_filter` may be executed way more times. Query used for the comparison:
```
EXPLAIN ANALYZE SELECT count(1) FROM nation n JOIN lineitem li ON n.n_nationkey < li.l_orderkey
```
and for the single partition execution I've got the following results:
```
-- Before:
NestedLoopJoinExec: join_type=Inner, filter=n_nationkey@0 < l_orderkey@1, metrics=[output_rows=150029850, build_input_rows=25, output_batches=733, build_input_batches=1, input_batches=733, input_rows=6001215, build_mem_used=296, join_time=1.257203344s, build_time=541.719µs]
-- After:
NestedLoopJoinExec: join_type=Inner, filter=n_nationkey@0 < l_orderkey@1, metrics=[output_rows=150029850, build_input_batches=1, input_batches=733, output_batches=733, input_rows=6001215, build_input_rows=25, build_mem_used=296, build_time=488.521µs, join_time=9.563931093s]
```
(`join_time` metric shows the issue).
Any thoughts on 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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada merged PR #12504:
URL: https://github.com/apache/datafusion/pull/12504
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "korowa (via GitHub)" <gi...@apache.org>.
korowa commented on PR #12504:
URL: https://github.com/apache/datafusion/pull/12504#issuecomment-2359236994
If this PR enforces ordering by the price of slowdown for all types of inputs (the example shows fairly small build side input, but it also may be enough), it, perhaps, should be reverted and reimplemented -- I suppose, there exist more compromise solutions for this issue, since, if the root cause of slowdown demonstrated in the example, is increased number calls to `apply_join_filter_to_indices`, the problem, perhaps, could be solved by something like #8952 (which could accumulate indices up to batch size and only then apply filters).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on PR #12504:
URL: https://github.com/apache/datafusion/pull/12504#issuecomment-2360137695
Thanks for reporting this @korowa. I believe it’s possible to maintain the order without affecting performance. We will work on fixing this shortly.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #12504:
URL: https://github.com/apache/datafusion/pull/12504#issuecomment-2359152482
Filed https://github.com/apache/datafusion/issues/12528
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #12504:
URL: https://github.com/apache/datafusion/pull/12504#issuecomment-2359150939
Thanks @korowa for checking this, the join time is 9x now @berkaysynnada @alihan-synnada
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Preserve the order of right table in NestedLoopJoinExec [datafusion]
Posted by "alihan-synnada (via GitHub)" <gi...@apache.org>.
alihan-synnada commented on code in PR #12504:
URL: https://github.com/apache/datafusion/pull/12504#discussion_r1764602039
##########
datafusion/physical-plan/src/joins/nested_loop_join.rs:
##########
@@ -238,6 +238,19 @@ impl NestedLoopJoinExec {
PlanProperties::new(eq_properties, output_partitioning, mode)
}
+
+ fn maintains_input_order(join_type: JoinType) -> Vec<bool> {
Review Comment:
Addressed in 09a6ceee435e1237a361b87d059b34f4fac03385
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org