apache / apache/datafusion-comet

size, arrays_zip, map_from_arrays and array_append return wrong answers for a nondeterministic child

Open
#5,781 0 comments 0 reactions 0 assignees View on GitHub
area:expressions array expressions bug correctness map expressions requires-triage
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.