feat: report native vs codegen-dispatch expression coverage in extended explain - #5201
feat: report native vs codegen-dispatch expression coverage in extended explain#5201andygrove wants to merge 3 commits into
Conversation
…ed explain The extended explain summary reported operator coverage and transition count but said nothing about expressions, so there was no way to see how much of a plan's expression evaluation ran in native DataFusion kernels versus Spark's own generated code inside the codegen dispatcher. Track both paths on the expression tree with a new `NATIVE_EXPRS` tag, the counterpart of the existing `CODEGEN_DISPATCH_EXPRS`, and roll them up per operator onto the converted Comet plan node alongside the info messages. The summary line now ends with, for example: Comet accelerated 14 expressions (14 native, 1 codegen dispatch). Counts are distinct expression names per plan. A name can appear in both buckets when the same function is lowered natively for one set of arguments and dispatched for another, so the total is the union rather than the sum. Structural nodes (attribute references, literals, aliases, bound references) are excluded; they appear in nearly every tree and would swamp the names that matter, and excluding literals also keeps the dispatcher's closure-serialized payload out of the native count. Add `ExtendedExplainInfo.getNativeExpressions` and `getCodegenDispatchExpressions` for programmatic access to the two name lists. Dispatch tagging is no longer gated on `spark.comet.explain.codegen.enabled`; that config now controls only whether the `[COMET-INFO: JVM codegen dispatcher: ...]` segment is rendered, so the counts are available by default. Enable that config in `CometPlanStabilitySuite` so the TPC-DS goldens record which expressions took the dispatcher path, not just how many. Golden files regenerated for Spark 3.4, 3.5, 4.0, 4.1 and 4.2; the only changes are summary lines and `COMET-INFO` segments.
comphead
left a comment
comphead
left a comment
There was a problem hiding this comment.
Thanks @andygrove would it also address issue I came across recently here https://github.com/apache/datafusion-comet/pull/4870/changes#diff-9d04b74555b859929052456a8fd83506fd2154fa2a5dbf246ec4f1a04b807b11R40
So plan looks fully native however coverage is 97% and 1 transition
Quality pass over the previous commit, no behavior change:
- Collapse the three byte-identical "append a name to a Set-valued tag" bodies
(`withInfo`, `withCodegenDispatchExpr`, `withNativeExpr`) onto one private
`appendTagValue`, with a bulk `appendTagValues` for lifting a whole name set.
- Add `CometExplainInfo.collectTagValues` and use it from both roll-up sites in
`CometExecRule` (including the pre-existing `EXTENSION_INFO` line) and from
`liftCoverageTags`, replacing three spellings of the same union.
- `rollUpInfoMessages` now writes the coverage tags through `appendTagValues`
rather than raw `setTagValue`, so there is one way to write these tags.
- `liftCoverageTags` walks the promoted tree once instead of twice, and drops
the identity `collect { case e: Expression => e }`.
- Classify native-vs-dispatched from a dedicated `DISPATCHED_SELF` marker
instead of sniffing `CODEGEN_DISPATCH_EXPRS`. That tag accumulates descendant
names during roll-up and lifting, so reading it as "was I dispatched" made the
classification depend on planner visit order.
- Route the two accessors through the existing `sortup` traversal, as
`fallbackReasons` already does, so they no longer render and discard a full
plan string just to read tags off nodes.
- Drop two no-op configs from the new test, fix an inaccurate comment about the
dispatcher's closure payload (that `Literal` is built locally and is never
reachable from `op.expressions`), and trim a comment that restated its code.
The root of that plan has a c2r, so that explains the transition and one of the reported fallbacks. There is also a |
The summary read Comet accelerated 13 expressions (13 native, 1 codegen dispatch). The total was the union of the two name sets rather than their sum, because the same function can be lowered natively for one set of arguments and dispatched for another, so a name can be in both. Correct by design, but it presents as an arithmetic error to anyone reading the line. It now reads Accelerated expressions: 13 native, 1 codegen dispatch. which reports the same two counts, has no total to misread, and is grammatical at any count. Also drops the merged-set allocation the total needed. Golden files regenerated for Spark 3.4, 3.5, 4.0, 4.1 and 4.2; the only changed lines are the summary.
For this specific file https://github.com/comphead/arrow-datafusion-comet/blob/d781362f4ce284d5e87997be385fd68d31860b30/spark/src/test/resources/tpcds-plan-stability/approved-plans-v1_4-spark3_5/q67/extended.txt there is no |
yes, there is bug with counting comet reused subquery. I filed an issue #5203 |
2 participants
Which issue does this PR close?
N/A
Rationale for this change
The extended explain summary reports operator coverage and the number of
Spark/Comet transitions, but says nothing about expressions. There was no way to
see how much of a plan's expression evaluation ran in native DataFusion kernels
versus Spark's own generated code inside the JVM codegen dispatcher.
What changes are included in this PR?
Expression coverage is tracked on the expression tree with a new
NATIVE_EXPRStag, the counterpart of the existing
CODEGEN_DISPATCH_EXPRS, and both arerolled up per operator onto the converted Comet plan node alongside the info
messages. The summary line now ends with the counts:
Two new accessors on
ExtendedExplainInfogive the same information as data:Notes on the counting:
buckets when the same function is lowered natively for one set of arguments and
dispatched for another, so the total is the union rather than the sum.
castinthe TPC-DS goldens is a real example.
excluded. They appear in nearly every expression tree and would swamp the names
that matter, and excluding literals also keeps the dispatcher's
closure-serialized payload out of the native count.
prettyNamelowercased (the UDF name for aScalaUDF), matching the existing[COMET-INFO: JVM codegen dispatcher: ...]naming.
Dispatch tagging is no longer gated on
spark.comet.explain.codegen.enabled.That config now controls only whether the
[COMET-INFO: ...]segment isrendered, so the counts are available by default. The config is now enabled in
CometPlanStabilitySuiteso the goldens record which expressions took thedispatcher path, not just how many — across TPC-DS that is
castandupper.How are these changes tested?
Two new tests in
CometCodegenSuitecover the split counts and both accessors(including that they are independent of
explain.codegen.enabled), and theall-fallback case.
TPC-DS plan stability goldens were regenerated for Spark 3.4, 3.5, 4.0, 4.1 and
4.2. All 158 changed
extended.txtfiles differ only in summary lines andCOMET-INFOsegments — no plan shapes changed. The suites were re-run againstthe regenerated goldens for 3.4, 3.5, 4.0 and the default profile.