Add operators to ignore list and update WindowExpr parser #890
Conversation
Signed-off-by: Niranjan Artal <[email protected]>
…-rapids-tools-833
core/src/main/scala/com/nvidia/spark/rapids/tool/planparser/SQLPlanParser.scala
Outdated
Show resolved
Hide resolved
| val windowFunc = windowFunctionPattern.findAllIn(expr).toList | ||
| // Check to ensure windowFunc is not empty before accessing last element | ||
| if (windowFunc.nonEmpty) { | ||
| val expr = windowFunc.last |
There was a problem hiding this comment.
This variable name shadows a variable name in the outer scope "expr"
core/src/main/scala/org/apache/spark/sql/rapids/tool/ToolUtils.scala
Outdated
Show resolved
Hide resolved
Signed-off-by: Niranjan Artal <[email protected]>
| case _ => // NO OP | ||
| if (windowExprs.nonEmpty) { | ||
| for ( i <- 0 to windowExprs.size - 1 ) { | ||
| val windowFunc = windowFunctionPattern.findAllIn(windowExprs(i)).toList |
There was a problem hiding this comment.
Can we use windowExprs.forEach for more idiomatic pattern?
Using last() can throw exceptions if windowFunc is empty. Can we use lastOption?
There was a problem hiding this comment.
Thanks @nartal1 !
I am ok with the changes. I don't remember why we would skip the last entry though.
If you have an insight, you can put a code comment so that we understand that in the future.
| @@ -484,7 +488,7 @@ object SQLPlanParser extends Logging { | |||
| case c if c.contains("CreateDataSourceTableAsSelectCommand") => | |||
| // create data source table doesn't show the format so we can't determine | |||
| // if we support it | |||
| ExecInfo(node, sqlID, node.name, expr = "", 1, duration = None, node.id, | |||
| ExecInfo(node, sqlID, normalizedNodeName, expr = "", 1, duration = None, node.id, | |||
There was a problem hiding this comment.
Nit: It is not introduced by this PR, but I do not see we need this case anymore.
It should be handled by the default case, right?
There was a problem hiding this comment.
You are right. This would be handled by default case. Removed this case statement.
core/src/main/scala/org/apache/spark/sql/rapids/tool/ToolUtils.scala
Outdated
Show resolved
Hide resolved
This PR fixes #833 and fixes #834.
For 833, we added operators mentioned in the issue to IgnorePerf list. Also updated the shouldIgnore function to handle classNames correctly.
For 834, fixed the bug in parser for WindowExec for the case where windowExprs is empty. This happens in case where the string for WIndowExec is truncated.