You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "sandugood (via GitHub)" <gi...@apache.org> on 2026/05/18 12:23:34 UTC
[PR] feat(AQE): added empty join handling [datafusion-ballista]
sandugood opened a new pull request, #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723
# Which issue does this PR close?
Handles #1359, particularly this part:
verify [if we can reuse instead]
> (https://github.com/spiceai/spiceai/blob/348b7cc7eaac7eaebbce277e8569d8119989a921/crates/datafusion-optimizer-rules/src/physical_plan/hash_join_optimization.rs#L39) of creating our own rule for eliminating joins
# Rationale for this change
Handling cases of different physical types of joins and adding branching for different logical parts (i.e Left or LeftAnti) without adding separate AQE rule - adding logic straight into the `PropagateEmptyExecRule`
# What changes are included in this PR?
1. Added `JoinInfo` struct to contain information about the join (both left and right part)
2. Added `as_join` function, which can be extended in the future to add support for other physical joins. Right now it handles both SortMerge and HashJoin
3. Reused `is_guaranteed_empty` from the Spice-AI implementation of eliminating empty joins.
# Are there any user-facing changes?
No
# Were this changes tested?
Yes, tests were included in both `propagate_empty.rs` and `join_info.rs`
--
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] feat(AQE): empty join handling [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#discussion_r3311467819
##########
ballista/scheduler/src/state/aqe/optimizer_rule/join_info.rs:
##########
@@ -0,0 +1,226 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
can we move this helpers to `propagate_empty.rs` ?
--
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] feat(AQE): empty join handling [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#issuecomment-4558834491
#1752 has few other rules added, could you please copy paste them here. Will merge this pr before that one and makes sense to include them
--
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] feat(aqe): empty join handling [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on code in PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#discussion_r3343859083
##########
ballista/scheduler/src/state/aqe/optimizer_rule/propagate_empty.rs:
##########
@@ -85,12 +98,47 @@ impl PropagateEmptyExecRule {
&& is_empty_exec!(aggregation.input())
{
empty_exec!(aggregation)
- } else if let Some(hash_join) = plan.as_any().downcast_ref::<HashJoinExec>()
- // TODO: - we need other joins, this one is used for testing cancellation
- && hash_join.join_type == Inner
- && (is_empty_exec!(hash_join.left) || is_empty_exec!(hash_join.right))
+ } else if let Some(repartition) = plan.as_any().downcast_ref::<RepartitionExec>()
+ && is_empty_exec!(repartition.input())
+ {
+ empty_exec!(repartition)
+ } else if let Some(coalesce_partition) =
+ plan.as_any().downcast_ref::<CoalescePartitionsExec>()
+ && is_empty_exec!(coalesce_partition.input())
{
- empty_exec!(hash_join)
+ empty_exec!(coalesce_partition)
+ } else if let Some(sort_preserving_merge) =
+ plan.as_any().downcast_ref::<SortPreservingMergeExec>()
+ && is_empty_exec!(sort_preserving_merge.input())
+ {
+ empty_exec!(sort_preserving_merge)
+ } else if let Some(join) = as_join(&plan) {
+ let left_empty = is_guaranteed_empty(join.left);
+ let right_empty = is_guaranteed_empty(join.right);
+
+ // Checking whether join would produce an empty result
+ let should_eliminate = match join.join_type {
+ JoinType::Inner => left_empty || right_empty,
+ JoinType::Full => left_empty && right_empty,
+ JoinType::Left
+ | JoinType::RightSemi
+ | JoinType::LeftAnti
+ | JoinType::LeftMark => left_empty,
+ JoinType::Right
+ | JoinType::LeftSemi
+ | JoinType::RightAnti
+ | JoinType::RightMark => right_empty,
Review Comment:
Added, as well as left-semi and full
--
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] feat(AQE): empty join handling [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on code in PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#discussion_r3313809234
##########
ballista/scheduler/src/state/aqe/optimizer_rule/join_info.rs:
##########
@@ -0,0 +1,226 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
Yeah, I think it makes sense, since it is basically related to the propagate_empty rule
--
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] feat(AQE): empty join handling [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#issuecomment-4559040720
Seems like there is an error when trying to get packages from crates.io
Could you, please, rerun the CI in a while?
--
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] feat(aqe): empty join handling [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on code in PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#discussion_r3341771648
##########
ballista/scheduler/src/state/aqe/optimizer_rule/propagate_empty.rs:
##########
@@ -85,12 +98,47 @@ impl PropagateEmptyExecRule {
&& is_empty_exec!(aggregation.input())
{
empty_exec!(aggregation)
- } else if let Some(hash_join) = plan.as_any().downcast_ref::<HashJoinExec>()
- // TODO: - we need other joins, this one is used for testing cancellation
- && hash_join.join_type == Inner
- && (is_empty_exec!(hash_join.left) || is_empty_exec!(hash_join.right))
+ } else if let Some(repartition) = plan.as_any().downcast_ref::<RepartitionExec>()
+ && is_empty_exec!(repartition.input())
+ {
+ empty_exec!(repartition)
+ } else if let Some(coalesce_partition) =
+ plan.as_any().downcast_ref::<CoalescePartitionsExec>()
+ && is_empty_exec!(coalesce_partition.input())
{
- empty_exec!(hash_join)
+ empty_exec!(coalesce_partition)
+ } else if let Some(sort_preserving_merge) =
+ plan.as_any().downcast_ref::<SortPreservingMergeExec>()
+ && is_empty_exec!(sort_preserving_merge.input())
+ {
+ empty_exec!(sort_preserving_merge)
+ } else if let Some(join) = as_join(&plan) {
+ let left_empty = is_guaranteed_empty(join.left);
+ let right_empty = is_guaranteed_empty(join.right);
+
+ // Checking whether join would produce an empty result
+ let should_eliminate = match join.join_type {
+ JoinType::Inner => left_empty || right_empty,
+ JoinType::Full => left_empty && right_empty,
+ JoinType::Left
+ | JoinType::RightSemi
+ | JoinType::LeftAnti
+ | JoinType::LeftMark => left_empty,
Review Comment:
should we add left semi?
https://github.com/apache/datafusion/blob/7843ab385bdc90db02a80ccfd410eb74c1aeb0fb/datafusion/optimizer/src/propagate_empty_relation.rs#L143-L153
##########
ballista/scheduler/src/state/aqe/optimizer_rule/propagate_empty.rs:
##########
@@ -85,12 +98,47 @@ impl PropagateEmptyExecRule {
&& is_empty_exec!(aggregation.input())
{
empty_exec!(aggregation)
- } else if let Some(hash_join) = plan.as_any().downcast_ref::<HashJoinExec>()
- // TODO: - we need other joins, this one is used for testing cancellation
- && hash_join.join_type == Inner
- && (is_empty_exec!(hash_join.left) || is_empty_exec!(hash_join.right))
+ } else if let Some(repartition) = plan.as_any().downcast_ref::<RepartitionExec>()
+ && is_empty_exec!(repartition.input())
+ {
+ empty_exec!(repartition)
+ } else if let Some(coalesce_partition) =
+ plan.as_any().downcast_ref::<CoalescePartitionsExec>()
+ && is_empty_exec!(coalesce_partition.input())
{
- empty_exec!(hash_join)
+ empty_exec!(coalesce_partition)
+ } else if let Some(sort_preserving_merge) =
+ plan.as_any().downcast_ref::<SortPreservingMergeExec>()
+ && is_empty_exec!(sort_preserving_merge.input())
+ {
+ empty_exec!(sort_preserving_merge)
+ } else if let Some(join) = as_join(&plan) {
+ let left_empty = is_guaranteed_empty(join.left);
+ let right_empty = is_guaranteed_empty(join.right);
+
+ // Checking whether join would produce an empty result
+ let should_eliminate = match join.join_type {
+ JoinType::Inner => left_empty || right_empty,
+ JoinType::Full => left_empty && right_empty,
+ JoinType::Left
+ | JoinType::RightSemi
+ | JoinType::LeftAnti
+ | JoinType::LeftMark => left_empty,
+ JoinType::Right
+ | JoinType::LeftSemi
+ | JoinType::RightAnti
+ | JoinType::RightMark => right_empty,
Review Comment:
should we add right semi?
https://github.com/apache/datafusion/blob/7843ab385bdc90db02a80ccfd410eb74c1aeb0fb/datafusion/optimizer/src/propagate_empty_relation.rs#L143-L153
--
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] feat(aqe): empty join handling [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm merged PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723
--
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] feat(AQE): empty join handling [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#issuecomment-4496459987
thanks @sandugood will have a look soon
--
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] feat(aqe): empty join handling [datafusion-ballista]
Posted by "metegenez (via GitHub)" <gi...@apache.org>.
metegenez commented on PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#issuecomment-4621489728
> Addressed the comments, added test-cases as a sanity-check The last test case simulates the mentioned flaky test fail in #1752, but it would be nice if you could rerun the same logic against this PR @metegenez Thank you in advance and thanks @milenkovicm for the review
I will check it soon, a bit busy these days. Thanks for the contribution.
--
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] feat(AQE): empty join handling [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#issuecomment-4597010497
No need to hurry, take your time and thanks for the review
Going to check those comments on #1752 regarding the emptyexec and see what can be (or need to be) done
--
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] feat(AQE): empty join handling [datafusion-ballista]
Posted by "milenkovicm (via GitHub)" <gi...@apache.org>.
milenkovicm commented on PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#issuecomment-4596973911
@sandugood my apologies I got stuck with #1752 and needed a bit of a break. I hope to get back and review this tomorrow. Also please check that pr as there are some comments regarding empty exec and I'm not sure if this pr covers them
--
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] feat(aqe): empty join handling [datafusion-ballista]
Posted by "sandugood (via GitHub)" <gi...@apache.org>.
sandugood commented on PR #1723:
URL: https://github.com/apache/datafusion-ballista/pull/1723#issuecomment-4606306227
Addressed the comments, added test-cases as a sanity-check
The last test case simulates the mentioned flaky test fail in #1752, but it would be nice if you could rerun the same logic against this PR @metegenez
Thank you in advance and thanks @milenkovicm for the review
--
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