Change the explanation of why the operator will not work on GPU #4328
Conversation
Signed-off-by: remzi <[email protected]>
|
build |
| @@ -124,7 +124,7 @@ the driver logs with `spark.rapids.sql.explain=all`. | |||
| this version: | |||
|
|
|||
| ``` | |||
| !NOT_FOUND <RowDataSourceScanExec> cannot run on GPU because no GPU enabled version of operator class org.apache.spark.sql.execution.RowDataSourceScanExec could be found | |||
| !Unsupported operator: <RowDataSourceScanExec> cannot run on GPU because no GPU enabled version of operator class org.apache.spark.sql.execution.RowDataSourceScanExec could be found | |||
There was a problem hiding this comment.
This still says "no GPU enabled version of operator class .... could be found". The original problem that triggered #4088 was that the "class not found" message led the user to believe this was a classloader/classpath issue rather than a normal situation where we know this Catalyst operation is not supported.
|
build |
HaoYang670
changed the title
Signed-off-by: remzi <[email protected]>
|
build |
| @@ -1307,7 +1307,7 @@ final class RuleNotFoundExprMeta[INPUT <: Expression]( | |||
| extends ExprMeta[INPUT](expr, conf, parent, new NoRuleDataFromReplacementRule) { | |||
|
|
|||
| override def tagExprForGpu(): Unit = | |||
| willNotWorkOnGpu(s"no GPU enabled version of expression ${expr.getClass} could be found") | |||
| willNotWorkOnGpu(s"GPU does not currently support the operator ${expr.getClass}") | |||
There was a problem hiding this comment.
I think "currently" is appropriate, as without it the user may incorrectly believe the operation is impossible to support.
There was a problem hiding this comment.
Ah, sorry, I didn't see the context of the other changes. I agree that we should be consistent here. If we're not using "currently" in the other instances when we state something is unsupported then we shouldn't state it only here.
There was a problem hiding this comment.
Sorry, I made some inconsistent changes. I will add "currently" to all changes.
| @@ -1307,7 +1307,7 @@ final class RuleNotFoundExprMeta[INPUT <: Expression]( | |||
| extends ExprMeta[INPUT](expr, conf, parent, new NoRuleDataFromReplacementRule) { | |||
|
|
|||
| override def tagExprForGpu(): Unit = | |||
| willNotWorkOnGpu(s"no GPU enabled version of expression ${expr.getClass} could be found") | |||
| willNotWorkOnGpu(s"GPU does not currently support the operator ${expr.getClass}") | |||
There was a problem hiding this comment.
Ah, sorry, I didn't see the context of the other changes. I agree that we should be consistent here. If we're not using "currently" in the other instances when we state something is unsupported then we shouldn't state it only here.
| @@ -46,7 +46,7 @@ trait DataFromReplacementRule { | |||
| * A version of DataFromReplacementRule that is used when no replacement rule can be found. | |||
| */ | |||
| final class NoRuleDataFromReplacementRule extends DataFromReplacementRule { | |||
| override val operationName: String = "NOT_FOUND" | |||
| override val operationName: String = "Not supported by GPU: " | |||
There was a problem hiding this comment.
This is redundant, as it later explains on the same line that the operation is not supported. We don't put "Not supported by GPU: " in front of other lines that will not end up on the GPU, so it's weird to call out this specially. I think this should just be an empty string.
There was a problem hiding this comment.
Thank you for your advice. I will change it.
…ght be supported by GPU in the future Signed-off-by: remzi <[email protected]>
|
build |
1 similar comment
|
build |
|
re-triggered due to weird #4316 |
sameerz
added
the
task
Signed-off-by: HaoYang670 [email protected]
close #4088