Skip to content

chore: report the codegen-dispatch gate from getSupportLevel - #5599

Draft
andygrove wants to merge 1 commit into
mainfrom
andygrove/dispatch-gate-support-level
Draft

chore: report the codegen-dispatch gate from getSupportLevel#5599
andygrove wants to merge 1 commit into
mainfrom
andygrove/dispatch-gate-support-level

Conversation

@andygrove

Copy link
Copy Markdown
Member

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 getSupportLevelCometGetJsonObject, 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.

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.

Extract the two checks into CometScalaUDF.dispatchSupportLevel and report
them from getSupportLevel on CometCodegenDispatch (62 serdes inherit it)
and CometScalaUDF. Behaviour 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 before.

Six CometCodegenDispatch subclasses override getSupportLevel
(CometGetJsonObject, CometLengthOfJsonArray, CometStructsToJson,
CometJsonToStructs, CometMakeTimestamp, CometToUnixTimestamp) and are not
covered here, since each has to compose the gate with its own native
opt-in branch. Same for the serdes that call emitJvmCodegenDispatch
inline from convert. Both are follow-ups.

Related to #5574.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant