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.
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.
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.
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.
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