fix: evaluate the array_append item on rows whose array is NULL

[kazuyukitanimura]

Which issue does this PR close?

Closes #6086.

Rationale for this change

CometArrayAppend reproduces Spark's NULL propagation with CASE WHEN IS NOT NULL THEN array_append(, ) 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 (size, arrays_zip, map_from_arrays and array_append return wrong answers for a nondeterministic child #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.

[andygrove]

Thanks for tracking this down and for the clear write-up. Evaluating the item in the ELSE branch is a neat way to keep the native path, and the control query in the fixture makes the new test meaningful.

I have a few concerns to resolve before this merges.

Overlap with #5867. That PR is approved and mergeable, and it rewrites CometArrayAppend too. It declines any nondeterministic child and routes ANSI plus a nullable array through the JVM codegen dispatcher as Incompatible. It also adds array_append_ansi_null_array.sql with the same repro as #6086. The two approaches differ, so could we agree which one wins before either merges? If this lands second it needs a rebase over a rewritten serde. If #5867 lands first, is the native fix here still reachable, or only under allowIncompatible?

Nondeterministic items and nullability. getSupportLevel only declines a nondeterministic item when the array is nullable. For a non-nullable array it relies on DataFusion skipping the filter when the guard matches every row, which #5867 deliberately avoids leaning on. Could the comment say why that is safe, or could the decline ignore nullability? Should a nondeterministic nullable array on the left be declined here as well? It is serialized in both the guard and THEN, so it drifts the same way.

Docs. getSupportLevel returns Unsupported(Some(...)), but getUnsupportedReasons() isn't overridden. Could you add it so the generated compatibility page lists the fallback?

Tests. Would you add a few cases to array_append_eager_eval.sql? The first would be a nondeterministic item with a non-nullable array that stays native with expect_native, since the PR relies on that. The second would be tables where every array is NULL and where none is, so each batch takes only one branch. The third would be a struct or array item that goes through widenElementInLockstep.

Verification. The fixture is MaxSparkVersion: 3.5, and the PR tier only runs the default Spark profile, so CI never runs it. Could you run CometSqlFileTestSuite array_append on the 3.5 profile, or apply the Spark 3.5 run-* label, and post the result?

On behavior against branch-1.1, the item is now evaluated on rows with a NULL array, which fixes the ANSI divergence but adds work on that path. A nondeterministic item with a nullable array now falls back. I read both as intended. I found nothing else that differs from the release.

[kazuyukitanimura]

Thanks for the careful review. @andygrove I've pushed changes for the code, docs and test points. Two items need a decision or a test run first.

Overlap with #5867. I agree we should settle this before either merges. The two PRs make different trade-offs:

• fix: dispatch map lookups with normalized keys and nondeterministic null-guarded children #5867 sends ANSI plus a nullable array through the JVM codegen dispatcher as Incompatible, so the native guard only runs under allowIncompatible.
• This PR keeps the native path for every item and makes it correct, by evaluating the item in the ELSE branch. It costs one extra evaluation of the item on rows whose array is NULL, and none when the item is a literal or column read.

If #5867 lands first, the native fix is reachable only under allowIncompatible, and this PR would need a rebase over the rewritten serde. If this lands first, #5867 would need to drop its ANSI Incompatible routing for array_append, and its array_append_ansi_null_array.sql repro would pass natively. I have no strong preference.

Nondeterministic operands and nullability.

• I now decline when the array is nullable and either child is nondeterministic, so a nondeterministic array on the left is covered too. It is serialized in the guard and in THEN, so it drifted the same way.
• For a non-nullable array, I changed convert to emit no CASE at all. Each operand is evaluated once over the whole batch, in order, so this no longer depends on DataFusion skipping the filter. Unlike fix: dispatch map lookups with normalized keys and nondeterministic null-guarded children #5867, a nondeterministic item with a non-nullable array therefore stays native. If you'd rather match fix: dispatch map lookups with normalized keys and nondeterministic null-guarded children #5867 and decline regardless of nullability, that is a one-line change.

Docs. I added getUnsupportedReasons() for the nondeterministic fallback, and updated the array_append audit note.

Tests. array_append_eager_eval.sql now also covers:

• a nondeterministic item and a nondeterministic array, both falling back;
• a nondeterministic item with a non-nullable array, via expect_native;
• a throwing item on a non-nullable array;
• a table where every array is NULL and one where none is, each with a throwing and a non-throwing item;
• nested array and map items that go through widenElementInLockstep.

Verification. I'll apply the Spark 3.5 run-* label or run it myself and post the result here. The new expectations haven't been executed yet, so a few may need adjusting. The nested-map expect_error case is the one I'm least sure of.

branch-1.1. Thanks for checking. I agree with your reading of both behavior changes.

[andygrove]

Thanks for the follow-up changes. Dropping the guard for a non-nullable array and the extra nondeterministic cases are good additions, and the fixture covers the right shapes. I ran the fixture locally on the 3.5 profile at dc74c67d22 and it fails, so I'm requesting changes.

The new ELSE branch has the wrong type. A nullable array with any item that is not a literal or a column now fails natively. A minimal case with no ANSI is array_append(IF(_1 % 2 = 0, array(1), CAST(NULL AS ARRAY<INT>)), _1 + 1). It aborts with column types must match schema types, expected List(Int32) but found Int32 at column index 1. The inner CASE WHEN item IS NOT NULL THEN nullLit ELSE nullLit uses nullLiteralProto, which is typed elementType, so it produces an element-typed array that the outer CASE can't merge with the list result. The same error hides the DIVIDE_BY_ZERO expectation in the fixture, so the expect_error queries on the nullable-array path were not testing anything yet. This query works on branch-1.1, so it is a regression against the release. Could the inner CASE use an array-typed NULL literal, ArrayType(elementType, containsNull = true), and could the outer ELSE match it?

Verification. The fixture is MaxSparkVersion: 3.5 and the PR tier only builds the default profile, so CI has not run it. After the fix, could you run CometSqlFileTestSuite array_append under -Pspark-3.5 and post the result? The first failing query stops the file, so I could not see whether the nested map case and the array() insert behave as you expect.

Unrelated CI change. The ci: purge POM-only Maven entries that declare no packaging commit touches .github/actions/setup-spark-builder/action.yaml and dev/local-ci.sh. It looks unrelated to array_append, and #6947 appears to cover the same purge. This fix is meant for backport-1.1, so could we drop that commit from this PR and land it separately?

Overlap with #5867. That PR is approved and also rewrites CometArrayAppend. Could you and @andygrove settle which one lands first and record the decision on both PRs? Whichever goes second needs a rebase and may need to drop or adjust its array_append handling.

Cost on the non-ANSI path. The item is now evaluated on NULL-array rows even when ANSI is off, which is the default on 3.4 and 3.5. Did you measure what that costs for a typical item such as a + 1? If it is noticeable, is there a cheap way to skip the extra evaluation when the item cannot throw?