kazuyukitanimura opened a new pull request, #7025:
URL: https://github.com/apache/datafusion-comet/pull/7025
## Which issue does this PR close?
Closes #6086.
## Rationale for this change
CometArrayAppend reproduces Spark's NULL propagation with CASE WHEN
<array> IS NOT NULL THEN array_append(<array>, <item>) ELSE null END.
DataFusion's CaseExpr
evaluates the THEN branch only on the rows the guard selects, so the item
was never evaluated where the array is NULL.
Spark's ArrayAppend.doGenCode emits the item's code outside nullSafeExec.
It evaluates the item on every row and guards only the assignment of the
result. Under ANSI
mode, an item that raises on a row whose array is NULL (for example 1 /
(_1 - 1)) therefore raises in Spark but not in Comet. Comet returned [NULL] for
that row and
completed, so it answered a query that Spark fails.
ArrayAppend is RuntimeReplaceable on Spark 4.x (rewritten to
array_insert(-1)), so CometArrayAppend is only reachable on Spark 3.4 and 3.5.
## What changes are included in this PR?
- CometArrayAppend.convert now evaluates the item in the ELSE branch as
well, as CASE WHEN item IS NOT NULL THEN NULL ELSE NULL. Each row evaluates the
item exactly
once: in THEN where the array is non-NULL, and in ELSE where it is NULL.
This is option 1 from the issue, so the native path is kept for every item.
- No extra work when it isn't needed. The ELSE stays a plain NULL literal
when the item is a literal or column read (which cannot throw) or when the
array is
non-nullable.
- CometArrayAppend.getSupportLevel now returns Unsupported when the array
is nullable and the item is nondeterministic. The fix serializes the item
twice, and each
copy would see only a subset of the rows, so a stateful item like rand()
would silently move values. The query falls back to Spark, as CometElementAt
does for a
nondeterministic operand. This also covers the nondeterministic-item
half of the problem (#5781).
- New fixture array_append_eager_eval.sql, limited to Spark 3.4/3.5 (see
below).
- Audit note added to the array_append entry in
expression-audits/array_funcs.md.
I checked the sibling serdes the issue lists. The issue found
CometMapFromArrays, CometSize and CometArraysZip unaffected, and I made no
changes to them.
## How are these changes tested?
New SQL file test
spark/src/test/resources/sql-tests/expressions/array/array_append_eager_eval.sql
(MaxSparkVersion: 3.5, ANSI on):
- the control query, which confirms ANSI divide raises on its own;
- the repro from the issue, expecting DIVIDE_BY_ZERO where the array is
NULL;
- a throwing item on a row whose array is non-NULL, still expecting
DIVIDE_BY_ZERO;
- a non-throwing computed item, plus column and literal items, which must
stay native (expect_native(array_append));
- a nondeterministic item with a nullable array, which must fall back
(expect_fallback).
mvn test-compile passes, including spotless and scalastyle. I could not
run the Spark-based suites in my environment, so I have not run
CometSqlFileTestSuite against
this change. Before merging, someone needs to run it on Spark 3.4 or 3.5
(./mvnw test -Dsuites="org.apache.comet.CometSqlFileTestSuite array_append"
-Dtest=none)
and run the spark sql_core-1 shard or apply a run-* label.
Backport: the issue is labelled backport-1.1. The fix should be
cherry-picked with -x to the release branches once it lands.
--
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]
