andygrove opened a new pull request, #5593: URL: https://github.com/apache/datafusion-comet/pull/5593
## Which issue does this PR close? Related to #5574. That issue is about the dispatcher being unreachable from `convert`. This PR does not fix that — it adds the diagnostic that makes the affected cases visible, so the cleanup in #5574 can be scoped from evidence rather than from a static sweep. ## Rationale for this change A serde whose `getSupportLevel` returns `Compatible` is promising that `convert` will succeed. When `convert` returns `None` instead, two things happen and neither is obvious: - the enclosing operator falls back to Spark, and - the JVM codegen dispatcher never gets a chance at the expression, because `dispatchIfFallback` is only reached from the `Unsupported` and `Incompatible` arms of `exprToProtoInternal` (`QueryPlanSerde.scala:941` and `:968`). So a check that lives in `convert` instead of `getSupportLevel` silently costs the user a whole-operator fallback that the dispatcher could have absorbed. Today nothing surfaces that. ## What changes are included in this PR? A warning on the `Compatible` arm when `convert` returns `None`, plus a new `CometSerdeInvariantSuite`. The one piece of actual logic is the suppression rule. `convert` recurses into children, so a `Compatible` serde legitimately returns `None` when a *child* could not be serialized — `getSupportLevel` inspects the node, not the subtree. Without a guard, a single unsupported leaf would warn once for every ancestor on the way up. The warning therefore stays quiet when any strict descendant already carries a `FALLBACK_REASONS` tag, which is how a failed child announces itself. Note this deliberately checks *descendants only*: several serdes (`CometCreateArray`, `CometCreateNamedStruct`, `CometArraysZip`) also tag themselves when a child fails, so keying off the node's own tag would defeat the check. Two categories warn without being a serde bug, and both are documented in the scaladoc: - `getSupportLevel` is not given `inputs`, so an `inputs`-dependent decline cannot be moved there. `CometAttributeReference` is the only one today. - With `spark.comet.exec.scalaUDF.codegen.enabled=false`, every serde that reports `Compatible` and then routes to `emitJvmCodegenDispatch` warns, because the disabled-config check lives in the dispatcher rather than in `getSupportLevel`. Those are genuine instances of the invariant violation; moving that check up belongs with #5574, not here. ## How are these changes tested? New `CometSerdeInvariantSuite` (5 tests, registered in both PR workflows): warns on a real `Compatible`-then-`None` path, stays quiet when only a child failed, stays quiet when the parent tags itself *and* a child failed, warns when the node is the only tagged expression, and warns with a placeholder when no reason was recorded. The tests capture WARN output through a log4j2 appender attached to the `QueryPlanSerde` logger. On noise, which was the main thing I wanted to check before proposing this: `CometArrayExpressionSuite` (57 tests) produced exactly one warning, from the test that disables the dispatcher config, and it is a true positive: ``` CometScalaUDF reported Compatible for UDF(knownnotnull(_4#1136)) but convert() returned None (scalaudf: spark.comet.exec.scalaUDF.codegen.enabled=false; expression has no native path so the plan falls back to Spark), so the enclosing operator falls back to Spark. ``` Under the default configuration it is silent. -- 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]
