Skip to content
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

Validating assumption on children partition count #56

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

edmondop
Copy link
Contributor

In the query stage, we take the output partition count of the first child if the plan has children, assuming all the plans have the same partition count.

While this is effectively true, if a bug is introduced in planning, this assumption will not hold anymore, and the program might start behaving in surprising ways that are difficult to troubleshoot. Rather than taking the value from the first child, the last child, or a random child, ensuring that if there are children they all have the same output partition count is a more robust approach

@edmondop
Copy link
Contributor Author

I think this should be merged before #54 @andygrove

self.plan.children()[0]
.output_partitioning()
.partition_count()
let mut output_partition_counts = HashSet::new();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do this check without creating additional data structures:

    let partition_count = self.plan.children()[0].output_partitioning().partition_count();
    for i in 1..self.plan.children().len() {
        if self.plan.children()[i].output_partitioning().partition_count() != partition_count {
            return Err(...)
        }
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, do you think we should r change the signature to return an error ? That was my original idea, I think there is a 'plan_err!' macro in datafusion we can use

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #54 no longer looks at the plan's children in this section of code, so perhaps this check (which is a good check) should move somewhere else. I'd suggest adding it in QueryStage::new and have it return Result<Self>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, so let's postpone this as an improvement on #54

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I converted it into Draft in the meantime

@edmondop edmondop marked this pull request as draft December 15, 2024 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants