[SPARK-41752][SQL][UI] Group nested executions under the root execution #39268

linhongliu-db · 2022-12-29T00:06:40Z

What changes were proposed in this pull request?

This PR proposes to group all sub-executions together in SQL UI if they belong to the same root execution.

This feature is controlled by conf spark.ui.sql.groupSubExecutionEnabled and the default value is set to true

We can have some follow-up improvements after this PR:

Add links to SQL page and Job page to indicate the root execution ID.
Better handling for the root execution missing case (e.g. eviction due to retaining limit). In this PR, the sub-executions will be displayed ungrouped.

Why are the changes needed?

better user experience.

In PR #39220, the CTAS query will trigger a sub-execution to perform the data insertion. But the current UI will display the two executions separately which may confuse the users.
In addition, this change should also help the structured streaming cases

Does this PR introduce any user-facing change?

Yes, the screenshot of the UI change is shown below
SQL Query:

CREATE TABLE t USING PARQUET AS SELECT 'a' as a, 1 as b

UI before this PR

UI after this PR with sub executions collapsed

UI after this PR with sub execution expanded

How was this patch tested?

UT

linhongliu-db · 2022-12-29T00:48:12Z

cc @cloud-fan @HeartSaVioR

core/src/main/scala/org/apache/spark/internal/config/UI.scala

cloud-fan · 2022-12-29T04:03:07Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala

executionIdToSubExecutions(e.rootExecutionId) += e

cloud-fan · 2022-12-29T04:05:39Z

also cc @ulysses-you

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala

linhongliu-db · 2022-12-30T03:45:29Z

The test failed at python linter. should be caused by some "connect" PRs.

annotations failed mypy checks:
python/pyspark/sql/connect/client.py:25: error: Skipping analyzing "grpc_status": module is installed, but missing library stubs or py.typed marker  [import]
python/pyspark/sql/connect/client.py:25: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports
python/pyspark/sql/connect/client.py:30: error: Skipping analyzing "google.rpc": module is installed, but missing library stubs or py.typed marker  [import]
Found 2 errors in 1 file (checked 381 source files)

cloud-fan · 2022-12-30T04:07:04Z

@zhengruifeng @HyukjinKwon are you aware of anything about this python failure?

zhengruifeng · 2022-12-30T04:34:22Z

@cloud-fan I can not repro this fail in my local env

the latest mypy check in master also succeed. https://github.com/apache/spark/actions/runs/3804744443/jobs/6472202104

cloud-fan · 2022-12-30T04:43:41Z

@linhongliu-db can you rebase your branch and try again?

dongjoon-hyun · 2022-12-31T01:41:10Z

core/src/main/scala/org/apache/spark/internal/config/UI.scala

This PR introduces 4 config namespace groups like the following. Shall we simplify the config namespace?

spark.ui.sql.* spark.ui.sql.group.* spark.ui.sql.group.sub.* spark.ui.sql.group.sub.execution.*

changed to spark.ui.sql.groupSubExecutionEnabled but I'm glad to take any other naming suggestions. :)

core/src/main/scala/org/apache/spark/internal/config/UI.scala

dongjoon-hyun · 2022-12-31T01:44:14Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala

Do we have any other usage for this methods, setRootExecutionId and unsetRootExecutionId? These methods seem to be used once.

yes, it's only used once. I personally think that using a function can better explain the logic since it's not a no-brainer.

dongjoon-hyun · 2022-12-31T01:46:25Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala

Just a question. Do we have a test coverage for only Spark event logs to validate this code path?

we have this one: https://github.com/apache/spark/blob/master/sql/core/src/test/scala/org/apache/spark/sql/execution/ui/AllExecutionsPageSuite.scala#L69

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala

dongjoon-hyun · 2022-12-31T01:52:20Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala

This <tr> doesn't need additional indentation here. Could you align the indentation with the previous <tr> at line 389?

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/AllExecutionsPage.scala

linhongliu-db · 2023-01-04T03:10:53Z

working on the comments

dongjoon-hyun · 2023-01-04T22:25:24Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala

+  /**
+   * Unset the "root" SQL Execution Id once the "root" SQL execution completes.
+   */
+  private def unsetRootExecutionId(sc: SparkContext, executionId: String): Unit = {


This method is also misleading because we set EXECUTION_ROOT_ID_KEY to null only when it's equal to executionId.

after a second thought, this function wrapper is misleading and doesn't make things clear. So I inlined it to the main function. Thanks for the suggestion.

dongjoon-hyun · 2023-01-04T22:33:04Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala

    } finally {
      executionIdToQueryExecution.remove(executionId)
      sc.setLocalProperty(EXECUTION_ID_KEY, oldExecutionId)
+      unsetRootExecutionId(sc, executionId.toString)


If we need to define a new method, shall we define it to accept Long directly?

dongjoon-hyun

Thank you for updates, @linhongliu-db . I have only minor comments.

cc @gengliangwang , too

gengliangwang · 2023-01-05T01:53:27Z

sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala

 case class SparkListenerSQLExecutionStart(
    executionId: Long,
+    // if the execution is a root, then rootExecutionId == executionId
+    rootExecutionId: Long,


We need to refactor the code change in https://github.com/apache/spark/blob/master/core/src/main/protobuf/org/apache/spark/status/protobuf/store_types.proto#L387

gengliangwang · 2023-01-05T01:54:07Z

sql/core/src/main/scala/org/apache/spark/status/protobuf/sql/SQLExecutionUIDataSerializer.scala


    new SQLExecutionUIData(
      executionId = ui.getExecutionId,
+      rootExecutionId = ui.getExecutionId,


This should be ui.getRootExecutionId after updating the protobuf definition.

gengliangwang · 2023-01-05T01:54:57Z

sql/core/src/test/scala/org/apache/spark/status/api/v1/sql/SqlResourceSuite.scala


    new SQLExecutionUIData(
      executionId = 0,
+      rootExecutionId = 0,


For testing purpose, let's use a different value from executionId

dongjoon-hyun · 2023-01-10T00:37:53Z

Could you update your PR, @linhongliu-db ? We have Apache Spark 3.4 Feature Freeze schedule.

Also, cc @xinrong-meng as Apache Spark 3.4. release manager.

linhongliu-db · 2023-01-10T02:29:07Z

@dongjoon-hyun working on it

cloud-fan · 2023-01-10T12:51:40Z

The failed YarnClusterSuite is definitely unrelated. I'm merging it to master, thanks!

dongjoon-hyun · 2023-01-10T17:59:51Z

Thank you, @linhongliu-db and @cloud-fan .

linhongliu-db · 2023-01-10T18:26:04Z

Thank you everyone for reviewing this!

…nUIData ### What changes were proposed in this pull request? The new field `rootExecutionId` of `SQLExecutionUIData` is not correctly serialized/deserialized in #39268. This PR is to fix it. ### Why are the changes needed? Bug fix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #39489 from gengliangwang/SPARK-41752. Authored-by: Gengliang Wang <[email protected]> Signed-off-by: Gengliang Wang <[email protected]>

… execution ### What changes were proposed in this pull request? #39268 / [SPARK-41752](https://issues.apache.org/jira/browse/SPARK-41752) added a new non-optional `rootExecutionId: Long` field to the SparkListenerSQLExecutionStart case class. When JsonProtocol deserializes this event it uses the "ignore missing properties" Jackson deserialization option, causing the rootExecutionField to be initialized with a default value of 0. The value 0 is a legitimate execution ID, so in the deserialized event we have no ability to distinguish between the absence of a value and a case where all queries have the first query as the root. Thanks JoshRosen for reporting and investigating this issue. ### Why are the changes needed? Bug fix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #40403 from linhongliu-db/fix-nested-execution. Authored-by: Linhong Liu <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

… execution ### What changes were proposed in this pull request? #39268 / [SPARK-41752](https://issues.apache.org/jira/browse/SPARK-41752) added a new non-optional `rootExecutionId: Long` field to the SparkListenerSQLExecutionStart case class. When JsonProtocol deserializes this event it uses the "ignore missing properties" Jackson deserialization option, causing the rootExecutionField to be initialized with a default value of 0. The value 0 is a legitimate execution ID, so in the deserialized event we have no ability to distinguish between the absence of a value and a case where all queries have the first query as the root. Thanks JoshRosen for reporting and investigating this issue. ### Why are the changes needed? Bug fix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #40403 from linhongliu-db/fix-nested-execution. Authored-by: Linhong Liu <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 4db8e7b) Signed-off-by: Dongjoon Hyun <[email protected]>

… execution ### What changes were proposed in this pull request? apache/spark#39268 / [SPARK-41752](https://issues.apache.org/jira/browse/SPARK-41752) added a new non-optional `rootExecutionId: Long` field to the SparkListenerSQLExecutionStart case class. When JsonProtocol deserializes this event it uses the "ignore missing properties" Jackson deserialization option, causing the rootExecutionField to be initialized with a default value of 0. The value 0 is a legitimate execution ID, so in the deserialized event we have no ability to distinguish between the absence of a value and a case where all queries have the first query as the root. Thanks JoshRosen for reporting and investigating this issue. ### Why are the changes needed? Bug fix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes #40403 from linhongliu-db/fix-nested-execution. Authored-by: Linhong Liu <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]>

… execution ### What changes were proposed in this pull request? apache#39268 / [SPARK-41752](https://issues.apache.org/jira/browse/SPARK-41752) added a new non-optional `rootExecutionId: Long` field to the SparkListenerSQLExecutionStart case class. When JsonProtocol deserializes this event it uses the "ignore missing properties" Jackson deserialization option, causing the rootExecutionField to be initialized with a default value of 0. The value 0 is a legitimate execution ID, so in the deserialized event we have no ability to distinguish between the absence of a value and a case where all queries have the first query as the root. Thanks JoshRosen for reporting and investigating this issue. ### Why are the changes needed? Bug fix ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? UT Closes apache#40403 from linhongliu-db/fix-nested-execution. Authored-by: Linhong Liu <[email protected]> Signed-off-by: Dongjoon Hyun <[email protected]> (cherry picked from commit 4db8e7b) Signed-off-by: Dongjoon Hyun <[email protected]>

wangyum · 2023-07-17T06:13:25Z

@linhongliu-db It seems this patch makes CTAS missing the child info on UI: https://issues.apache.org/jira/browse/SPARK-44213

github-actions bot added CORE SQL WEB UI labels Dec 29, 2022

cloud-fan reviewed Dec 29, 2022

View reviewed changes

core/src/main/scala/org/apache/spark/internal/config/UI.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Dec 29, 2022

View reviewed changes

cloud-fan approved these changes Dec 29, 2022

View reviewed changes

ulysses-you reviewed Dec 29, 2022

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala Outdated Show resolved Hide resolved

dongjoon-hyun requested changes Dec 31, 2022

View reviewed changes

linhongliu-db added 5 commits January 3, 2023 19:11

init

07aa2e2

fix test

bc39922

address comments

79ec051

switch flag

c62c0a1

code style and address comments

4d3c15a

linhongliu-db force-pushed the SPARK-41752 branch from c4b1583 to 4d3c15a Compare January 4, 2023 05:13

linhongliu-db requested review from dongjoon-hyun and ulysses-you and removed request for dongjoon-hyun and ulysses-you January 4, 2023 05:18

dongjoon-hyun reviewed Jan 4, 2023

View reviewed changes

dongjoon-hyun approved these changes Jan 4, 2023

View reviewed changes

gengliangwang reviewed Jan 5, 2023

View reviewed changes

Merge remote-tracking branch 'apache/master' into SPARK-41752

12e322c

linhongliu-db added 3 commits January 9, 2023 21:59

address comments

2fc7eba

Merge remote-tracking branch 'apache/master' into SPARK-41752

e2a0e81

address comments

ee2c4f5

cloud-fan closed this in c124037 Jan 10, 2023

linhongliu-db deleted the SPARK-41752 branch January 10, 2023 18:26

gengliangwang mentioned this pull request Jan 10, 2023

[SPARK-41752][SQL][FOLLOW-UP] Fix Protobuf serializer for SQLExecutionUIData #39489

Closed

linhongliu-db mentioned this pull request Mar 13, 2023

[SPARK-42754][SQL][UI] Fix backward compatibility issue in nested SQL execution #40403

Closed

[SPARK-41752][SQL][UI] Group nested executions under the root execution #39268

[SPARK-41752][SQL][UI] Group nested executions under the root execution #39268

Uh oh!

Conversation

linhongliu-db commented Dec 29, 2022 • edited by cloud-fan Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

linhongliu-db commented Dec 29, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Dec 29, 2022

Uh oh!

Uh oh!

linhongliu-db commented Dec 30, 2022

Uh oh!

cloud-fan commented Dec 30, 2022

Uh oh!

zhengruifeng commented Dec 30, 2022

Uh oh!

cloud-fan commented Dec 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

linhongliu-db commented Jan 4, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dongjoon-hyun commented Jan 10, 2023

Uh oh!

linhongliu-db commented Jan 10, 2023

Uh oh!

cloud-fan commented Jan 10, 2023

Uh oh!

dongjoon-hyun commented Jan 10, 2023

Uh oh!

linhongliu-db commented Dec 29, 2022 •

edited by cloud-fan

Loading