-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Internal error with repartitioning after equivalence consolidation #8043
Comments
@mustafasrepo / @ozankabak do you have any idea when the fix for this might be available? It is blocking updating the use of DataFusion in IOx -- perhaps if you can explain what is wrong, I can work on a patch to fix it |
@mustafasrepo is actively working on it, I think we will have the fix out in a couple days at most |
Thank you so much |
Ok, I spent a while debugging our problem today What is happening is that a This diff fixed my problem: diff --git a/datafusion/physical-plan/src/repartition/mod.rs b/datafusion/physical-plan/src/repartition/mod.rs
index 66f7037e5c..17babcd109 100644
--- a/datafusion/physical-plan/src/repartition/mod.rs
+++ b/datafusion/physical-plan/src/repartition/mod.rs
@@ -644,16 +644,16 @@ impl RepartitionExec {
})
}
- /// Set Order preserving flag
+
+ /// Set Order preserving flag, which controlls if this node is
+ /// `RepartitionExec` or `SortPreservingRepartitionExec`. If the input is
+ /// not ordered, or has only one partiton, remains a `RepartitionExec`.
pub fn with_preserve_order(mut self, preserve_order: bool) -> Self {
- // Set "preserve order" mode only if the input partition count is larger than 1
- // Because in these cases naive `RepartitionExec` cannot maintain ordering. Using
- // `SortPreservingRepartitionExec` is necessity. However, when input partition number
- // is 1, `RepartitionExec` can maintain ordering. In this case, we don't need to use
- // `SortPreservingRepartitionExec` variant to maintain ordering.
- if self.input.output_partitioning().partition_count() > 1 {
- self.preserve_order = preserve_order
- }
+ self.preserve_order = preserve_order &&
+ // If the input isn't ordered, there is no ordering to preserve
+ self.input.output_ordering().is_some() &&
+ // if there is only one input partition, merging is required to maintain order
+ self.input.output_partitioning().partition_count() > 1;
self
} I will prepare a PR with this fix shortly |
Good catch. It is interesting that your test failure surfaced one pre-existing bug and one lacking feature (i.e. supporting ordering of complex expressions which @mustafasrepo is working on). |
Yeah, as I was telling some people internally, since IOx makes heavy use of sort orderings (and subsequently this code) I think we are more heavily impacted by such bugs than others. I feel like we can improve DataFusion for everyone by being the early on testers and then feeding back both bug fixes and rough edges in the APIs (like #8120). 🚀 |
#8127 is ready for review |
While testing #8006 with our internal test suite, one of our tests fail because there are no sort expressions in a sort preserving repartition
The input plan looks like:
But then after EnforceSorting the
SortPreservingMergeExec
seems to have to sort exprs anymore:This then causes a failure during execution / streaming merge
Originally posted by @alamb in #8006 (comment)
The text was updated successfully, but these errors were encountered: