test: restore ANSI array access error coverage

[rich7420]

Which issue does this PR close?

Part of #5076.

Rationale for this change

The ANSI get_array_item and element_at SQL fixtures still skip ten error cases for #3375, although the error-message fix has merged.

What changes are included in this PR?

Restore error checks for out-of-bounds indices and element_at's zero index. Add valid boundary queries that check Comet execution and result parity.

The literal-array cases use column indices so Spark's SimplifyExtractValueOps cannot replace an invalid lookup with NULL when constant folding is disabled.

How are these changes tested?

Both SQL fixtures passed locally on Spark 4.1.3 / JDK 21 after building the native library. This covers the ten restored error cases, valid boundary queries and existing NULL short-circuit tests.

Fork CI completed with 44 successful checks, including the Linux and macOS expressions jobs.

[sunchao]

Correctness

Reviewed HEAD f25e0145 against authoritative base 5627ab8c (authored diff from merge-base 17f54da8). No P1/P2 issue found. The change restores ten ANSI error queries that were still ignored for #3375: four bracket-access cases, four out-of-range element_at cases, and two zero-index element_at cases. It also adds two valid queries covering six boundary positions. The existing ElementAt NULL short-circuit and nondeterministic-fallback queries remain intact.

The supplied indices match maintained Spark 3.5/4.0 semantics: bracket access is zero-based and rejects negative indices. element_at is one-based, accepts negative positions from the end, and rejects zero. Comet's existing ListExtract paths and JVM converters map these cases to the expected Spark error identifiers. The file-level ANSI setting overrides the harness default. The normal query directives check both answers and native operators. expect_error requires both executions to throw and checks a message substring, so this does not establish complete exception-type, parameter, query-context, or per-error native-plan equivalence.

I verified both changed SQL files passing in the author's Spark 3.5/Linux, Spark 4.0/Linux, and Spark 4.0/macOS logs. Their checkout tree equals this HEAD, with matching logged native-library producer/consumer identities. Those fork results cover the HEAD tree, not the newer authoritative base. Upstream Spark 4.2 expressions also passed both files on the current base/head merge. The upstream native build and Rust tests passed there. At 2026-09-09T14:24:36.212380+00:00, upstream checks were 40 successful, 10 skipped and 1 queued. This is not an all-green CI claim. No local product test was run. Maintained Spark 3.4/4.1 sources were unavailable, so I make no compatibility claim for those versions.

Performance

There is no production execution change. Test work increases by ten restored error queries and two valid queries over one-row fixtures. Each uses the existing Spark/Comet comparison path. The two integer columns are confined to the bracket-access fixture. This is bounded regression coverage, and neither a runtime speedup nor a benchmark result is claimed.

Design

The implementation keeps the cases in the existing ANSI array fixtures and pairs negative checks with successful boundary checks. That combination detects a disabled error path while also checking native execution on valid input. Using persisted index columns for literal-array bracket access is justified: the maintained Spark 3.5 simplification rule can replace an invalid CreateArray/IntegerLiteral lookup with NULL even when ConstantFolding is excluded. A column index avoids that rule. The inspected rule does not match ElementAt, so its literal-index cases can remain direct.

The newer base does not overlap the two authored fixtures. I checked its JNI exception-classification interaction: the new typed walker recognizes shuffle-capacity and illegal-state exceptions, while the structured Spark array-error route remains intact. The current merge adds exactly this authored patch to the base. No before-merge design change is needed.

Abstraction & complexity

The change reuses the established SQL-file directives and introduces no test helper, production API, version branch, or new abstraction. The explicit boundary queries make zero-based and one-based behavior easy to inspect, and the two fixture columns solve the optimizer issue locally without changing global test configuration. The scope is appropriate for restoring this coverage. I found no abstraction or complexity issue requiring a change.

[andygrove]

Thanks for picking this up, these two files have been stale for a while.

I checked out the branch, built the native library and ran both fixtures on Spark 4.1.3 with JDK 17. They pass. I also dumped the executed plan for every query in both files to make sure they genuinely reach CometProject rather than quietly falling back, and they all do, including the three literal-array element_at cases.

Your SimplifyExtractValueOps reasoning holds up. I read ComplexTypes.scala on 3.4.3, 3.5.9, 4.0.4 and 4.1.3, and in every one of them the GetArrayItem(CreateArray(...), IntegerLiteral) case rewrites an out-of-range lookup to a null literal with no ANSI guard, so the old SELECT array(1, 2, 3)[5] could never have thrown in Spark. There is no equivalent rule for ElementAt, so leaving those literal cases direct is correct.

I left three things inline that I would like to see addressed before this merges. I applied all of them locally and both fixtures still pass.

Two things I turned up while poking around the same code paths, neither of which belongs in this PR. element_at(arr, -2147483648) under ANSI is broken in both engines. Comet's debug build panics with attempt to negate with overflow out of index.abs() in one_based_index, and Spark is worse, because math.abs(Int.MinValue) stays negative so its bounds check passes and the vectorized reader then reads unmapped memory. I got a reproducible JVM SIGSEGV in ColumnarArray.isNullAt. There is no Spark behavior to match there so I would not add coverage for it, but the native panic probably deserves its own issue. Separately, the note at array_insert.sql:162 is still accurate rather than stale. Spark raises INVALID_INDEX_OF_ZERO for array_insert(arr, 0, x) and Comet raises Position for array_insert..., in both ANSI and legacy mode, and CometArrayInsert is marked Compatible() with no getIncompatibleReasons. I could not find a tracking issue for that one.

[andygrove]

Nice catch on SimplifyExtractValueOps. I checked ComplexTypes.scala on 3.4 through 4.1 and the rule really does fold an out-of-range CreateArray lookup to a null literal with no ANSI guard, so the column index is the only way these cases can throw at all.

On the pattern itself though, expect_error is a substring match on the message, and INVALID_ARRAY_INDEX happens to be a prefix of INVALID_ARRAY_INDEX_IN_ELEMENT_AT. That means all four of these would still pass if the bracket path ever started raising the element_at error class, which is exactly the confusion this file exists to rule out. Could we use expect_error([INVALID_ARRAY_INDEX]) instead? Both engines render the bracketed form, so that pins the class exactly. Worth doing the same in element_at_ansi.sql for consistency.

[rich7420]

Nice catch! thanks for that

[andygrove]

Adding these valid boundary queries is the right instinct given expect_error skips the operator check.

Since you are pinning boundaries anyway, would it be worth pairing them with the first invalid index too? arr[3] is the one that would catch an off-by-one in zero_based_index, and arr[10] would sail straight past it. I tried it locally and it behaves correctly today, so this is purely locking in what already works.

[rich7420]

ok , I will address it

[andygrove]

Same thought as on the get_array_item file. Would it be worth pairing the valid boundaries with the first invalid index rather than only 10 and -10? element_at(arr, 4) and element_at(arr, -4) are where an off-by-one in one_based_index in native/spark-expr/src/array_funcs/list_extract.rs would actually surface, and the far out of range values would not catch it. Both behave correctly today, so this is just locking in what already works.

One more line while you are in here would be useful. SELECT element_at(arr, CAST(NULL AS INT)) FROM ansi_element_at_oob returns NULL rather than throwing even under ANSI, and nothing in this file guards that branch.

[andygrove]

These three literal-array cases (this one, line 63 and line 75) have no FROM, so they plan as CometProject over CometSparkRowToColumnar over Scan OneRowRelation. That only reaches Comet because CometTestBase sets spark.comet.sparkToColumnar.enabled to true, and the product default for that config is false. Since expect_error does not assert native execution the way query does, if that test base setting ever changed these three would quietly become Spark vs Spark and keep passing.

Would you mind adding FROM ansi_element_at_oob to them? The table has a single row so the expected error is unchanged, and it puts them over a real Comet scan like everything else in the file. It also matches what you already did on the get_array_item side.

[rich7420]

Ok, I'll add to them

[sunchao]

Re-reviewed a4b81b77 since f25e0145. No new or remaining P1/P2 findings. The update addresses the four review threads: bracketed error identifiers prevent the prefix mismatch, arr[3] and element_at(arr, ±4) cover the first invalid positions, the NULL-index query is present, and all three literal-array element_at errors now read from the table. Existing NULL short-circuit and nondeterministic-fallback coverage is unchanged.

Both updated fixtures passed in Spark 3.5/Linux, Spark 4.0/Linux and Spark 4.0/macOS. These jobs executed merge 8a8bff0 (2d3eca21 + this head), with matching native-library producer/consumer checkout and archive digests. The current merge e7a18112 retains both fixture blobs but includes newer base changes, so these results do not establish execution of that newer merge.

At 2026-09-11T06:25:57.955105+00:00, checks were 44 successful, 10 skipped and 1 failed. The Spark 4.0 shuffle job timed out downloading the eventstream:1.0.1 dependency descriptor before its tests ran. No local product tests were run. Canonical source checks used maintained Spark 3.5/4.0 only. Maintained 3.4/4.1 sources remain unavailable.

[rich7420]

@andygrove could you take another look please

[andygrove]

Thanks for pairing the valid boundaries with the first invalid index and for moving the literal-array cases onto a real scan. That covers everything I raised. The one red check is the Spark 4.0 shuffle job, which does not run CometSqlFileTestSuite, so it is unrelated to this change. LGTM.