andygrove opened a new pull request, #5692: URL: https://github.com/apache/datafusion-comet/pull/5692
## Which issue does this PR close? Closes #5573. Closes #5575. Both are children of the codegen-dispatch coverage audit, #5572. ## Rationale for this change `CometStaticInvoke` dispatches on an allowlist of `(functionName, staticObject)` pairs and fails the whole operator back to Spark for anything else. `Invoke` has no entry in the serde map at all. Both nodes are codegen-friendly — `StaticInvoke.doGenCode` emits a static method call, `Invoke.doGenCode` emits a method call on the target object — so routing them through the JVM codegen dispatcher runs Spark's own implementation inside the Comet pipeline and matches Spark by construction, instead of losing the whole projection for every lowering nobody has allowlisted yet. Spark keeps adding these. Three concrete cases that were fallbacks before this PR: - `lpad` / `rpad` on **binary** input lower to `StaticInvoke(ByteArray, funcName, ...)` on every supported Spark version (3.4 through 4.1). - Spark 4.1's `to_time(str, fmt)` lowers to an evaluator-backed `Invoke`. - `encode`, and the `utf-8` form of `to_binary`, lower to `StaticInvoke(Encode.encode, ...)` on Spark 4.0+. The compatibility guide already recorded both as falling back for exactly this reason. #5573 is the prerequisite: the closure-serialize in `emitJvmCodegenDispatch` was the one failure mode in that method that threw during planning rather than degrading to a Spark fallback, and a catch-all is what starts handing the dispatcher arbitrary trees. ## What changes are included in this PR? **#5573** — `CometScalaUDF.emitJvmCodegenDispatch` wraps the closure-serialize in `try` / `catch NonFatal`, tags a fallback reason and returns `None`, matching the shape of every other check around it. `NonFatal` rather than `NotSerializableException` because Java serialization reports an unserializable object graph as one of several exception types depending on where it trips, and a custom `writeObject` can throw anything. The scaladoc now states that the method never throws. **#5575** — `CometStaticInvoke.convert`'s `case None` routes to `CometStaticInvokeCodegenDispatch`; if the dispatcher declines (config off, or `canHandle` refuses the tree), it still tags a fallback reason naming the function and the declaring class, so the diagnostic that existed before is preserved. New `CometInvoke extends CometCodegenDispatch[Invoke]`, registered as `classOf[Invoke]` in `QueryPlanSerde.miscExpressions`. **Docs** — `expressions.md` rows for `encode` and `to_binary`, and the `lpad` / `rpad` known-limitation notes in the string expression audit. ### One deliberate deviation from #5572 #5572 frames these items as "add the `CodegenDispatchFallback` mixin". I implemented that variant first for `CometStaticInvoke` and it **produces wrong answers**, so this PR scopes the catch-all to functions with no handler at all. The mixin also rescues a *handler's* `Unsupported`. `CometIcebergTruncate` declines a decimal because Iceberg's `truncate` can return a value wider than the column's declared precision, which Spark turns into null only when the row is materialized. The dispatcher writes into the same Arrow `Decimal128(p, s)` vector a native kernel would, so the rescued plan returned `-100000000000000.0000` for a row Spark nulls, in `CometIcebergSystemFunctionSuite`'s "truncate on a decimal falls back to Spark". The mixin's stated contract is that the case "must be something `Expression.doGenCode` can compile". That does not cover a limit living at the **Arrow output boundary**, which the dispatcher and a native kernel share. Worth checking each remaining child issue's `Unsupported` reason against that distinction. The reasoning is recorded in `CometStaticInvoke`'s scaladoc and in the Iceberg test so it is not re-litigated. ### Residual risk, not addressed here The dispatcher Janino-compiles at execute time with no recovery, so a `StaticInvoke` whose target class is not public would fail the task where Spark's whole-stage codegen falls back to interpreted evaluation. I could not exhibit one — Spark's own lowerings all use public utility classes, and the DSv2 magic-method contract effectively requires a public class since Spark's own WSCG emits the same call. A plan-time guard would have to replicate `StaticInvoke.doGenCode`'s name mangling, and getting that approximation wrong would reject valid dispatches, which is the worse failure. Flagging it rather than guessing. Encoder and deserializer trees — which are most `StaticInvoke` / `Invoke` usage in typed Dataset operations — are unaffected either way: their arguments are `ObjectType`, which `CometBatchKernelCodegen.isSupportedDataType` rejects, so `canHandle` declines them and they fall back exactly as before. ## How are these changes tested? New tests in `CometCodegenSuite`: - `dispatch falls back cleanly when the bound tree cannot be closure-serialized (#5573)` — an `Invoke` holding a deliberately non-`Serializable` target. `canHandle` greenlights the tree (string in, string out), so the serializer is the step that refuses, and the assertion is on the fallback reason rather than on an escaping exception. - `unrecognized StaticInvoke routes through the dispatcher instead of falling back (#5575)` — `lpad` on a binary column: asserts the dispatcher ran and the operator stayed native, then asserts a clean fallback with the dispatcher disabled. - `Invoke routes through the codegen dispatcher (#5575)` — pins the serde-map registration by going through `QueryPlanSerde.exprToProto`, then runs the compiled kernel to check the emitted method call evaluates. Existing tests updated to the new behaviour: - `CometStringExpressionSuite`'s binary `lpad` / `rpad` flip from `checkSparkAnswerAndFallbackReason` to `checkSparkAnswerAndOperator`. - `expressions/datetime/to_time.sql` flips two `expect_fallback(invoke is not supported)` blocks to plain `query`. - `CometIcebergSystemFunctionSuite`'s unlisted-static-invoke test now asserts the dispatch proto, and that the declaring-class diagnostic survives when the dispatcher is off. Regression runs, all on this branch: | Scope | Result | | --- | --- | | `CometExpressionSuite`, `CometCodegenFuzzSuite`, `CometSqlFileTestSuite`, `CometExecSuite` (4.1) | 782 passed | | `CometCodegenSuite`, `CometStringExpressionSuite`, `CometIcebergSystemFunctionSuite`, `CometSqlFileTestSuite` (4.1) | 602 passed | | `CometStringExpressionSuite`, `CometCodegenSuite` (3.4 and 3.5) | 121 passed each | | `CometStringExpressionSuite`, `CometCodegenSuite`, `CometSqlFileTestSuite` (4.0) | 590 passed | Cross-compiled against Spark 3.4 / 3.5 / 4.0 / 4.1; spotless and scalastyle clean; `mvn package` regenerates the docs with no churn beyond the note edits above. -- 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]
