Skip to content

Isolate cached serializers between SQL test suites [fast-ut] [reduced-it] - #15488

Merged
thirtiseven merged 1 commit into
NVIDIA:mainfrom
thirtiseven:fix/statistics-cache-serializer-isolation
Aug 3, 2026
Merged

Isolate cached serializers between SQL test suites [fast-ut] [reduced-it]#15488
thirtiseven merged 1 commit into
NVIDIA:mainfrom
thirtiseven:fix/statistics-cache-serializer-isolation

Conversation

@thirtiseven

@thirtiseven thirtiseven commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Fixes #15464.

Description

RapidsStatisticsCollectionSuite selects Spark's DefaultCachedBatchSerializer, while other RAPIDS SQL suites use ParquetCachedBatchSerializer. Spark caches the first configured serializer in a JVM-global InMemoryRelation singleton. The standard serial ScalaTest path reused that state between suites, so a preceding cache suite could cause SPARK-33687: analyze all tables in a specific database to report a size of 161 bytes instead of the expected 4 bytes.

The fast parallel unit test runner already cleared this state between suites, which made serial and parallel runs behave differently. This change:

  • moves the existing reflective serializer reset into a shared test helper;
  • resets the serializer before and after every RAPIDS SQL suite, including failure cleanup; and
  • keeps the fast unit test worker cleanup as a defensive boundary using the same helper.

There is no user-facing behavior change.

Validation:

  • Before the fix, running RapidsCachedTableSuite followed by RapidsStatisticsCollectionSuite serially reproduced the issue: 107 succeeded and 1 failed.
  • The same serial suite order passes: 108 succeeded, 0 failed.
  • With -Drapids.parallelUnitTests=true -DparallelForkCount=2, the suites pass in separate child JVMs: 73/73 and 35/35.
  • The repository scalastyle check processed 1,731 files with 0 errors and 0 warnings.

Checklists

Documentation

  • Updated for new or modified user-facing features or behaviors
  • No user-facing change

Testing

  • Added or modified tests to cover new code paths
  • Covered by existing tests
    (Please provide the names of the existing tests in the PR description.)
  • Not required

Performance

  • Tests ran and results are added in the PR description
  • Issue filed with a link in the PR description
  • Not required

Signed-off-by: Haoyang Li <haoyangl@nvidia.com>
@thirtiseven thirtiseven self-assigned this Aug 3, 2026
@thirtiseven
thirtiseven requested a review from res-life August 3, 2026 02:55
@thirtiseven
thirtiseven marked this pull request as ready for review August 3, 2026 02:55
@thirtiseven thirtiseven changed the title Isolate cached serializers between SQL test suites [fast-ut] Isolate cached serializers between SQL test suites [fast-ut] [reduced-it] Aug 3, 2026
@greptile-apps

greptile-apps Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR centralizes Spark’s cached-batch serializer reset and applies it at RAPIDS SQL suite boundaries so serial and parallel test execution isolate JVM-global serializer state consistently.

  • Moves the reflective reset from ParallelUnitTestRunner into TestUtils.
  • Clears the serializer before SQL suite initialization and after teardown, including teardown failures.
  • Retains worker-level cleanup as a defensive boundary.

Confidence Score: 5/5

The PR appears safe to merge, with serializer state consistently reset at suite and worker boundaries.

The shared helper preserves the existing worker cleanup and adds correctly ordered, exception-safe suite cleanup without exposing a concurrent same-JVM suite path.

Important Files Changed

Filename Overview
tests/src/test/scala/com/nvidia/spark/rapids/ParallelUnitTestRunner.scala Replaces the runner-local serializer reset with the shared helper while preserving worker cleanup behavior.
tests/src/test/scala/com/nvidia/spark/rapids/TestUtils.scala Adds the shared reflective helper for resetting Spark’s JVM-global cached serializer.
tests/src/test/spark330/scala/org/apache/spark/sql/rapids/utils/RapidsSQLTestsBaseTrait.scala Resets serializer state before suite setup and after teardown, with failure-safe cleanup.

Sequence Diagram

sequenceDiagram
    participant Runner as ScalaTest Runner
    participant Suite as RAPIDS SQL Suite
    participant Cache as InMemoryRelation Singleton
    participant Spark as SparkSession
    Runner->>Suite: beforeAll()
    Suite->>Cache: clearSerializer()
    Suite->>Spark: super.beforeAll()
    Runner->>Suite: run tests
    Runner->>Suite: afterAll()
    Suite->>Spark: super.afterAll()
    Suite->>Cache: clearSerializer() in finally
    Runner->>Cache: defensive worker cleanup
Loading

Reviews (1): Last reviewed commit: "Isolate cached serializers between SQL t..." | Re-trigger Greptile

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes intermittent Spark SQL suite failures caused by Spark caching the first configured CachedBatchSerializer in a JVM-global singleton, which allowed earlier suites to influence later suites’ cache-serialization behavior in serial test runs.

Changes:

  • Centralizes the reflective reset of Spark’s cached cache-serializer into TestUtils.
  • Clears the cached serializer at RAPIDS SQL suite boundaries (before/after suite execution).
  • Reuses the shared helper from the fast parallel unit test worker cleanup.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
tests/src/test/spark330/scala/org/apache/spark/sql/rapids/utils/RapidsSQLTestsBaseTrait.scala Adds suite-boundary serializer resets to isolate Spark SQL suite state in serial runs.
tests/src/test/scala/com/nvidia/spark/rapids/TestUtils.scala Introduces a shared helper to reflectively clear Spark’s JVM-global cached serializer.
tests/src/test/scala/com/nvidia/spark/rapids/ParallelUnitTestRunner.scala Switches worker cleanup to call the shared serializer-reset helper.

@res-life res-life left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@thirtiseven

Copy link
Copy Markdown
Collaborator Author

build

@thirtiseven
thirtiseven merged commit de2d031 into NVIDIA:main Aug 3, 2026
60 of 61 checks passed
@thirtiseven
thirtiseven deleted the fix/statistics-cache-serializer-isolation branch August 3, 2026 05:24
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.

[BUG] RapidsStatisticsCollectionSuite intermittently reports an incorrect sizeInBytes after analyzing all tables in a database

4 participants