Skip to content

test: deduplicate Throwable cause-chain traversal - #5441

Open
peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:test/deduplicate-throwable-cause-chain
Open

test: deduplicate Throwable cause-chain traversal#5441
peterxcli wants to merge 1 commit into
apache:mainfrom
peterxcli:test/deduplicate-throwable-cause-chain

Conversation

@peterxcli

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #5223.

Rationale for this change

Several Comet test suites independently walk Throwable cause chains. Centralizing the common traversal keeps null termination and outer-to-inner ordering consistent while preserving suite-specific exception policies.

What changes are included in this PR?

  • Add a protected causeChain helper to CometTestBase.
  • Migrate equivalent full-chain traversal in the temporal, regexp, Parquet, execution, and task-metrics suites.
  • Leave bounded, root-only, one-level, and cycle-safe traversals unchanged.

How are these changes tested?

  • ./mvnw spotless:check
  • make core
  • Spark 4.1 / Scala 2.13 focused run: 6 tests across 5 suites passed.
  • Spark 3.4 / Scala 2.12 compatibility run: all changed test sources compiled and the focused ParquetReadV1Suite regression passed (1/1).

@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.

Summary

Approve this test-only refactor at a534294f91074c87c5d5a0be74952f2725b9cfd9, reviewed against bba47448ecddca5d6bf62e2ca843b64703cf734d. Five independent review scopes were completed before reconciliation, covering semantics, compatibility and adversarial cases, routing and resource safety, tests/build/CI, and discussion deduplication. No verified [P1] or [P2] regression remains in this patch; this is a code-review approval, not a claim that the current JVM CI is green.

Prior state and problem

The affected tests independently followed Throwable.getCause to inspect structured errors, exception types, or messages. The temporal suite already had the proposed helper locally, while the regexp, execution, Parquet, and task-metrics tests repeated equivalent full-chain walks. Consolidating traversal reduces duplication without moving each suite's distinct exception-selection policy into a universal comparator.

Design approach

The PR moves the temporal helper's implementation into a protected CometTestBase.causeChain(error: Throwable): Seq[Throwable] method. It then replaces the repeated walks with calls to that inherited helper, leaving type filtering, message projection, assertions, and deepest structured-error selection in their existing callers. The patch is confined to six test-source files; production operators, planner rules, configuration, dependencies, and workflows are unchanged.

Correctness / compatibility analysis

For the ordinary finite cause chains these tests inspect, the helper retains the outer exception, follows causes in order, and excludes the null terminator. The temporal selector still chooses the deepest matching SparkThrowable, regexp assertions still require the original Spark exception and exclude CometNativeException, and the task-metrics projection still drops null messages. Exact source comparisons accounted for every changed byte using only the intended extraction and substitutions.

Independent validation included 2,094 semantic checks over 131 finite chains, plus 148 helper-level assertions compiled and run across Scala 2.12.17, 2.12.18, 2.13.16, 2.13.17, and 2.13.18. The latter used JDK 11 for Scala 2.12 and JDK 17 for Scala 2.13; the separate semantic probe used Scala 2.13.16 and the cached Spark 4.0.4 SparkThrowable interface. All six standalone probe executions were rerun during reconciliation and passed. Inherited-member checks found no causeChain conflict in the inspected Spark dependency ancestry. The repository's read-only suite-registration check also passed, with 184 registration entries. These are focused helper/source checks, not full Comet JVM-suite runs.

Current captured CI has 31 failed, 12 successful, and 10 skipped checks. Thirty failures report CometAggregateSuite.scala:318 referencing the absent CometConf.COMET_EXEC_SHUFFLE_ENABLED; both implicated files are byte-identical in the pinned base and head, so that diagnostic predates this patch. See the Spark 4.1 expressions job and the reference already present in the base. The Delta gate separately fails at a compiled-class stage whose Maven output is suppressed; its exact internal error was not verifiable and is not asserted to have the same cause.

The Rust job actually passed 899 tests, with 4 skipped, but does not exercise the changed Scala helper. The green Spark 4.1 build explicitly skips test compilation and execution, and the affected JVM suites did not execute in the failing matrix jobs. The PR author's reported focused Spark 4.1 and Spark 3.4 runs were not independently reproduced; no full local JVM/native suite was run for this review.

Key design decisions

Keeping this method protected and test-only gives the existing suites a common traversal without adding a production API or shared mutable state. Keeping deepestSparkThrowable local preserves its specific policy, and preserving Option(error.getMessage) retains the metrics test's null-message handling. Bounded, root-only, one-level, and cycle-safe traversals remain untouched because they do not share the same contract.

Implementation sketch

The shared implementation starts an iterator at the supplied throwable, repeatedly calls getCause, stops before null, and exposes the result as a sequence. The temporal suite inherits that implementation; regexp and Parquet tests inspect the sequence's types, the two execution tests map it to class/message text, and the task-metrics test filters and inspects messages. All calls remain after their existing intercepted actions, with query construction, native-plan checks, file setup, job-group cleanup, and listener removal unchanged.

Behavioral changes worth calling out

Scala 2.12's iterator toSeq produces a lazy Stream, whereas the tested Scala 2.13 versions produce a strict list. This is not a blanket eager/lazy equivalence: the regexp case already requires Spark 4.0+, and both changed execution-suite callers immediately force their message sequence with mkString; temporal and Parquet already used the same toSeq implementation. Synthetic mutable or side-effecting throwables expose evaluation-order differences, but no reachable regression in these callers was established. Cycle detection is absent both before and after the extraction; this PR does not broaden the intentionally different cycle-safe helpers' contract.

Suggested improvements

No patch-specific code revision is requested from this review. Build validation still needs a green JVM result after the preexisting missing-configuration reference is addressed, and the Delta gate's hidden failure needs observable diagnostics before its cause can be certified. Those are explicit validation limitations rather than new inline findings against this extraction; no workflows were rerun or approved as part of the review.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deduplicate Throwable cause-chain traversal in Comet tests

2 participants