sunchao commented on code in PR #5610:
URL: https://github.com/apache/datafusion-comet/pull/5610#discussion_r3971872627
##########
spark/src/main/scala/org/apache/comet/serde/CometScalaUDF.scala:
##########
@@ -172,6 +172,18 @@ object CometScalaUDF extends
CometExpressionSerde[ScalaUDF] {
// their descendants' names.
expr.setTagValue(CometExplainInfo.DISPATCHED_SELF, ())
withCodegenDispatchExpr(expr, exprName)
+ // The whole subtree under `expr` was bound and closure-serialized into
this one kernel, so
+ // every expression in it ran in the JVM, not just the root. Naming only
the root understates
+ // that: for `hypot(abs(b), c)` the dispatched set would hold `hypot`
alone, and a test could
+ // assert `abs` was native while an `abs` was in fact running inside the
kernel. Attribute
+ // references and literals are the kernel's inputs rather than work it
performed, so they are
+ // left out - which also keeps them from appearing in the coverage stats
as "expressions".
+ target.foreach {
+ case _: AttributeReference | _: Literal =>
+ case node if !(node eq target) =>
+ withCodegenDispatchExpr(expr, CometExplainInfo.exprDisplayName(node))
Review Comment:
### Correctness
[P2] Regenerate Q78's approved plan for the expanded dispatch names
Could you regenerate
`tpcds-plan-stability/approved-plans-v2_7/q78/extended.txt` alongside this
change? Recording descendants here changes Q78's dispatcher list from `round`
to `add, cast, coalesce, divide, knownfloatingpointnormalized,
normalizenanandzero, round`, and its dispatch count from 1 to 7, while the
approved plan still contains the old values. All five Linux exec profiles fail
this exact comparison at `CometPlanStabilitySuite.scala:172`. See the [Spark
3.5
job](https://github.com/apache/datafusion-comet/actions/runs/34372705635/job/102547005711)
and [Spark 4.0
job](https://github.com/apache/datafusion-comet/actions/runs/34372705635/job/102547005707).
I checked that this source and the approved plan are identical between the
reviewed head and CI's merge. Updating the golden through the suite's
regeneration workflow preserves the intended expanded coverage and restores the
plan-stability test.
--
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]
