You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/06/25 17:49:41 UTC
[PR] feat: route Unsupported through codegen dispatch for opt-in serdes [datafusion-comet]
andygrove opened a new pull request, #4728:
URL: https://github.com/apache/datafusion-comet/pull/4728
## Summary
- `CodegenDispatchFallback` already keeps `Incompatible` cases inside the Comet pipeline by routing them through the JVM codegen dispatcher (Spark's own `doGenCode` invoked inside the kernel). This PR extends the same routing to `Unsupported` cases on those serdes: when `getSupportLevel` returns `Unsupported` and the serde mixes in `CodegenDispatchFallback`, run Spark's generated code via the dispatcher before falling back to Spark.
- Affects `Concat` (non-string children), `SortArray` (nested arrays with Struct or Null children), `ArrayIntersect` (collated strings), `TruncDate` and `TruncTimestamp` (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.
- Refreshes the expression compatibility docs: drops the "faster native" wording (no measured speed claim), clarifies that the default path for `NativeOptInAvailable` serdes runs in the JVM via Spark codegen, and renders `Unsupported` reasons differently for `CodegenDispatchFallback` serdes (always handled via JVM dispatch) vs everything else (still falls back to Spark).
## Behavior change
For the five `CodegenDispatchFallback` serdes listed above, inputs that previously caused the entire projection to fall back to Spark now stay inside Comet via dispatch. Dispatch already returns `None` cleanly when its global flag (`spark.comet.exec.scalaUDF.codegen.enabled`) is disabled or `CometBatchKernelCodegen.canHandle` refuses the tree, so the safety net to Spark fallback is preserved.
No `[COMET-INFO]` plan hint is emitted in the `Unsupported` arm — unlike `Incompatible`, 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)
- [ ] Spot-check extended explain on a `Concat` with non-string children — confirm no `[COMET: …fall back…]` and the projection remains a `CometProject`
- [ ] Regenerate docs and review `string.md`, `array.md`, `datetime.md` for the new "no native implementation, runs in the JVM" wording
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route Unsupported through codegen dispatch for opt-in serdes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4728:
URL: https://github.com/apache/datafusion-comet/pull/4728#discussion_r3484639018
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -121,8 +121,7 @@ object CometSortArray extends CometExpressionSerde[SortArray] with CodegenDispat
" floating-point types is not 100% compatible with Spark")
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")
Review 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?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route Unsupported through codegen dispatch for opt-in serdes [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4728:
URL: https://github.com/apache/datafusion-comet/pull/4728
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route Unsupported through codegen dispatch for opt-in serdes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4728:
URL: https://github.com/apache/datafusion-comet/pull/4728#discussion_r3484638698
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -121,8 +121,7 @@ object CometSortArray extends CometExpressionSerde[SortArray] with CodegenDispat
" floating-point types is not 100% compatible with Spark")
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")
Review 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?
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -121,8 +121,7 @@ object CometSortArray extends CometExpressionSerde[SortArray] with CodegenDispat
" floating-point types is not 100% compatible with Spark")
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")
Review 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?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route Unsupported through codegen dispatch for opt-in serdes [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4728:
URL: https://github.com/apache/datafusion-comet/pull/4728#discussion_r3484640762
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -767,8 +767,14 @@ object QueryPlanSerde extends Logging with CometExprShim with CometTypeShim {
}
handler.getSupportLevel(expr) match {
case Unsupported(notes) =>
- withFallbackReason(expr, notes.getOrElse(""))
- 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
Review 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.
##########
spark/src/main/scala/org/apache/comet/serde/CometExpressionSerde.scala:
##########
@@ -101,14 +105,22 @@ trait CometExpressionSerde[T <: Expression] {
}
/**
- * Opt-in marker for expression serdes that have a native implementation which is `Incompatible`
- * with Spark for some inputs. When such an expression reports `Incompatible` and the user has not
- * enabled `allowIncompatible` for it, mixing in this trait routes it through the JVM codegen
- * dispatcher (running Spark's own `doGenCode` inside the Comet pipeline) instead of falling the
- * projection back to Spark, so it stays native while still matching Spark exactly.
+ * Mixin for serdes whose native implementation cannot match Spark for some inputs. When
+ * `getSupportLevel` returns `Incompatible` and the user has not enabled `allowIncompatible`, the
+ * expression routes through the JVM codegen dispatcher (Spark's own `doGenCode` inside the Comet
+ * pipeline) instead of falling the projection back to Spark. When `getSupportLevel` returns
+ * `Unsupported`, the expression always routes through the dispatcher -- the serde is declaring
+ * "no native path exists for this case; run Spark's code in-pipeline." Spark fallback is reserved
+ * 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
Review 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.
##########
spark/src/test/resources/sql-tests/expressions/array/array_concat.sql:
##########
@@ -24,17 +24,20 @@ CREATE TABLE test_array_concat(c1 array<int>, c2 array<int>, c3 array<int>, c4 a
statement
INSERT INTO test_array_concat VALUES (array(0, 1), array(2, 3), array(), array(null), null), (array(1, 2), array(3, 4), array(), array(null), null), (array(2, 3), array(4, 5), array(), array(null), null)
-query expect_fallback(CONCAT supports only string input parameters)
+-- Concat mixes in CodegenDispatchFallback, so non-string (array) children have no native path
+-- but route through the JVM codegen dispatcher (Spark's own Concat.doGenCode inside the Comet
+-- pipeline) and stay native while matching Spark exactly.
+query
SELECT concat(c1, c2) AS x FROM test_array_concat
-query expect_fallback(CONCAT supports only string input parameters)
Review 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.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: route Unsupported through codegen dispatch for opt-in serdes [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4728:
URL: https://github.com/apache/datafusion-comet/pull/4728#issuecomment-4834808815
Thanks for the review @mbutrovich. I just pushed some updated. PTAL when you can.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org