pan3793 commented on PR #58313: URL: https://github.com/apache/spark/pull/58313#issuecomment-5440922468
Thanks for the review. (1), (2) and the test minor are addressed in 5682ba382ba. **(1) + (2)** Extracted `SQLExecution.NONE_EXPLAIN_MODE` / `NO_PLAN_DESCRIPTION` and a `SQLExecution.planDescription(qe, uiExplainMode)` helper, as suggested. Both call sites are one line now, `equalsIgnoreCase` drops the dependency on the conf's `transform`, and the double `conf.uiExplainMode` read in `AdaptiveSparkPlanExec` is gone with it. The helper's scaladoc records why `none` is deliberately not an `ExplainMode`. **(3)** I can't attribute a number to `none` alone -- the customer environment doesn't let me profile. What I can report is the combined effect of this PR and SPARK-59024 (#58314), which is what they run: - end-to-end job time: ~30min -> ~12min - driver memory: previously needed at least 24g, now stable at 5g - SQL UI: the execution page never responded before; it now renders the DAG in ~100s The remaining ~100s is on the `SparkPlanInfo` / plan-graph side, so your reading is right that it still costs. It also isn't something that can be folded into this change: `sparkPlanInfo` is not display-only -- `SQLAppStatusListener.onExecutionStart` / `onAdaptiveExecutionUpdate` build `SparkPlanGraph` from it to register every SQL metric accumulator, so unlike the plan description it has no placeholder form. Reducing it would be a separate design discussion. **Minors** - Tests now reference `SQLExecution.NO_PLAN_DESCRIPTION`. - Skipped the `SQLConf`-level test: `withSQLConf(UI_EXPLAIN_MODE.key -> "none")` in the two updated suites already fails if `checkValue` rejects `none`, and the rejection path was equally untested before this PR. - Agreed on docs, no change needed. Also added a note to the PR description on why `spark.sql.maxPlanStringLength` doesn't cover this: `StringConcat.append` only skips storing text past the limit and `TreeNode.generateTreeString` has no early exit, so it bounds the final string's memory but not the CPU spent building it. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
