[SPARK-38617][SQL][WEBUI] Show Spark rule and phase timings in SQL UI and REST API

[yliou]

What changes were proposed in this pull request?

The changes proposed are to add a clickable field on SQL UI to show timing of spark phases and rule timing information from QueryPlanningTracker along with a separate REST endpoint to show the timing statistics. Adds a SQLConf to control the number of rules that
show up on the UI and the REST endpoint call (default value is 15).
UI Change:
New REST API endpoint output:
SPARK-38617RestOutput.txt

Why are the changes needed?

Currently user is unable to see Spark phase timing information or which Spark rules take the most time for a specific query execution on the UI. This PR adds that information.

Does this PR introduce any user-facing change?

Yes on the UI

How was this patch tested?

Tested locally and added a class to test the new endpoint.

[martin-g]

SPARK38617.txt is in rich text format. It would be easier to review if it was plain text.

[martin-g]

IMO it would be better to use a new method instead of overwriting #toString().
toString might be called implicitly somewhere and cause performance issue. I have seen this in the wild few times.

[yliou]

Renamed method to compileStatsString

[martin-g]

In what cases this exception might be thrown ? I guess when #read() cannot find it.
It is weird that it does not return an Option or Either.
Also planGraph() and executionsList() above do not try to recover.
def execution(executionId: Long) does the same as here.
IMO this class needs some love.

[yliou]

I added this try catch statement as a precaution after adding the if statement on SQLAppStatusListener.scala:393. Some unit tests were failing without the if statement. I can remove the try catch statement if desired.

[martin-g]

should offset and length be validated to be positive ?

[yliou]

It shouldn't be needed. I added SqlCompilerResource.scala based on SqlResource.scala

[martin-g]

the filtering filters nothing with the provided two persons. Is this intentional ?

[yliou]

Yes

[ulysses-you]

#35856 has tried the similar things although at present it miss the details of the compile time for each phase. And its scope also includes the AQE, I think is good to go.

[yliou]

SPARK38617.txt is in rich text format. It would be easier to review if it was plain text.

Attached new file with plain text format.

[yliou]

#35856 has tried the similar things although at present it miss the details of the compile time for each phase. And its scope also includes the AQE, I think is good to go.

What does good to go mean here as I don't see an approval at #35856? From what I see the changes at #35856 are different and complimentary with this PR as that PR adds information regarding AQE plan description changes and this PR has timing of spark phases and rule timing information. The changes in both PRs will help for troubleshooting.

[sigmod]

cc @jchen5

[ulysses-you]

@yliou I agree both can help for troubleshooting. And I have mentioned the difference, it miss the details of the compile time for each phase. That PR introduces a disk-based store so I'm thinking if can re-use the same code path. IMO it would be better to go with that PR first at least.

[mridulm]

+CC @shardulm94, @thejdeep

[sigmod]

Can we use a more meaningful JSON field names? E.g.,
value -> timeMs
name -> phase

{
   “phase” : “optimization”,
   “timeMs” : “373"
 }, {
   “phase” : “analysis”,
   “timeMs” : “12"
 } ],
 “ruleInfo” : [ {
   “ruleName” : “org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation”,
   “timeMs” : “32.659566”,
   “numInvocations” : 5,
   “numEffectiveInvocations” : 3
 }, 
 ....
]

Also, can timeMs be a number instead of a string?

[yliou]

@ulysses-you got it. I think it is possible to re-use the same code path but not sure how that'd work with ExecutionPage.scala to get the info on the UI.

[yliou]

Can we use a more meaningful JSON field names? E.g., value -> timeMs name -> phase

{
   “phase” : “optimization”,
   “timeMs” : “373"
 }, {
   “phase” : “analysis”,
   “timeMs” : “12"
 } ],
 “ruleInfo” : [ {
   “ruleName” : “org.apache.spark.sql.catalyst.optimizer.ConvertToLocalRelation”,
   “timeMs” : “32.659566”,
   “numInvocations” : 5,
   “numEffectiveInvocations” : 3
 }, 
 ....
]

Also, can timeMs be a number instead of a string?

Updated Json field names and changed timeMs to be a number

[linhongliu-db]

@ulysses-you @yliou adding debug information is always good. But I'm worried about two issues:

1. we should be careful when adding information to live UI's in-memory KV store. sooner or later, it will bring headaches
2. we should be careful when adding new REST API endpoints. since two PRs both for troubleshooting, maybe we can use the same endpoint.

[jchen5]

The class name mismatches: SqlCompilerResource vs SqlCompileResourceSuite (the r in compiler, typo?)

[yliou]

Thanks for catching the typo! I renamed the file.

[yliou]

Updated the PR to be more concise regarding the names of rules so that Spark event logs aren't as impacted as before in terms of size.

[github-actions]

We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

[yliou]

@dependabot reopen

[yliou]

Hi @ulysses-you @linhongliu-db now that #35856 got reverted and #38567 got merged, should I create another pull request for this feature to try to get it merged?