chore: report the codegen-dispatch gate from getSupportLevel - #5599
Draft
andygrove wants to merge 1 commit into
Draft
chore: report the codegen-dispatch gate from getSupportLevel#5599andygrove wants to merge 1 commit into
andygrove wants to merge 1 commit into
Conversation
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.
andygrove
force-pushed
the
andygrove/dispatch-gate-support-level
branch
from
August 31, 2026 16:18
6ae8bfa to
9c4b983
Compare
andygrove
marked this pull request as draft
August 31, 2026 16:50
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
CompatiblefromgetSupportLeveland then discovered insideconvertthat the dispatcher would not run the expression — either becausespark.comet.exec.scalaUDF.codegen.enabledis off, or becauseCometBatchKernelCodegen.canHandlerejected the bound tree.That breaks the serde invariant, and it is the one shape the dispatcher cannot see:
dispatchIfFallbackis reached only from theUnsupportedandIncompatiblearms ofexprToProtoInternal(QueryPlanSerde.scala:941and: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.dispatchSupportLeveland report them fromgetSupportLevelonCometCodegenDispatch(62 serdes inherit it) and onCometScalaUDF. The binding stepcanHandleneeds is factored intobindForDispatchand shared withemitJvmCodegenDispatch.Behavior is unchanged.
CometCodegenDispatchdoes not mix inCodegenDispatchFallback, so anUnsupportedresult tags the same reason text and falls the operator back to Spark exactly as theconvert-side decline did. The reason strings are reproduced verbatim, including theexprName:prefix, so EXPLAIN output is identical.emitJvmCodegenDispatchkeeps 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 fromconvertwithout gating first.Not covered, deliberately: six
CometCodegenDispatchsubclasses overridegetSupportLevel—CometGetJsonObject,CometLengthOfJsonArray,CometStructsToJson,CometJsonToStructs,CometMakeTimestamp,CometToUnixTimestamp. Each needs the gate composed with its own native opt-in branch (underallowIncompatible=truethey 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 inlineconvertcallers. Both are follow-ups on #5574.How are these changes tested?
Three new tests in
CometCodegenSuite, usingfind_in_setas a stand-in for the plain dispatch serdes:Compatiblewhen the dispatcher will run the expression,Unsupportednaming the config key when the dispatcher is disabled, andUnsupportedwhencanHandlerefuses the tree (NullTypeoutput).Regression runs on Spark 4.1, all green:
CometCodegenSuite— 88 tests (85 existing + 3 new)CometCodegenHOFSuite,CometCodegenSourceSuite,CometFallbackInvarianceSuite,CometScalaUDFClassLoaderSuite— 158 testsCometExpressionSuite,CometSqlFileTestSuite,CometTemporalExpressionSuite,CometJsonExpressionSuite,CometStringExpressionSuite,CometMathExpressionSuite— 688 testsI 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.