sunchao commented on code in PR #5692:
URL: https://github.com/apache/datafusion-comet/pull/5692#discussion_r3937536823
##########
spark/src/main/scala/org/apache/comet/serde/statics.scala:
##########
@@ -78,16 +95,38 @@ object CometStaticInvoke extends
CometExpressionSerde[StaticInvoke] {
case Some(handler) =>
handler.convert(expr, inputs, binding)
case None =>
- // Every Iceberg system function is named `invoke`, so name the
declaring class too.
- withFallbackReason(
- expr,
- s"Static invoke expression: ${expr.functionName} is not supported " +
- s"(declared on ${expr.staticObject.getName})")
- None
+ // Nothing in the allowlist covers this lowering, so run Spark's own
implementation inside
+ // the Comet pipeline rather than failing the whole operator back to
Spark.
+ // `StaticInvoke.doGenCode` emits a static method call, so the kernel
matches Spark by
+ // construction. Spark 4.x keeps lowering more `RuntimeReplaceable`
functions this way
+ // (`encode`, `is_valid_utf8`, the `TIME` family, ...) and `lpad` /
`rpad` on binary has
+ // lowered to `StaticInvoke(ByteArray, ...)` since Spark 3.4.
+ //
+ // The encoder and deserializer trees that make up most `StaticInvoke`
usage in typed
+ // Dataset operations are unaffected: their arguments are
`ObjectType`, which
+ // `CometBatchKernelCodegen.isSupportedDataType` rejects, so the
dispatcher declines them
+ // and they fall back exactly as before.
+ CometStaticInvokeCodegenDispatch.convert(expr, inputs, binding).orElse
{
Review Comment:
### Performance
[P2] Could you add matched microbenchmark results for the newly enabled
static-call and receiver-call paths? This catch-all changes existing binary
padding, encoding and `to_time` queries from Spark operator fallback to the JVM
dispatcher. The function itself still runs Spark code, while dispatch adds
expression transport, first-use compilation, per-batch bridge work and Arrow
output materialization. The benefit from keeping surrounding operators in Comet
needs to be measured against those costs, especially for short binary/string
values and small inputs.
Please compare dispatch enabled with the existing dispatcher-disabled
fallback on identical nonconstant inputs, covering `lpad`/`rpad`, `encode` or
UTF-8 `to_binary`, and one evaluator-backed `Invoke` such as `to_time`. Include
small and multi-batch inputs plus a mixed native projection, verify equal
results, and state the Spark/JDK versions, batch size and warmup. Separate
first-use compilation from steady-state timings. No slowdown has been measured
here. These results are needed to validate the new default execution-path
tradeoff.
--
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]
