Part of #5572.
CometScalaUDF.emitJvmCodegenDispatch closure-serializes the bound expression with no guard:
val serializer = SparkEnv.get.closureSerializer.newInstance()
val buffer = serializer.serialize(boundExpr)
spark/src/main/scala/org/apache/comet/serde/CometScalaUDF.scala:111-112
Every other failure mode in that method degrades cleanly — the disabled-config check, CometBatchKernelCodegen.canHandle, the literal-payload conversion, the data-arg conversion and the return-type conversion all call withFallbackReason and return None, which the caller treats as a clean Spark fallback. The serialize call is the one step that can throw instead, and a NotSerializableException there escapes during planning, which is a much worse failure than falling back.
Today's callers are all curated expression types where this is very unlikely to bite. It matters because of where the sweep wants to go next: routing an unrecognized StaticInvoke or Invoke through the dispatcher as a catch-all hands it arbitrary trees, including ones holding references the closure serializer will refuse. So this should land before that change.
The fix is small — wrap the serialize in a try / catch, call withFallbackReason with the exception message and return None, matching the shape of the checks around it. A unit test can force it with an expression capturing a deliberately non-serializable object.
Part of #5572.
CometScalaUDF.emitJvmCodegenDispatchclosure-serializes the bound expression with no guard:spark/src/main/scala/org/apache/comet/serde/CometScalaUDF.scala:111-112Every other failure mode in that method degrades cleanly — the disabled-config check,
CometBatchKernelCodegen.canHandle, the literal-payload conversion, the data-arg conversion and the return-type conversion all callwithFallbackReasonand returnNone, which the caller treats as a clean Spark fallback. The serialize call is the one step that can throw instead, and aNotSerializableExceptionthere escapes during planning, which is a much worse failure than falling back.Today's callers are all curated expression types where this is very unlikely to bite. It matters because of where the sweep wants to go next: routing an unrecognized
StaticInvokeorInvokethrough the dispatcher as a catch-all hands it arbitrary trees, including ones holding references the closure serializer will refuse. So this should land before that change.The fix is small — wrap the serialize in a
try/catch, callwithFallbackReasonwith the exception message and returnNone, matching the shape of the checks around it. A unit test can force it with an expression capturing a deliberately non-serializable object.