schenksj commented on code in PR #4827:
URL: https://github.com/apache/datafusion-comet/pull/4827#discussion_r4467904543
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -847,6 +886,33 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
None
}
})
+ .orElse {
+ // Last-resort JVM expression detour. Any `None` above — no registered
handler, an
+ // `Unsupported`/opt-out `Incompatible` support level, or a handler
whose `convert`
+ // returned `None` — means this node has no native translation. Inside
a detour-eligible
+ // context (projection / filter, see `jvmDetourAllowed`) retry the
*outermost* unsupported
+ // node as a JVM detour: `emitJvmCodegenDispatch` ships its argument
columns over Arrow FFI
+ // and evaluates it with Spark's own codegen inside the Comet
pipeline, so the operator —
+ // and the native island around it — stays native. Supported ancestors
keep converting
+ // natively and reference this detour's output. If the detour cannot
fire (feature off,
+ // dispatcher off, or `CometBatchKernelCodegen.canHandle` rejects the
tree), `None`
+ // propagates unchanged and the operator falls back exactly as before.
+ if (jvmDetourAllowed) {
+ CometScalaUDF.emitJvmCodegenDispatch(expr, inputs, binding).map {
proto =>
Review Comment:
Upstream fixed this in #6712 (`DispatchOccurrence`). A tree with a
`Nondeterministic` node is
tagged with a unique occurrence id before serialization, so two identical
occurrences get separate
kernels and state. The rebased branch picks that up. I added
`SELECT uuid(42) AS a, uuid(42) AS b` (Spark 4.0+, `Uuid` forced through the
detour) and a
version-independent `abs(rand(42))` twice. Both check the result against
Spark and that `a == b` on
every row, and both pass on the default profile (Spark 4.1).
##########
spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala:
##########
@@ -847,6 +886,33 @@ object QueryPlanSerde extends Logging with CometExprShim
with CometTypeShim {
None
}
})
+ .orElse {
+ // Last-resort JVM expression detour. Any `None` above — no registered
handler, an
+ // `Unsupported`/opt-out `Incompatible` support level, or a handler
whose `convert`
+ // returned `None` — means this node has no native translation. Inside
a detour-eligible
+ // context (projection / filter, see `jvmDetourAllowed`) retry the
*outermost* unsupported
+ // node as a JVM detour: `emitJvmCodegenDispatch` ships its argument
columns over Arrow FFI
+ // and evaluates it with Spark's own codegen inside the Comet
pipeline, so the operator —
+ // and the native island around it — stays native. Supported ancestors
keep converting
+ // natively and reference this detour's output. If the detour cannot
fire (feature off,
+ // dispatcher off, or `CometBatchKernelCodegen.canHandle` rejects the
tree), `None`
+ // propagates unchanged and the operator falls back exactly as before.
+ if (jvmDetourAllowed) {
Review Comment:
`main` now supports `IntervalYearVector` and `IntervalMonthDayNanoVector`
inputs in
`CometScalaUDFCodegen.specFor` (#4976), so `canHandle` and the input getters
agree again. I added
your two-projection case, `abs(ym)` over `make_ym_interval(CAST(rand(42) *
10 AS INT))`, with both
expressions forced through the detour. It matches Spark and keeps the
projection in Comet.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
