Implement fast path of with_new_children() in ExecutionPlan

[mingmwang]

Which issue does this PR close?

Closes #1965.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

[mingmwang]

@yjshen @alamb

Please help to take a look.

[alamb]

I think this looks great @mingmwang -- thank you very much.

Another thing we might want to consider doing would be to change the signature of ExecutionPlan trait to require an Arc to call with_new_children, like so:

    fn with_new_children(
        self: Arc<Self>,
        children: Vec<Arc<dyn ExecutionPlan>>,
    ) -> Result<Arc<dyn ExecutionPlan>> {
  // put with_new_children_if_necessary implementation here
...
}

That would ensure that we could always apply the with_new_children_if_necessary logic

[alamb]

What would you think about adding an assert here such asassert_eq!(children.len(), 1) on the theory it might catch bugs faster when making changes?

The same question applies to other with_new_children implementations below

[mingmwang]

I removed the length check in the with_new_children() method and move the check to with_new_children_if_necessary() so that we can avoid the duplicate logic in all the trait implementations. In future if someone misuse this and still call the with_new_children() and pass the wrong children param, it will lose the check, but if this is the case, the real problem is he should not call with_new_children() and everyone should call with_new_children_if_necessary().

[mingmwang]

I'm not sure in Rust whether I can declare a method in the Trait to protected so that it can only be visible in this module and in the Trait implementations.

[alamb]

👍

[mingmwang]

ExecutionPlan

I think this looks great @mingmwang -- thank you very much.

Another thing we might want to consider doing would be to change the signature of ExecutionPlan trait to require an Arc to call with_new_children, like so:

    fn with_new_children(
        self: Arc<Self>,
        children: Vec<Arc<dyn ExecutionPlan>>,
    ) -> Result<Arc<dyn ExecutionPlan>> {
  // put with_new_children_if_necessary implementation here
...
}

That would ensure that we could always apply the with_new_children_if_necessary logic

Done.

[yjshen]

LGTM overall. Minor comments on the doc.

[yjshen]

Suggested change

	/// Returns a new plan where all children were replaced by new plans if the provided children
	/// do not share the same point references with the existing children.
	/// Returns a copy of this plan if we change any child according to pointer comparison.
	///

[mingmwang]

Done.

[yjshen]

This should be false-positive I think?

[mingmwang]

I think it should be 'false-native'(two expected equal things end up not comparing equal) for Trait objects.

https://stackoverflow.com/questions/67109860/how-to-compare-trait-objects-within-an-arc

[yjshen]

Since we are reporting two equal items to be non-equal, it results in making a copy, so I term it 'false-positive'.

It should be 'false negative' if you only mean data equality. 😆

[alamb]

I think this is a step in the right direction -- thank you @mingmwang