apache / apache/datafusion-comet
size, arrays_zip, map_from_arrays and array_append return wrong answers for a nondeterministic child
- Dominant language
- Scala
- Stars
- 1.3k
- Forks
- 373
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 198
Description
## Describe the bug
Four serdes build a `CASE WHEN IS NOT NULL THEN ELSE 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))) 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), )` 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`.
Contributor guide
Research direction
Start at the four serde call sites in arrays.scala:54 and 765/844 and maps.scala:154, then compare their handling with CometElementAt at arrays.scala:634-638 and the precedent comment at 686-689. Run the supplied 16-row Parquet SQL reproductions, and verify that each operation matches Spark without serializing a nondeterministic child twice, including ArrayAppend on Spark 3.4/3.5.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- scala, spark
- Domain
- data-engineering
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 58/100