schenksj commented on PR #4827: URL: https://github.com/apache/datafusion-comet/pull/4827#issuecomment-4931178317
@andygrove Yes — that's exactly right, with one refinement on "any." **No new execution mechanism.** The JVM-detour machinery already ships end-to-end on `main` — `JvmScalarUdfExpr` (a DataFusion `PhysicalExpr` that exports its args over Arrow FFI and calls back into the JVM), `CometUdfBridge`, and the `CometScalaUDFCodegen` dispatcher that Janino-compiles Spark's own `doGenCode` into an Arrow-direct batch kernel. Today that path is reached only for a curated **allow-list**: `ScalaUDF`, serdes extending `CometCodegenDispatch` (HOFs, CSV/JSON, parts of date/time), and `Incompatible` serdes tagged `CodegenDispatchFallback`. An expression with no serde handler, or whose `convert` returns `None`, aborts the **whole operator** and drops the native island below it. This PR adds a last-resort `.orElse` hook so that unsupported node detours to the JVM instead. No proto change, no new JNI surface, no new operator. **On "any expression" — gated, not unconditional.** The hook only fires when the existing dispatcher `canHandle` accepts the subtree, which rejects aggregates, generators, `Unevaluable`, unsupported types, and oversized trees (and this PR adds a subquery guard). It's also scoped to `CometProjectExec` / `CometFilterExec` in v1 — deliberately kept out of join keys, sort keys, group/agg, and partitioning expressions until benchmarked. So: within Project/Filter, any *codegen-capable* expression can detour, rather than only the allow-listed ones — but it's not a blanket "everything routes to codegen." **Ecosystem precedent** (since this is the "does it disrupt planned work" question): this is the same lever Gluten ships as `ColumnarPartialProjectExec` (on by default, `spark.gluten.sql.columnar.partial.project`) and Blaze/Auron ship as their partial-project split. The difference is that Comet's version needs no dedicated operator — the detoured subtree is just one more `PhysicalExpr` in DataFusion's existing `ProjectionExec`/`FilterExec` vector, so there's no per-operator batch recomposition or extra C2R/R2C transition. It's **default-off** (`spark.comet.exec.jvmDetour.enabled=false`) and experimental, so it can't affect anyone who doesn't opt in. @mbutrovich — happy to align this with whatever you were sketching; if your idea was operator-shaped (a `CometPartialProjectExec`), the design doc has a "considered alternatives" section on why v1 went expression-granular instead, and I'd value your read on it. *(Disclosure: this PR and this response were written with help from AI — Claude Code.)* -- 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]
