[SPARK-57987][SQL] Expose modifiedConfigs and node description in SQL REST API#57066
[SPARK-57987][SQL] Expose modifiedConfigs and node description in SQL REST API#57066ulysses-you wants to merge 3 commits into
Conversation
… REST API Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
|
||
| import org.apache.spark.sql.execution.ui.SparkPlanGraphEdge | ||
|
|
||
| case class Metric private[spark] (name: String, value: String) |
There was a problem hiding this comment.
reorder the class code position to Metric -> Node -> ExecutionData to make compile happy sometimes
There was a problem hiding this comment.
so essential change here are:
- add
desc: String = nulltocase class Node - add
val modifiedConfigs: Map[String, String] = Map.empty)toclass ExecutionData
how about also exposing planId to Node, I found that sometimes the LLM might confuse about nodeId and planId
There was a problem hiding this comment.
We do not have a planId field here. Maybe we can add it in other pr.
|
cc @cloud-fan @yaooqinn @sarutak thank you |
sarutak
left a comment
sarutak
left a comment
There was a problem hiding this comment.
It might be nice to add a test case where modifiedConfigs is empty (Map.empty) and/or desc is null, to verify the JSON serialization behavior for default values. Not a blocker since existing fields (queryId, errorMessage) don't have equivalent tests either.
| "org.apache.spark.ml.recommendation.ALS.getIntermediateStorageLevel") | ||
| "org.apache.spark.ml.recommendation.ALS.getIntermediateStorageLevel"), | ||
| // [SPARK-57987] Add desc field to the SQL REST API Node case class | ||
| ProblemFilters.exclude[Problem]("org.apache.spark.status.api.v1.sql.Node*") |
There was a problem hiding this comment.
nit: Can we narrow this to specific problem types?
This would still allow MiMa to catch unexpected issues (like class removal) while permitting the case class field addition. Not blocking, just a suggestion for future-proofing.
|
thank you @sarutak , addressed comment |
… REST API ### What changes were proposed in this pull request? This PR exposes two additional fields in the SQL REST API (`/api/v1/applications/[app-id]/sql/[execution-id]`): - `modifiedConfigs` on `ExecutionData` -- the SQL configs modified for the execution, sourced from `SQLExecutionUIData.modifiedConfigs`. - `desc` on `Node` -- the physical plan node description, sourced from `SparkPlanGraphNode.desc`. ### Why are the changes needed? The SQL REST API already surfaces plan and metric information, but callers cannot see which configs were modified for a given execution, nor the per-node description string. Exposing these makes the REST API self-sufficient for tooling that inspects executions without scraping the Web UI. ### Does this PR introduce _any_ user-facing change? Yes. Two new fields are added to the SQL REST API response: `modifiedConfigs` (map) on the execution object and `desc` (string) on each plan node. Both default to empty/absent, so existing consumers are unaffected. ### How was this patch tested? Extended `SqlResourceSuite` to populate and assert both fields (node `desc` via the `details = true` cases, and `modifiedConfigs` via `verifyExpectedExecutionData`). All 7 tests in the suite pass. ### Was this patch authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 4.8) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Closes #57066 from ulysses-you/sqlresource. Authored-by: Xiduo You <ulyssesyou18@gmail.com> Signed-off-by: Xiduo You <ulyssesyou@apache.org> (cherry picked from commit f2f6bde) Signed-off-by: Xiduo You <ulyssesyou@apache.org>
|
thank you all for review! |
3 participants
What changes were proposed in this pull request?
This PR exposes two additional fields in the SQL REST API (
/api/v1/applications/[app-id]/sql/[execution-id]):modifiedConfigsonExecutionData-- the SQL configs modified for the execution, sourced fromSQLExecutionUIData.modifiedConfigs.desconNode-- the physical plan node description, sourced fromSparkPlanGraphNode.desc.Why are the changes needed?
The SQL REST API already surfaces plan and metric information, but callers cannot see which configs were modified for a given execution, nor the per-node description string. Exposing these makes the REST API self-sufficient for tooling that inspects executions without scraping the Web UI.
Does this PR introduce any user-facing change?
Yes. Two new fields are added to the SQL REST API response:
modifiedConfigs(map) on the execution object anddesc(string) on each plan node. Both default to empty/absent, so existing consumers are unaffected.How was this patch tested?
Extended
SqlResourceSuiteto populate and assert both fields (nodedescvia thedetails = truecases, andmodifiedConfigsviaverifyExpectedExecutionData). All 7 tests in the suite pass.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Claude Opus 4.8)
🤖 Generated with Claude Code