schenksj commented on PR #4827:
URL:
https://github.com/apache/datafusion-comet/pull/4827#issuecomment-6631504568
@andygrove, replying to your Sep 28 automated review:
1. Fixed. `CometJvmDetour` checks every node of the item before it detours.
If any node has
`spark.comet.expression.<Name>.enabled=false`, it records a fallback
reason naming the config and
doesn't detour. The name is the serde's config name, or the class name
when there is no serde.
It checks the whole tree because the dispatcher would carry a disabled
descendant along. The
suite covers `Abs` (Rust-implemented serde), `RLike` (dispatch serde),
and a disabled `upper`
below an expression the conversion stops at. Each one falls back to Spark
with no dispatcher
activity. As you suggested, the suite, the fuzz tests and the benchmark
now drive expressions
through the detour with a new `testing`-category, `internal()` config,
`spark.comet.testing.unsupportedExpressions`, instead of the disable
switch.
One caveat I found along the way, which predates this PR: on Spark 4.x
the switch has no effect
on `main` for expressions that Spark lowers to `Invoke` / `StaticInvoke`.
With
`spark.comet.expression.ParseUrl.enabled=false`, `parse_url` still runs
in a `CometProject`, and
the same happens for `StructsToJson`, with the detour off. The 4.x shim
rebuilds `ParseUrl`,
whose conversion refuses, but it then returns `None`, and the generic
`Invoke` serde dispatches
the call anyway. The detour's own check can't see these, because the plan
holds the `Invoke`.
I've written that up as a separate issue (#7478) rather than widening
this PR, and narrowed the config
doc so it only claims what the detour itself does.
2. `main` had already rewritten the `CometBatchKernelCodegen.canHandle`
comment: it now rejects
`ExecSubqueryExpression` and explains why. The detour still checks for a
subquery itself, before
its non-Catalyst-class check, so the explain output names the subquery as
the reason rather
than the package check. I also corrected the `CometCodegenSuite` comment,
which said a populated subquery
is serialized into the kernel. In that test the subquery is a sibling of
the dispatched UDF.
--
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]
