andygrove commented on PR #6697: URL: https://github.com/apache/datafusion-comet/pull/6697#issuecomment-6009117322
Thanks @mbutrovich, this breakdown is really useful, and I think your reading of it is right. The gap I attributed to the per-row call is mostly the dispatcher's overhead around the call, so I've corrected that. **Benchmark failure.** You're right. The native-plan check only stripped the top-level adaptive node, so it flagged the `ShuffleQueryStage`. It now checks the plan inside each query stage, the same way `CometTestBase.checkCometOperatorsInFinalPlan` does, and it also excludes `AQEShuffleReadExec`. That's fixed in c362d0f59, and the benchmark runs cleanly again. **Framing.** Agreed. I rewrote the "Choosing between an ordinary UDF and a vectorized UDF" section of the user guide around what a function of one row can't express: work done once per batch, a different value representation such as UTF-8 bytes, and one call per batch into a batch-oriented library. It now says that rewriting a simple function of primitive values gains little. The benchmark's scaladoc and the PR description now put the gap down to dispatch overhead rather than the call itself. **The dispatcher questions.** I think all three are worth doing, each in its own PR, and I've filed an issue for each. 1. **Null guard** (#6704). Yes. The serde could recognize Spark's `if(isnull(c), null, f(knownnotnull(c)))` shape around a `ScalaUDF` and hand the whole `If` to the dispatcher. The null check then becomes a branch in the kernel's loop instead of a filter-and-merge `CASE` over the batch. It only matches that exact shape, so it shouldn't change any results. 2. **Cache key** (#6705). Yes. A hash computed on the driver and shipped through the proto would remove both the copy of the closure and the hash on every batch. 3. **Boxed parameters** (#6706). Probably, but carefully. A direct null check and unbox would skip the per-row projection, but it has to do exactly what Spark's input encoder does for each boxed type. I'd start with the boxed primitives, where that's easy to show. -- 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]
