-
Notifications
You must be signed in to change notification settings - Fork 232
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
Improve warnings about AQE nodes not supported on GPU #647
Conversation
Signed-off-by: Andy Grove <andygrove@nvidia.com>
build |
exec[AdaptiveSparkPlanExec]("Adaptive query", (exec, conf, p, r) => | ||
new SparkPlanMeta[AdaptiveSparkPlanExec](exec, conf, p, r) { | ||
override def tagPlanForGpu(): Unit = | ||
willNotWorkOnGpu("this is an adaptive plan") |
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.
Do we need to be a bit more descriptive here? I can see users wondering why it says, this is an adaptive plan
yet the docs say the plugin support AQE. "If it supports AQE, why isn't it handling adaptive plans? Isn't that the whole point of the plugin supporting AQE?" 😄
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.
Yeah, I agree. This whole approach is still confusing. What I really wanted to do was not have any messages related to the AQE operators. Currently, we either replace an operator or show a warning. I think we need a third option of "this does not need replacing". I'm going to look at this again today and see if this can be implemented without major surgery on GpuOverrides.
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.
I totally agree. There are other places we want this too. Perhaps it is just a list of class names that we should not warn about at all.
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.
I pushed an update to introduce a new DoNotReplaceSparkPlanMeta
rule and logic to suppress warnings about operators that use this rule. I manually tested with TPC-DS with AQE and it looks a lot cleaner now.
Signed-off-by: Andy Grove <andygrove@nvidia.com>
build |
* Improve warnings about AQE nodes not supported on GPU Signed-off-by: Andy Grove <andygrove@nvidia.com> * Introduce new DoNotReplaceSparkPlanMeta rule Signed-off-by: Andy Grove <andygrove@nvidia.com>
* Improve warnings about AQE nodes not supported on GPU Signed-off-by: Andy Grove <andygrove@nvidia.com> * Introduce new DoNotReplaceSparkPlanMeta rule Signed-off-by: Andy Grove <andygrove@nvidia.com>
…#647) Signed-off-by: Peixin Li <pxli@nyu.edu> Signed-off-by: Peixin Li <pxli@nyu.edu>
Signed-off-by: Andy Grove andygrove@nvidia.com
This PR improves UX slightly by adding specific handling in GpuOverrides for AQE operators so that we can provide better information to the user, explaining why these nodes are not swapped out for GPU versions.
For example, before this change, the user might see:
With this change, the user would see:
This is still slightly misleading, because the query stage may already be running on GPU, but at least looks less like an internal error or unsupported feature now.
This closes #607