sunchao commented on code in PR #5452:
URL: https://github.com/apache/datafusion-comet/pull/5452#discussion_r3874895480
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -634,6 +651,42 @@ object CometElementAt extends
CometExpressionSerde[ElementAt] {
None
}
}
+
+ // Spark's ElementAt is a BinaryExpression: for a NULL map/array it
returns NULL WITHOUT
+ // evaluating the key/index child. Native scalar functions evaluate the
key eagerly over the
+ // whole batch, so under ANSI (failOnError) a throwing key (e.g. a
divide-by-zero) fires even on
+ // rows whose map/array is NULL, where Spark short-circuits. When the left
can actually be NULL,
+ // guard the lookup with CASE WHEN left IS NOT NULL THEN <lookup> ELSE
null so the key is only
+ // evaluated on the selected rows (DataFusion's CaseExpr filters the batch
before the THEN
+ // branch), reproducing the short-circuit. Mirrors the CASE-WHEN idiom in
CometArrayAppend /
+ // CometSize; the ELSE null literal carries the result type, as in
CometArraysZip.
+ if (expr.failOnError && expr.left.nullable) {
+ val isNotNullExpr = createUnaryExpr(
+ expr,
+ expr.left,
Review Comment:
[P2] Evaluate a stateful lookup operand only once
`expr.left` was already serialized into `baseExpr`, so serializing it again
here creates a second native instance of stateful descendants. CASE evaluates
this condition over the full batch, but evaluates the lookup only over selected
rows. With ANSI enabled and ordered INT `id = [1, 2]` in the first batch of
partition 0:
```sql
SELECT element_at(
element_at(
array(CAST(NULL AS ARRAY<BIGINT>),
array(monotonically_increasing_id(), CAST(NULL AS BIGINT))),
id),
id - 1)
FROM t
```
The source-derived results are `[NULL, 1]` for Spark and the previous
serializer, but `[NULL, 0]` here: only `id = 2` reaches the independent
THEN-side counter, which starts at zero. Spark's Project optimizer does not
hoist this nested nondeterminism, and native planning/projection does not
memoize the two serialized instances. The NULL first row also short-circuits
before index-zero validation in the previous native path, so that control does
not throw.
This is source-verified, not a fresh runtime reproduction. Please
materialize the left operand once, or retain a safe evaluation path when this
guard would duplicate stateful descendants.
--
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]
