Add docs to update_coalesce_ctx_children.
#14907
Conversation
…text, selectively indicating when coalesce should be removed
wiedld
force-pushed
the
wiedld/refactor-coalesce-ctx
branch
from
8b1019a to
535e74f
Compare
| // since we are checking distribution requirements after the coalesce occurs | ||
| let parent = &coalesce_context.plan; | ||
|
|
||
| for child_context in coalesce_context.children.iter_mut() { |
There was a problem hiding this comment.
this method used to check is_coalesce_partitions once before iterating through the children -- this PR now checks for each child.
As written this loop seems to update child_context.data multiple times (once in each loop body)
I think it is likely the same given the coalesce partitions has a single child, but I found the previous implementation easier to understand
There was a problem hiding this comment.
See #14907 (comment)
| update_coalesce_ctx_children(&mut requirements); | ||
| let coalesce_can_be_removed = requirements.children.iter().any(|child| child.data); |
There was a problem hiding this comment.
this variable is called coalesce_can_be_removed but the comments above say that .data means:
The data
/// attribute stores whether the plan is aCoalescePartitionsExecor is
/// connected to aCoalescePartitionsExecvia its children.
This seems inconsistent to me
There was a problem hiding this comment.
I switched it to be docs only: #14907 (comment)
Then if I move forward with another refactoring attempt, I'll first make an issue to justify the refactor.
|
If this is ready, I'd also like to take a look |
wiedld
changed the title
update_coalesce_ctx_children.update_coalesce_ctx_children.
|
@berkaysynnada and @alamb -- I just switched it to be only the docs in this PR. That way we have some forward progress for now. And then we can have further discussions/decisions about what to do. |
Which issue does this PR close?
Rationale for this change
The initial PR to fix the above issue, also did a refactor and added docs.
I'm breaking up the initial PR, and this is only
the refactorthe docs portion.What changes are included in this PR?
Refactor and add docs to more tightly defineupdate_coalesce_ctx_children. Afterwards, I could remove a few of the conditionals in the other helper methods, since it's now handled with the proper and full labeling of the coalesce context (of which coalesce to remove).Only docs.
Are these changes tested?
N/A
Are there any user-facing changes?
No