feat: route Unsupported through codegen dispatch for opt-in serdes#4728
feat: route Unsupported through codegen dispatch for opt-in serdes#4728andygrove wants to merge 3 commits into
Conversation
CodegenDispatchFallback already routes Incompatible cases through the JVM codegen dispatcher so the projection stays inside the Comet pipeline instead of falling back to Spark. Apply the same routing to Unsupported cases: when getSupportLevel returns Unsupported and the serde mixes in CodegenDispatchFallback, run Spark's own doGenCode via the dispatcher before resorting to Spark fallback. Affects Concat (non-string children), SortArray (nested Struct/Null children), ArrayIntersect (collated strings), TruncDate and TruncTimestamp (formats outside the native set). Also refresh the expression compatibility docs: drop "faster native" wording (no measured claim), clarify that the default path runs in the JVM via Spark codegen, and render Unsupported reasons differently for CodegenDispatchFallback serdes (always JVM dispatch) vs everything else (always Spark fallback).
The Unsupported and Incompatible arms both pattern-match on CodegenDispatchFallback and call emitJvmCodegenDispatch. Lift that pattern into a single helper that returns the matched handler alongside the dispatched expression, so the Incompatible arm can reach nativeOptInConfigKeyOverride for its [COMET-INFO] hint without re-matching the same value.
…h codegen dispatch Concat, SortArray, TruncDate, and TruncTimestamp now route their previously Unsupported cases through the JVM codegen dispatcher and stay native instead of falling back to Spark. Update the test expectations to assert native execution and a matching answer rather than a fallback reason.
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
Thanks @andygrove! This looks like a clean, low-risk extension of the existing CodegenDispatchFallback mechanism. The Incompatible arm already routes through the JVM codegen dispatcher, and this PR applies the same routing to the Unsupported arm. No new execution logic is introduced, results match Spark by construction since doGenCode is Spark's own implementation, and the fallback safety net is preserved when the global flag is off or canHandle rejects the tree.
I traced the control flow in QueryPlanSerde and it is equivalent to the old Incompatible handling. I also enumerated every serde mixing CodegenDispatchFallback and confirmed the set that can return Unsupported matches the five named in the description (Concat, SortArray, ArrayIntersect, TruncDate, TruncTimestamp). The rest only return Compatible or Incompatible, so they are genuinely unaffected.
| override def getUnsupportedReasons(): Seq[String] = Seq( | ||
| "Nested arrays with `Struct` or `Null` child values are not supported natively and will" + | ||
| " fall back to Spark.") | ||
| "Nested arrays with `Struct` or `Null` child values are not supported natively") |
There was a problem hiding this comment.
SortArray actually has a second Unsupported branch, the non-boolean-literal ascendingOrder case at line 156, and this PR routes that through dispatch too. The description only calls out the nested Struct/Null case. The dispatch path looks safe since Spark requires ascendingOrder to be foldable so doGenCode can always compile it, but I do not see a test that confirms this case stays native rather than falling back. Could we add one, or confirm it is covered elsewhere?
| query | ||
| SELECT concat(c1, c2) AS x FROM test_array_concat | ||
|
|
||
| query expect_fallback(CONCAT supports only string input parameters) |
There was a problem hiding this comment.
Switching these from expect_fallback to plain query is the right call now that they stay native, and the same applies to the other four files. One side effect is that the removed expect_fallback assertions were the only tests pinning the disabled-dispatcher fallback for these cases. Would it be worth one test with spark.comet.exec.scalaUDF.codegen.enabled=false confirming these Unsupported cases still fall back to Spark cleanly? That locks in the safety net the description mentions.
| None | ||
| // `CodegenDispatchFallback` serdes have no native path for these cases either, but the | ||
| // dispatcher can still run Spark's own `doGenCode` inside the Comet pipeline. Try that | ||
| // before falling the projection back to Spark. No `[COMET-INFO]` hint here: unlike |
There was a problem hiding this comment.
The comment explaining why there is no [COMET-INFO] hint in the Unsupported arm is helpful. Did you consider a debug-level log when an Unsupported case routes through dispatch? Without the opt-in flag there is no plan hint, so anyone debugging why a projection stayed native has nothing to go on. Not essential, just a thought.
| * for the case where the dispatcher itself cannot handle the expression (e.g. the global codegen | ||
| * flag is off, or the kernel rejects the bound tree). | ||
| * | ||
| * Contract for `Unsupported` reasons on a `CodegenDispatchFallback` serde: the case must be |
There was a problem hiding this comment.
The expanded contract reads well. Worth noting for future authors that it is essentially always satisfiable: any case that reaches a serde already passed Spark analysis, so Spark supports it and doGenCode can compile it. The only way dispatch declines is a kernel limitation in canHandle, which falls back cleanly. No change needed, just confirming the reasoning holds.
2 participants
Summary
CodegenDispatchFallbackalready keepsIncompatiblecases inside the Comet pipeline by routing them through the JVM codegen dispatcher (Spark's owndoGenCodeinvoked inside the kernel). This PR extends the same routing toUnsupportedcases on those serdes: whengetSupportLevelreturnsUnsupportedand the serde mixes inCodegenDispatchFallback, run Spark's generated code via the dispatcher before falling back to Spark.Concat(non-string children),SortArray(nested arrays with Struct or Null children),ArrayIntersect(collated strings),TruncDateandTruncTimestamp(formats outside the native set). All five conditions are things Spark itself supports, so the projection now stays in Comet's pipeline instead of bouncing out to Spark.NativeOptInAvailableserdes runs in the JVM via Spark codegen, and rendersUnsupportedreasons differently forCodegenDispatchFallbackserdes (always handled via JVM dispatch) vs everything else (still falls back to Spark).Behavior change
For the five
CodegenDispatchFallbackserdes listed above, inputs that previously caused the entire projection to fall back to Spark now stay inside Comet via dispatch. Dispatch already returnsNonecleanly when its global flag (spark.comet.exec.scalaUDF.codegen.enabled) is disabled orCometBatchKernelCodegen.canHandlerefuses the tree, so the safety net to Spark fallback is preserved.No
[COMET-INFO]plan hint is emitted in theUnsupportedarm — unlikeIncompatible, there is no native opt-in for the user to flip.Test plan
./mvnw test -pl spark -Pspark-3.5 -Dsuites="org.apache.comet.CometExpressionSuite"(concat, sort_array, array_intersect, trunc coverage)Concatwith non-string children — confirm no[COMET: …fall back…]and the projection remains aCometProjectstring.md,array.md,datetime.mdfor the new "no native implementation, runs in the JVM" wording