-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: integrate batch coalescer with async fn exec #19342
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: integrate batch coalescer with async fn exec #19342
Conversation
54fe5f4 to
76f6837
Compare
76f6837 to
f0e4be2
Compare
|
Hey @jizezhang 👋🏻 I think we can review it now :) |
alamb
left a comment
There was a problem hiding this 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
alamb
left a comment
There was a problem hiding this 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.
|
BTW I think we'll need one more PR to remove the rule from |
jizezhang
left a comment
There was a problem hiding this 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.
|
Thank you for patiently reviewing and helping me! 🙇🏻
I will raise a PR for both of them after this gets merged. And also another PR to implement metrics in |
Which issue does this PR close?
BatchCoalescerintoAsyncFuncExecand remove fromCoalesceBatchesoptimization rule #19331What 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