sunchao commented on code in PR #5766:
URL: https://github.com/apache/datafusion-comet/pull/5766#discussion_r3954338855
##########
spark/src/main/scala/org/apache/comet/serde/arrays.scala:
##########
@@ -690,7 +667,12 @@ object CometElementAt extends
CometExpressionSerde[ElementAt] {
// 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) {
+ //
+ // The guard serializes `left` a second time and runs the THEN branch over
a different row
+ // selection, so a stateful operand (`rand()`,
`monotonically_increasing_id()`) would advance
+ // its state twice and silently move values and NULLs. Restrict it to
deterministic operands;
+ // the rest keep the pre-existing eager-key behaviour.
+ if (expr.failOnError && expr.left.nullable && expr.left.deterministic) {
Review Comment:
### Correctness
#### [P2] Preserve NULL short-circuiting for nondeterministic operands
Skipping the CASE guard still admits these expressions natively, but
`ListExtract.evaluate` evaluates the ordinal over the entire batch before
inspecting the array's nulls. Spark's `ElementAt` evaluates its right child
only after a non-NULL left child. Under ANSI, a nullable nondeterministic array
can therefore return NULL in Spark while Comet raises an exception from the
index.
For a concrete regression case, use a Parquet table with non-NULL INT
columns `id` and `z`, where `z` is zero, and project `element_at(IF(rand(7L) <
2, CAST(NULL AS ARRAY<INT>), array(1)), id % z)`. The IF remains
nondeterministic but always returns NULL. The previous guard skips its THEN
branch, while the new unguarded path evaluates the remainder by zero. This is a
source-derived case, not a locally executed result. The added tests exercise a
nondeterministic left with a safe index and a deterministic left with a
throwing index separately, so they do not cover this combination.
Please decline the whole lookup to Spark, or dispatch the whole expression,
for a nullable nondeterministic left until the native path can evaluate the
left once and mask the index evaluation. Restoring the duplicated operand guard
would reintroduce the state drift this PR fixes. Add a combined regression case
that asserts both the answer and the selected execution path.
--
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]
