-
Notifications
You must be signed in to change notification settings - Fork 242
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
Add in docs to describe window performance #3022
Conversation
Signed-off-by: Robert (Bobby) Evans <[email protected]>
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.
Docs look good to me.
I really would be interested to hear from others if we think we should include some configs to allow users to selectively disable window operations.
It might be handy in practice to have window configs to control the various window types. That way if a user discovers a performance problem or bug in windowing then they can easily work around that case without forcing all window operations off the GPU. If it's easy I say let's do it, but if it's quite a bit of work then maybe we wait until we more clearly see the need. It seems to come down to how likely do we think a query will have multiple types of windows where disabling one window type but leaving another enabled is better than blanket disabling GpuWindowExec
.
build |
@jlowe I am trying to figure out what the configs should look like. I can make them specific to performance optimizations and small. Like
But if I try to make something more generic I would end up with a lot of configs (18 if I can count)
And then the performance problems/bugs are likely to be specific to different aggregations, so it might end up being 18-ish configs per aggregation (SUM, MIN, MAX, COUNT, ROW_NUMBER, LEAD, LAG, ...) I am not sure that this is what we want either. |
build |
Yeah so then I'm thinking maybe we hold off a bit until we get clearer guidance from real-world use as to what we really need. |
Had some offline review comments that I tried to clarify. |
build |
I filed #3023 to try and track some possible performance optimizations. |
Right now this is a document only change, but I really would be interested to hear from others if we think we should include some configs to allow users to selectively disable window operations. The problem is that the slowness is based off of the window size, and this is data dependent, as you can see from #3021. That means we cannot detect ahead of time if we are going to see slowness. I will file a follow on issue/epic for optimizations we know we can do.