feat: optimize CoalesceBatches in limit

[acking-you]

Which issue does this PR close?

Closes #11980.

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

[Dandandan]

I think we should turn this around: allow any approved plan nodes instead of disallowing some.
Otherwise this will be wrong for any added/forgotten nodes or user defined nodes.

[acking-you]

Ok, I get it. I'll give it a try.

[acking-you]

I think we should turn this around: allow any approved plan nodes instead of disallowing some. Otherwise this will be wrong for any added/forgotten nodes or user defined nodes.

After carefully considering the revised plan, I realized that identifying operators requiring a full table scan is still necessary regardless of the changes.
Because this optimization is always determined by whether or not there is a full table scan operator and whether or not it contains a limit operator.
Here are the new changes: https://github.com/acking-you/arrow-datafusion/blob/feat/optimize_coalesce_batches/datafusion/core/src/physical_optimizer/coalesce_batches.rs#L101-L134

[berkaysynnada]

The issue explained in #9792 was resolved with the implementation of #11652. This fix handles the problem related to waiting for the coalescer buffer to fill when a Limit -> ... -> CoalesceBatches pattern exists. The approach was to push down the limit (fetch + skip) into CoalesceBatches and eliminate the limit when it was no longer needed.

With #12003, it appears that additional corner cases are being addressed. It further refines the process by pushing limits as far down the execution plan as possible and removing any redundant limits.

It seems that these recent improvements already address the objective you're aiming for, without the need to define a constant thresholds. I think there is no difference between using a limit without coalescing and using a coalesce that can internally handle limits.

I am curious about your thoughts. Do you still see a need for additional optimization? If so, could you provide an example scenario or a test case that would help us discuss this further?

[acking-you]

The issue explained in #9792 was resolved with the implementation of #11652. This fix handles the problem related to waiting for the coalescer buffer to fill when a Limit -> ... -> CoalesceBatches pattern exists. The approach was to push down the limit (fetch + skip) into CoalesceBatches and eliminate the limit when it was no longer needed.

With #12003, it appears that additional corner cases are being addressed. It further refines the process by pushing limits as far down the execution plan as possible and removing any redundant limits.

It seems that these recent improvements already address the objective you're aiming for, without the need to define a constant thresholds. I think there is no difference between using a limit without coalescing and using a coalesce that can internally handle limits.

I am curious about your thoughts. Do you still see a need for additional optimization? If so, could you provide an example scenario or a test case that would help us discuss this further?

Thanks for providing the background on this optimization. I looked into the issues you mentioned and it seems they've been resolved exactly as I hoped. Great job! I'll reference the information you compiled in my issue.