Skip to content

test: strengthen ANSI exception assertions - #5800

Merged
sunchao merged 1 commit into
apache:mainfrom
rich7420:test/5076-ansi-error-assertions
Sep 10, 2026
Merged

sunchao merged 1 commit into
apache:mainfrom
rich7420:test/5076-ansi-error-assertions

Conversation

@rich7420

@rich7420 rich7420 commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #5076.

Rationale for this change

ANSI tests that only match message substrings can pass when Comet throws the wrong exception type.

What changes are included in this PR?

Check Comet execution and compare Spark exception types, error classes and SQLSTATE in nine existing array access, arithmetic and cast tests. Preserve the array and cast SQL-context checks.

Integral divide and decimal-to-decimal cast retain their existing checks with links to #5072. The stricter assertion reproduced the integral-divide exception-type mismatch locally.

How are these changes tested?

All 13 selected tests passed locally on Spark 4.1.3 / JDK 21: four array tests, eight arithmetic tests and one cast test.

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

@github-actions github-actions Bot added enhancement New feature or request test Testing related labels Sep 9, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correctness

Reviewed bfda2d07 against 5627ab8c. The prior assertions could accept a generic native exception carrying the expected words. The new checkSparkError helper verifies the Comet plan, requires both engines to fail, rejects any CometNativeException in the cause chain, and compares the deepest structured Spark exception's exact JVM class, expected error class and SQLSTATE. Selecting that underlying exception avoids comparing a task-level SparkException wrapper with the actual arithmetic, array-index or cast error. I found no P1/P2 issue in this change.

The nine updated cases exercise three array-index errors, integer add/subtract/multiply overflow, two division-by-zero cases and invalid string-to-date conversion. Each failing expression depends on a Parquet column, so constant folding cannot replace the expression being tested. The plan checker rejects an unconverted projection. These suites do not add Project to the Spark-to-columnar input allowlist. Both-success and one-sided-success results fail the new helper. Returning the converted exception also preserves the existing detailed array messages and the array/cast SQL-query-text assertions.

The maintained Spark 3.5 and 4.0 sources construct the corresponding SparkArithmeticException, SparkArrayIndexOutOfBoundsException, SparkRuntimeException and SparkDateTimeException. Spark 4.0 retains getErrorClass as an alias of getCondition, so the shared helper remains compatible with Spark 3.5's API. The Comet version shims construct those same exception types. This checks structured identity. It does not assert full arithmetic message or parameter equality.

The current Apache expression logs show all nine cases passing on Spark 3.4, Spark 3.5 and Spark 4.1. Spark 4.2 passes six. Add/subtract/multiply remain canceled by the preexisting #4142 assumptions. Those jobs checked out 5875eadd, whose parents are the assigned target base and reviewed head. All four authored files match that merge byte for byte. The complete trees differ because the target branch has advanced. The target-base JNI change adds typed IllegalState handling without changing these structured Spark-error paths.

The linked fork's macOS Spark 4.0 and Linux Spark 4.1 logs also pass all nine cases. They checked out 556e4fd8, a merge into the older ancestor whose complete tree equals this PR head. Those fork results do not qualify the newer target base. At 2026-09-09T14:28:56.201732+00:00, Apache checks were 40 successful, 10 skipped and 1 queued. The remaining queued check is macOS lint, so this is not an all-green CI claim. I did not run local runtime tests. The required maintained Spark 3.4/4.1 branches were unavailable, so CI coverage for those versions is separate from canonical-source verification.

Performance

Only test sources change, so this adds no cost to user queries. The three array tests and one cast test now also execute vanilla Spark, while the five arithmetic cases already ran both engines. The added plan walk and short exception-chain traversals are proportionate to checking native execution and typed failure parity. There is no new expression or performance claim requiring a benchmark here.

Design

One protected helper in CometTestBase keeps the failure contract consistent across the three suites and reuses the existing dual-engine runner and cause-chain utility. Keeping the array/cast context assertions at their call sites preserves their specific regression coverage. Integral divide and decimal-to-decimal overflow retain their previous assertions with explicit #5072 links. This is a bounded part of #5076, and leaves the other ANSI mismatches open.

Abstraction & complexity

The local structured-error extractor removes repeated unwrapping and parity checks without adding a configurable assertion framework or changing the existing permissive helper's callers. The returned typed exception serves the existing message/context checks directly. No additional restructuring is needed for these nine cases. The documented exception-conversion gaps can be addressed in their own fixes before their tests adopt the stricter helper.

@sunchao
sunchao merged commit df632c3 into apache:main Sep 10, 2026
56 checks passed
@sunchao

sunchao commented Sep 10, 2026

Copy link
Copy Markdown
Member

Merged, thanks @rich7420 !

@rich7420

Copy link
Copy Markdown
Contributor Author

Thanks for the review @sunchao

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request test Testing related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants