comphead opened a new pull request, #7473: URL: https://github.com/apache/datafusion-comet/pull/7473
## Which issue does this PR close? Part of #5781, which stays open. ## Rationale for this change `CometSize` serializes its child twice, as `CASE WHEN isnotnull(child) THEN size(child) ELSE <null answer> END`. DataFusion's `CASE` evaluates the `WHEN` over the batch and then the `THEN`, so the child runs twice. That costs little for a column, but an expensive child pays twice. For a Scala UDF it is two dispatcher calls per row. The native `size` already returns -1 for a NULL array or map, which is Spark's answer when `spark.sql.legacy.sizeOfNull` is on and ANSI is off, the default on Spark 3.x. A child that cannot be NULL never takes the NULL branch either. In both cases the child can go straight to `size`, the way `CometArrayAppend` already skips its guard for a non-nullable array. This came up in a production Spark 3.4 job. Spark's `InferFiltersFromGenerate` added `size(x) > 0 AND isnotnull(x)` for an `explode`, and the plan shows that filter below an aggregate, with `x` replaced by the Scala UDF call that produced it. Comet called the UDF three times per row where Spark called it twice. Estimated from the UDF's own timers, the extra call and its JVM round trip were about a fifth of that stage's task time. Evaluating the child once in these cases also keeps a nondeterministic child in step there (#5781). The guard stays for a nullable child when `legacySizeOfNull` is off, which includes Spark 4's default ANSI mode, so #5781 still applies to `size` in that case and to the other three expressions it lists. The condition is the one @grorge123 proposed for `CometSize` in #5526. The discussion on #5781 left narrowing the guards for a follow-up, and this takes only the `size` part. ## What changes are included in this PR? - `CometSize.convert` emits the native `size` alone when `legacySizeOfNull` is on or the child is not nullable, and keeps the guard otherwise. - Two native comments that said `size` only ever sees a null-free array in a Comet plan now say when it does. ## How are these changes tested? A new test in `CometArrayExpressionSuite` counts calls to a Scala UDF under `size`: with `legacySizeOfNull` on and a nullable UDF, and with it off and a non-nullable UDF. It checks the answer and native execution, and that Comet calls the UDF as many times as Spark does. With the guard, Comet would call it once more for each row whose array is not NULL. `size.sql` already covers both `legacySizeOfNull` settings for array and map columns with NULLs. The new test has not been run locally yet. `test-compile` passes on the default profile and with `-Pspark-3.5 -Pstrict-warnings`. -- 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]
