andygrove commented on code in PR #4891:
URL: https://github.com/apache/datafusion-comet/pull/4891#discussion_r3572561263
##########
spark/src/main/scala/org/apache/comet/serde/CometScalaUDF.scala:
##########
@@ -70,11 +72,12 @@ object CometScalaUDF extends CometExpressionSerde[ScalaUDF]
{
expr: Expression,
inputs: Seq[Attribute],
binding: Boolean): Option[Expr] = {
+ val exprName = expr.prettyName
Review Comment:
For a Scala/Java `ScalaUDF`, `expr.prettyName` resolves to `"scalaudf"`
because Spark only overrides `name` on `ScalaUDF`, not `prettyName` (see
`Expression.prettyName`, which is `nodeName.toLowerCase(Locale.ROOT)`). So
every user UDF gets the same prefix, and because `EXTENSION_INFO` and fallback
reasons are stored as `Set[String]`, a projection with two different UDFs will
collapse into one entry. That undercuts the goal described in the PR body of
keeping distinct expressions distinct. Would it be worth special-casing
`ScalaUDF` here to use something like `s.udfName.getOrElse(s.prettyName)`? The
`CometCodegenDispatch` subclasses (`Hypot`, `Levenshtein`, etc.) are fine
because they're distinct expression classes with distinct `nodeName`s. Only the
raw `ScalaUDF` path has this problem.
##########
spark/src/test/scala/org/apache/comet/CometCodegenSuite.scala:
##########
@@ -131,6 +131,42 @@ class CometCodegenSuite
}
}
+ test("explainCodegen.enabled surfaces routed expressions in COMET-INFO") {
Review Comment:
One of the reasons cited for making this configurable was to avoid the
annotation bleeding into golden files. Would it be worth adding a companion
assertion in this test that flips `COMET_EXPLAIN_CODEGEN_ENABLED` to `false`
and confirms the `hypot`/`levenshtein` info lines do NOT appear? That would pin
down the default-off guarantee.
Separately, the four new fallback-reason prefixes in
`emitJvmCodegenDispatch` (`<exprName>: <reason>`) change user-visible explain
and log output but don't seem to have any test asserting the new format. An
easy one: run `SELECT hypot(a, b)` with `COMET_SCALA_UDF_CODEGEN_ENABLED=false`
and assert the fallback reasons on the projection contain the `hypot: ` prefix.
##########
spark/src/main/scala/org/apache/comet/serde/CometScalaUDF.scala:
##########
@@ -125,6 +142,17 @@ object CometScalaUDF extends
CometExpressionSerde[ScalaUDF] {
udfBuilder
.setReturnType(returnTypeProto)
.setReturnNullable(expr.nullable)
+ // Surface the dispatch route in extended explain output when the user
opts in via
+ // `spark.comet.explainCodegen.enabled=true`. `rollUpInfoMessages` in
`CometExecRule` walks
+ // `op.expressions` via `TreeNode.collect` and copies each expression's
`EXTENSION_INFO` tag
+ // onto the resulting Comet plan node, where `ExtendedExplainInfo` renders
it as
+ // `[COMET-INFO: ...]`. Informational only: `withInfo` does not trigger
fallback, so the
+ // projection remains a Comet operator regardless of this setting.
+ if (CometConf.COMET_EXPLAIN_CODEGEN_ENABLED.get()) {
+ withInfo(
+ expr,
+ s"${exprName.toLowerCase(Locale.ROOT)}: routes through JVM codegen
dispatcher")
Review Comment:
`prettyName` already returns a lowercase string
(`nodeName.toLowerCase(Locale.ROOT)`) unless a `FUNC_ALIAS` overrides it. Is
the extra `toLowerCase` here intentional to handle the alias case? If so, the
fallback-reason calls above use raw `exprName` and would be inconsistent. Might
be simpler to drop the lowercase here (and the `java.util.Locale` import) so
the format matches the fallback paths.
##########
docs/source/user-guide/latest/understanding-comet-plans.md:
##########
@@ -108,6 +109,44 @@ when you want to see all reasons, including ones that
not include the surrounding plan, so it is best for accumulating diagnostics
across many queries.
+### `spark.comet.explainCodegen.enabled`
+
+Disabled by default. When enabled, every Spark expression that Comet routes
+through the JVM codegen dispatcher (Spark's own `doGenCode` running inside a
+Comet kernel — the path used for expressions with no native DataFusion
+implementation, gated by `spark.comet.exec.scalaUDF.codegen.enabled=true`) is
+annotated with `<expr_name>: routes through JVM codegen dispatcher`. The
+annotation rolls up onto the surrounding `CometProject` / `CometFilter` /
+etc. and is rendered inside the `[COMET-INFO: ...]` segment of extended
+explain output (verbose format). The projection stays Comet-native — this is
+informational only, useful for distinguishing "runs natively in DataFusion"
+from "runs Spark's generated code inside a Comet kernel".
+
+Example:
+
+```scala
+spark.conf.set("spark.comet.exec.scalaUDF.codegen.enabled", "true")
+spark.conf.set("spark.comet.explainCodegen.enabled", "true")
+
+val df = spark.sql("SELECT hypot(a, b), levenshtein(s1, s2) FROM t")
+println(new org.apache.comet.ExtendedExplainInfo()
+ .generateExtendedInfo(df.queryExecution.executedPlan))
+```
+
+Output:
+
+```
+CometNativeColumnarToRow
++- CometProject [COMET-INFO: hypot: routes through JVM codegen dispatcher,
levenshtein: routes through JVM codegen dispatcher]
+ +- CometNativeScan parquet spark_catalog.default.t
+
+Comet accelerated 2 out of 2 eligible operators (100%). Final plan contains 1
transitions between Spark and Comet.
+```
+
+Note that the operator is still `CometProject` (Comet-accelerated); only the
Review Comment:
The prose here already explains the dispatcher path well. One small
suggestion: could you add a sentence explicitly noting the annotation only
appears for expressions that lack a native DataFusion implementation? A reader
could infer this from the surrounding paragraph, but making it explicit avoids
anyone treating the tag as a general "this expression ran here" marker.
--
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]
