dwsmith1983 commented on issue #5781: URL: https://github.com/apache/datafusion-comet/issues/5781#issuecomment-6006818128
@grorge123 @mbutrovich following the ask on #5526 to settle the approach for the four null guards here. Both PRs decline a nondeterministic child in `size`, `array_append`, `arrays_zip` and `map_from_arrays`, since the native `CASE WHEN child IS NOT NULL` guard serializes the child twice and a stateful child advances in each copy. They differ in what happens next. #5526 reports `Unsupported` without the dispatcher mixin, so the projection falls back to Spark. #5867 mixes in `CodegenDispatchFallback`, so Spark's generated code evaluates the child once inside the Comet projection, which is also where #5867 sends `element_at`'s ANSI arm for a nondeterministic operand, and the surrounding operators stay native. The fixtures in #5867 compare the dispatched answers with Spark's for a stateful operand, a non-nullable stateful operand and a deterministic nullable one. Proposal: keep the dispatcher route from #5867 for these four and drop the `NullGuard` handling from #5526, which then has one less thing to carry when it is split. Two things from #5526 belong beside that, each as its own change: 1. The dispatcher caches one kernel instance per task per serialized expression bytes (`CometScalaUDFCodegen.kernelCache`), so two byte-identical nondeterministic occurrences in one projection share one state where Spark gives each occurrence its own. That is on `main` for every dispatched nondeterministic expression and is what the per-occurrence kernel state in #5526 fixes; it deserves its own issue and PR. #5867 does not widen it: those four shapes returned wrong answers natively before, and the single-occurrence case is what the fixtures cover. 2. #5526 also narrows the gate where it stops emitting the guard: `size` with `legacySizeOfNull` on or a non-nullable child goes straight to the native kernel, and `array_append` gates the item only when the array can be NULL. #5867 keeps the guards as they are on `main` and gates on determinism alone. Emitting fewer guards is a serde change of its own, separate from the routing question, and works as a follow-up to either PR. -- 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]
