Minor: remove confusing update_plan_from_children call from EnforceSorting

[xudong963]

Which issue does this PR close?

• Closes #.

Rationale for this change

update_sort_ctx_children only updates the data field of the context nodes, which tracks whether nodes are connected to a SortExec. update_plan_from_children() is about updating the actual execution plan structure.
The data field updates in update_sort_ctx_children don't actually require any changes to the underlying execution plan.
Therefore, the call to update_plan_from_children() in update_sort_ctx_children appears unnecessary.

So remove it to avoid confusion.

What changes are included in this PR?

remove unnecessary update_plan_from_children call

Are these changes tested?

Covered by existing tests

Are there any user-facing changes?

[xudong963]

This will be the first PR that I'm improving the EnforceSorting, I'll try to make the improvement as small as possible to reduce the review burden and avoid regression.

[alamb]

Thank you for starting to work on this code @xudong963

BTW I believe @wiedld is also plans to work to make this code better, see

• #14637

Maybe you can work together to improve things

[wiedld]

Thank you for starting to work on this code @xudong963

BTW I believe @wiedld is also plans to work to make this code better, see

Maybe you can work together to improve things

Happy to work together @xudong963 .

Right now I'm working on:

• put in a fix for this bug (and let reviewers correct the approach 😆 ).
• I'm in the process of refactoring the EnforceDistribution tests since:
  • those tests are still tightly coupled with EnforceSorting from when they were a single optimization.
  • we have already demonstrated that this optimizer pass is not idempotent.
  • make it a bit easier to understand and iterate upon

I can make sure to add you as a reviewers on those. Please lmk if/how you want to coordinate.

[wiedld]

Also, it looks like the failing CI tests are related to the sort enforcement. Such as not removing unnecessary sorts in some of the children.

[xudong963]

Also, it looks like the failing CI tests are related to the sort enforcement. Such as not removing unnecessary sorts in some of the children.

The problem is that the code still depends on the update_plan_from_children in update_sort_ctx_children, even if children aren't changed in update_sort_ctx_children. -- This is making things weird.

I did more updates:

1. Rename the update_plan_from_children to update_sort_ctx_children_data to show what the method does to update data.
2. Call update_plan_from_children after the children of PlanContext is changed.
3. After creating the PlanContext, if you can't guarantee they(PlanContext's plan's children's Execution and PlanContext's children's Execution) are consistent, call update_plan_from_children to sync.

[xudong963]

I can make sure to add you as a reviewers on those. Please lmk if/how you want to coordinate.

Sure, please add me.

[alamb]

@wiedld can you please review this PR?

Stale review

[wiedld]

LGTM

I also made a draft PR with added docs, which references exactly why this PR is ok. I'll update conflicts & merge that in after this PR.

Note that we could have also change the meaning of the function to update_ctx_and_sync_children. Either way works.

[alamb]

Thank you @xudong963 and @wiedld -- it is really neat to see some others start working on this code. I expect it will get even better than it already is!

[alamb]

I found the methods names hard to follow -- for example update_children_data actually has a OrderPreservationContext 🤔

[xudong963]

yes, I changed to update_order_preservation_ctx_children_data, longer but clearer

[alamb]

I wonder if we could find some way to avoid having to remember to do this 🤔

If we have to remember to call a function it seems likely either our future selves or others will forget to do so

[alamb]

Maybe one way would be to make these fields not pub and then control access

so like add a method so the only way to update children woudl be

impl PlanContext {
  fn update_children(&mut self, new_children: Vec<Self>) -> {
    self.children = new_children;
    self.update_child_data()
}

🤔

To be clear I am talking about some future PR

[xudong963]

Yes, I've tried this way to make the operation "atomic", but I found it hard to change, most places acquire mut self for PlanContext.

I have another way to avoid the mind burden. Currrently, we have two parallel tree structures:

• The ExecutionPlan tree (through plan field and its children)
• The PlanContext tree (through children field)

I'm trying to make them single.

[alamb]

FYI @ozankabak and @berkaysynnada

[xudong963]

I plan to merge the PR, and then @wiedld can continue to his PR.

I also made a draft PR with added docs, which references exactly why this PR is ok. I'll update conflicts & merge that in after this PR.