andygrove opened a new issue, #5781: URL: https://github.com/apache/datafusion-comet/issues/5781
## Describe the bug Four serdes build a `CASE WHEN <child> IS NOT NULL THEN <native op over child> ELSE <null> END` in which the child is serialized twice. Native evaluation advances each copy independently, so for a stateful child the null check and the operation see different values. The result is a silent wrong answer, not an error. | serde | site | where the child is serialized a second time | | --- | --- | --- | | `CometSize` | `spark/src/main/scala/org/apache/comet/serde/arrays.scala:765` | `createIsNotNullExprProto` over `expr.child` | | `CometArrayAppend` | `arrays.scala:54` | `createUnaryExpr` over `expr.children.head` | | `CometArraysZip` | `arrays.scala:844` | `expr.children.map(IsNotNull(_)).reduce(And)` | | `CometMapFromArrays` | `spark/src/main/scala/org/apache/comet/serde/maps.scala:154` | `createAndBinaryExpr` over `expr.left` and `expr.right` | `CometElementAt` had the same shape and was fixed in #5766 by declining a nullable nondeterministic operand (`arrays.scala:634-638`). That fix was deliberately scoped to `element_at`. These four were raised in [review of that PR](https://github.com/apache/datafusion-comet/pull/5766#discussion_r3952969992) and left alone; the comment now at `arrays.scala:686-689` cites them as precedent for the idiom. One difference from `element_at` matters. There the decline is gated on `expr.failOnError`, because the guard only exists to reproduce ANSI's short-circuit. These four use the guard for plain NULL propagation, with no ANSI gate, so the wrong answers are reachable in every configuration. ## Steps to reproduce A 16-row Parquet table, with the projection asserted to be native. Reproduced against `bc74cc7`, the merge base of #5766; the four call sites above are unchanged on `main` at `0d1348f`. ```sql SELECT _1, size(IF(monotonically_increasing_id() % 2 = 0, array(1), CAST(NULL AS ARRAY<INT>))) FROM tbl ``` Spark returns `1` for every row whose array is non-NULL. Comet returns `-1` on 5 of the 16 rows. `arrays_zip` over the same operand returns `[null,2]` where Spark returns `[1,2]`, and `map_from_arrays` returns `NULL` where Spark returns `Map(1 -> 2)`. `CometArrayAppend` is by inspection only. `ArrayAppend` is `RuntimeReplaceable` on Spark 4.x so the serde is unreachable there, but it is live on 3.4 and 3.5. ## Expected behavior Comet matches Spark, which evaluates the child once. ## Additional context The cheapest safe fix is the one #5766 applied to `element_at`: decline a nondeterministic child in `getSupportLevel`. Unlike `element_at`, none of these four is an ANSI short-circuit, so the guard cannot simply be dropped for a deterministic child — the native kernels do not reproduce Spark's NULL propagation on their own. `CometSize` has a cheaper option that keeps the coverage. `coalesce(size(x), <legacy sentinel>)` serializes the child once, and when `spark.sql.legacy.sizeOfNull` is off the `ELSE` branch is already a plain `null`, so it is worth checking whether the guard is needed at all in that configuration. Whatever shape the fix takes, the four are worth moving together so they do not drift apart from `CometElementAt`. -- 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]
