-
Notifications
You must be signed in to change notification settings - Fork 37
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
Mark wholestageCodeGen as shouldRemove when child nodes are removed #1142
Conversation
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]> Fixes NVIDIA#860 This commit adjusts the accuracy of the Qual tool by targeting the following issues: - child nodes of wholeStageCodeGen would not be assigned to stages if they have no metrics. - there is a corner case when all the childs of wholeStageCodeGen are marked as shouldRemove. In that case, the node would still be considered unsupported and contribute to the speedup. The changes are: - propagate the stageIDs of wholeStageCodeGen to the child nodes - a wholeStageCodeGen node is marked as shouldRemove when all the childs are marked as shouldRemove. - fix unit-test which has 4 different wholeStageCodeGen nodes that contain only `ColumnarToRow` execs
// average speedup across the execs in the WholeStageCodegen for now | ||
val supportedChildren = childNodes.filterNot(_.shouldRemove) | ||
val avSpeedupFactor = SQLPlanParser.averageSpeedup(supportedChildren.map(_.speedupFactor)) | ||
// TODO: revisit the logic behind adding the stages of child nodes to the current node. |
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.
The below allStagesIncludingChildren
is there since the early days. I don't quite understand the reason childNodes stages are appended to the wholeStageCodeGen stageIDs.
@tgravescs do you know why we this was needed? If not, I suggest we remove it.
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.
looking at the comment its saying that the child node might be associated to a stage but the whole stage codegen doesn't match it. So we basically add all the child node ones in. I don't remember when that can happen but would not remove it unless we are positive its not needed.
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.
so I don't think this comment was addressed before merged? unless you know this doesn't happen I would prefer to see this comment removed or to file an issue to investigate it. This to me could just lead to confusion from someone reading the code.
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.
My bad! haven't got my cup of coffee. For some reason I misread that I got 2 approvals.
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.
Posted a followup #1146
The bug of not assigning stages to execs inside wholeStageCodeGen also exists in the Profiler. We can either fix it in this PR or file a followup. |
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.
Thanks @amahussein ! The logic LGTM for marking wholestageCodeGen as shouldRemove.
core/src/main/scala/com/nvidia/spark/rapids/tool/planparser/WholeStageExecParser.scala
Show resolved
Hide resolved
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]> Follow-up on NVIDIA#1142 to remove TODO comment line in WholeStageExecParser
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]> Follow-up on #1142 to remove TODO comment line in WholeStageExecParser
Signed-off-by: Ahmed Hussein (amahussein) [email protected]
Fixes #860 , Fixes #793
This commit adjusts the accuracy of the Qual tool by targeting the following issues:
The changes are:
ColumnarToRow
execs