andygrove opened a new pull request, #5599: URL: https://github.com/apache/datafusion-comet/pull/5599
## Which issue does this PR close? Part of #5574. Follows #5595 (dead guards); this is the slice with actual value. ## Rationale for this change Serdes whose only path is the JVM codegen dispatcher reported `Compatible` from `getSupportLevel` and then discovered inside `convert` that the dispatcher would not run the expression — either because `spark.comet.exec.scalaUDF.codegen.enabled` is off, or because `CometBatchKernelCodegen.canHandle` rejected the bound tree. That breaks the serde invariant, and it is the one shape the dispatcher cannot see: `dispatchIfFallback` is reached only from the `Unsupported` and `Incompatible` arms of `exprToProtoInternal` (`QueryPlanSerde.scala:941` and `:968`). This was not a hypothetical. It is the single violation the warning in #5593 turned up on a real suite run. ## What changes are included in this PR? Extract the two checks into `CometScalaUDF.dispatchSupportLevel` and report them from `getSupportLevel` on `CometCodegenDispatch` (62 serdes inherit it) and on `CometScalaUDF`. The binding step `canHandle` needs is factored into `bindForDispatch` and shared with `emitJvmCodegenDispatch`. **Behavior is unchanged.** `CometCodegenDispatch` does not mix in `CodegenDispatchFallback`, so an `Unsupported` result tags the same reason text and falls the operator back to Spark exactly as the `convert`-side decline did. The reason strings are reproduced verbatim, including the `exprName:` prefix, so EXPLAIN output is identical. `emitJvmCodegenDispatch` keeps its own copies of both checks. For a gated serde that is a cheap re-verification; it is kept because roughly ten serdes (`CometRLike`, `CometRegExpReplace`, `CometStringSplit`, `CometInitCap`, the shims, …) call it directly from `convert` without gating first. **Not covered, deliberately:** six `CometCodegenDispatch` subclasses override `getSupportLevel` — `CometGetJsonObject`, `CometLengthOfJsonArray`, `CometStructsToJson`, `CometJsonToStructs`, `CometMakeTimestamp`, `CometToUnixTimestamp`. Each needs the gate composed with its own native opt-in branch (under `allowIncompatible=true` they take the native path, where the dispatcher gate should not apply), so a blanket base-class change would be wrong for them. Same for the inline `convert` callers. Both are follow-ups on #5574. ## How are these changes tested? Three new tests in `CometCodegenSuite`, using `find_in_set` as a stand-in for the plain dispatch serdes: `Compatible` when the dispatcher will run the expression, `Unsupported` naming the config key when the dispatcher is disabled, and `Unsupported` when `canHandle` refuses the tree (`NullType` output). Regression runs on Spark 4.1, all green: - `CometCodegenSuite` — 88 tests (85 existing + 3 new) - `CometCodegenHOFSuite`, `CometCodegenSourceSuite`, `CometFallbackInvarianceSuite`, `CometScalaUDFClassLoaderSuite` — 158 tests - `CometExpressionSuite`, `CometSqlFileTestSuite`, `CometTemporalExpressionSuite`, `CometJsonExpressionSuite`, `CometStringExpressionSuite`, `CometMathExpressionSuite` — 688 tests I also stacked this on top of #5593 locally and re-ran `CometArrayExpressionSuite`, the suite that previously reported one invariant violation: it now reports zero, with all 57 tests still passing. -- 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]
