-
Notifications
You must be signed in to change notification settings - Fork 40
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
Fix missing exec-to-stageId mapping in Qual tool #1437
Conversation
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]> Fixes NVIDIA#1156 This adds logic to walk the SparkGraph in order to assign execs to stages. For nodes that have no AccumIDs, the clusterization processes relies on adjacent nodes.
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
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 ! I tested your PR on different eventlogs and the assignment of stageId's to execs is more accurate and I see lesser execs with no stageId's assigned to them. This will be useful as we have more execs assigned to stages.
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Show resolved
Hide resolved
Signed-off-by: Ahmed Hussein (amahussein) <[email protected]>
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 @nartal1 !
I have addressed your comments.
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
core/src/main/scala/org/apache/spark/sql/rapids/tool/util/ToolsPlanGraph.scala
Outdated
Show resolved
Hide resolved
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.
LGTM. Thanks @amahussein !
Fixes #1156
This adds logic to walk the SparkGraph in order to assign execs to
stages. For nodes that have no AccumIDs, the clusterization processes
relies on adjacent nodes.
Some followups and improvements can be tracked in #1434
Refactoring and Improvements:
This pull request includes several changes to the
com.nvidia.spark.rapids.tool
package, focusing on improving the SQL plan analysis and stage mapping functionality. The changes involve refactoring methods, adding new interfaces, and updating tests to ensure comprehensive coverage.AppSQLPlanAnalyzer.scala
: Removed theSQLPlanParser.getStagesInSQLNode
method and replaced it withapp.getStageIDsFromAccumIds
to map stages to operators more efficiently.SQLPlanParser.scala
: Refactored theparseSQLPlan
method to useToolsPlanGraph.createGraphWithStageClusters
and updated related methods to utilize the newnodeIdToStagesFunc
parameter for improved stage mapping. [1] [2] [3] [4]WholeStageExecParser.scala
: Updated constructors to include thenodeIdToStagesFunc
parameter and refactored methods to use this function for stage mapping. [1] [2] [3]PhotonStageExecParser.scala
: Modified the constructor to include thenodeIdToStagesFunc
parameter for consistent stage mapping.New Interface:
AccumToStageRetriever.scala
: Introduced a new traitAccumToStageRetriever
to define the interface for retrieving stage IDs from accumulables, allowing for better separation of logic and easier testing.Test Updates:
BasePlanParserSuite.scala
andSQLPlanParserSuite.scala
: Added new methods to verify that all execution nodes are assigned to stages and updated existing tests to ensure comprehensive validation of the new stage mapping logic. [1] [2] [3] [4]These changes collectively enhance the accuracy and maintainability of the SQL plan analysis and stage mapping within the
com.nvidia.spark.rapids.tool
package.