-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Improve EnforceSorting docs. #14673
Improve EnforceSorting docs. #14673
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @wiedld -- this looks great
cc @xudong963
/// A node context object beneficial for writing optimizer rules. | ||
/// This context encapsulating an [`ExecutionPlan`] node with a payload. | ||
/// | ||
/// Since each wrapped node has it's children within both the [`PlanContext.plan.children()`], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
this is indeed quite subtle -- it would be awesome if we could find some way to make it harder to forget to call update_plan_from_children
/// whether we elect to transform [`CoalescePartitionsExec`] + [`SortExec`] cascades | ||
/// into [`SortExec`] + [`SortPreservingMergeExec`] cascades, which enables us to | ||
/// perform sorting in parallel. | ||
/// Performs optimizations based upon a series of subrules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
/// sort first on a per-partition basis, thereby parallelizing the sort. | ||
/// | ||
/// | ||
/// The outcome is that plans of the form |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A great reading experience, thanks @wiedld
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @wiedld and @xudong963
fn update_sort_ctx_children_data( | ||
mut node: PlanWithCorrespondingSort, | ||
mut node_and_ctx: PlanWithCorrespondingSort, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Which issue does this PR close?
Rationale for this change
update_plan_from_children
call fromEnforceSorting
#14650, that some of the EnforceSorting docs could be improved.What changes are included in this PR?
Only docs. No code.
Are these changes tested?
N/A
Are there any user-facing changes?
N/A