jordepic commented on code in PR #5361:
URL: https://github.com/apache/datafusion-comet/pull/5361#discussion_r3789634425
##########
spark/src/main/scala/org/apache/spark/sql/comet/CometMetricNode.scala:
##########
@@ -354,10 +354,21 @@ object CometMetricNode {
/**
* Creates a [[CometMetricNode]] from a [[CometPlan]].
+ *
+ * Stops walking at non-Comet nodes: a JVM-side `AQEShuffleReadExec` (or any
other Spark exec
+ * constructed off the planning thread) captures `SparkPlan.session` eagerly
as a `@transient
+ * final val`, which can be `null` if `SparkSession.getActiveSession`
returned `None` at the
+ * moment AQE's stage-finalisation rules built it. Forcing such a node's
`metrics` lazy val NPEs
+ * in `SQLMetrics.createMetric(sparkContext, ...)`. We don't own those
metrics anyway -- the
+ * native side only sources updates against operators it actually planned,
and JVM-side
+ * AQE-stage nodes belong to a different stage that's already been
materialised independently.
*/
- def fromCometPlan(cometPlan: SparkPlan): CometMetricNode = {
- val children = cometPlan.children.map(fromCometPlan)
- CometMetricNode(cometPlan.metrics, children)
+ def fromCometPlan(cometPlan: SparkPlan): CometMetricNode = cometPlan match {
Review Comment:
Correction to the above after the first full CI run: pre-checking `session
!= null` was wrong. `fromCometPlan` also runs inside task closures on
executors, where the `@transient session` is always null after deserialisation
— but `metrics` is safe there because the lazy val was materialised on the
driver and shipped with the plan. The pre-check therefore blanked every native
operator's metrics on executors, which `CometTaskMetricsSuite`'s "native
parquet write reports task-level output metrics" caught on all five [exec]
shards.
Now the guard wraps the `metrics` access itself and only swallows the NPE
from forcing an unmaterialised lazy val with a null session (the original AQE
stage-finalisation case). Every reachable metrics map — driver or executor,
Comet node or not — reports exactly as upstream did; `CometTaskMetricsSuite`
passes locally again.
Separately, the macos-14 [scans] job died in a hotspot-level crash ("error
occurred during error reporting, SIGTRAP") right after an unrelated timestamp
suite — that one just needs a re-run.
--
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]
