Skip to content

Conversation

@feniljain
Copy link
Contributor

Which issue does this PR close?

What changes are included in this PR?

Currently haven't included changes to remove it from optimizer, will do it in a subsequent PR as I see everyone else has done the same?

Mostly took code from other PRs and just stitched it together to work with AsyncFnExec

Are these changes tested?

In Progress

Are there any user-facing changes?

No

@github-actions github-actions bot added optimizer Optimizer rules physical-plan Changes to the physical-plan crate labels Dec 15, 2025
@feniljain feniljain force-pushed the feat-batch-coalesce-async-fn-exec branch from 76f6837 to f0e4be2 Compare December 19, 2025 08:32
@github-actions github-actions bot removed the optimizer Optimizer rules label Dec 19, 2025
@feniljain feniljain marked this pull request as ready for review December 19, 2025 10:11
@feniljain
Copy link
Contributor Author

Hey @jizezhang 👋🏻

I think we can review it now :)

@feniljain feniljain changed the title wip: integrate batch coalescer with async fn exec feat: integrate batch coalescer with async fn exec Dec 19, 2025
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @feniljain and @jizezhang -- I think this PR needs tests but otherwise could be merged in. I also have some ideas of how to simplify it a bit

@feniljain feniljain requested review from alamb and jizezhang December 30, 2025 12:16
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @feniljain - this looks good to me

I am also interested in @jizezhang's comments too.

@alamb
Copy link
Contributor

alamb commented Dec 30, 2025

BTW I think we'll need one more PR to remove the rule from CoalesceBatches (and maybe we can even remove the entire rule now...)

Copy link
Contributor

@jizezhang jizezhang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me too, thank you.

@feniljain
Copy link
Contributor Author

feniljain commented Dec 30, 2025

Thank you for patiently reviewing and helping me! 🙇🏻

BTW I think we'll need one more PR to remove the rule from CoalesceBatches (and maybe we can even remove the entire rule now...)

I will raise a PR for both of them after this gets merged. And also another PR to implement metrics in AsyncFuncExec :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-plan Changes to the physical-plan crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrate BatchCoalescer into AsyncFuncExec and remove from CoalesceBatches optimization rule

3 participants