[AutoSparkUT] Fix GPU missing-file recovery guidance [reduced-it] [fast-ut] [databricks] - #15835
[AutoSparkUT] Fix GPU missing-file recovery guidance [reduced-it] [fast-ut] [databricks]#15835wjxiz1992 wants to merge 8 commits into
Conversation
Signed-off-by: Allen Xu <allxu@nvidia.com>
Greptile SummaryThis PR preserves missing-file path context across asynchronous GPU readers and converts missing-file failures into Spark-version-appropriate recovery guidance or structured errors.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant Scan as GPU file scan
participant Async as Async reader
participant RDD as GpuDataSourceRDD
participant Shim as MissingFileErrorShim
participant User as Spark caller
Scan->>Async: Read planned file
Async-->>Async: FileNotFoundException
Async-->>RDD: Path-carrying failure
RDD->>Shim: Convert(path, original error, V1/V2 hint)
alt Spark 3.x
Shim-->>User: FileNotFoundException with recovery guidance
else Spark 4.x
Shim-->>User: FAILED_READ_FILE.FILE_NOT_EXIST with PATH
end
Reviews (8): Last reviewed commit: "Fix GpuDataSourceRDD import order" | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This pull request restores Spark-equivalent missing-file recovery guidance for GPU ORC scans when cached data files are deleted, aligning GPU error messaging with Spark’s V1/V2 expectations and re-enabling previously excluded upstream MetadataCache tests.
Changes:
- Add recovery-hint enrichment for
FileNotFoundException(includingExecutionException-wrapped cases) inGpuDataSourceRDD, with a switch to include the V1REFRESH TABLEhint. - Plumb
includeRefreshHint = truefrom V1 file-source scans (GpuFileSourceScanExec) intoGpuDataSourceRDD. - Re-enable the previously excluded Spark 3.3 MetadataCache suites and add RAPIDS-focused tests across COALESCING/MULTITHREADED ORC readers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/src/test/spark330/scala/org/apache/spark/sql/rapids/utils/RapidsTestSettings.scala | Re-enables the previously excluded MetadataCache tests now that GPU guidance is expected to match Spark. |
| tests/src/test/spark330/scala/org/apache/spark/sql/rapids/suites/RapidsMetadataCacheSuite.scala | Adds RAPIDS-specific tests asserting V1 vs V2 recovery-hint content across ORC reader modes. |
| sql-plugin/src/main/scala/org/apache/spark/sql/rapids/GpuFileSourceScanExec.scala | Passes includeRefreshHint = true for V1 file-source scans when constructing GpuDataSourceRDD. |
| sql-plugin/src/main/scala/com/nvidia/spark/rapids/shims/GpuDataSourceRDD.scala | Enriches missing-file exceptions with Spark-like recovery guidance, configurable for V1 vs V2 hint text. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
| } | ||
|
|
||
| override def next(): Object = { |
Performance impact: successful next() calls retain the existing iterator work and metrics-finally path; the added handlers allocate only when a missing-file exception is thrown. Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
| "You can explicitly invalidate the cache in Spark by " + | ||
| "recreating the Dataset/DataFrame involved." | ||
| } | ||
| val enrichedException = new FileNotFoundException(s"$message\n$recoveryHint") |
There was a problem hiding this comment.
Could we make this translation version-aware before merging? This source is shared by every supported Spark build, but starting with Spark 4.0 the CPU file readers translate missing files through FileDataSourceV2.attachFilePath into a SparkException with condition FAILED_READ_FILE.FILE_NOT_EXIST and a path parameter. MetadataCacheSuite now asserts that structured error.
This branch always returns a plain FileNotFoundException with the Spark 3.x message, while the only end-to-end coverage is spark330 and the shared unit test explicitly expects that raw type. As a result, Spark 4.x GPU scans would still diverge from CPU behavior. I think this needs a shimmed translation with the owning file path, plus representative 4.x V1/V2 coverage.
There was a problem hiding this comment.
As it's a SparkUT fix, I'm fine with doing it in a follow-up.
There was a problem hiding this comment.
Agreed. I’ll keep this SparkUT-focused PR scoped to Spark 3.x recovery guidance and handle Spark 4.x structured-error parity separately.
There was a problem hiding this comment.
ok, please file an issue to track if so.
Performance impact: compile-time-only API tightening; runtime policy values and exception-path behavior are unchanged. Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
|
build |
1 similar comment
|
build |
|
|
||
| Seq(("V1", true, "orc"), ("V2", false, "")).foreach { | ||
| case (sourceName, useV1, v1Sources) => | ||
| Seq(RapidsReaderType.COALESCING, RapidsReaderType.MULTITHREADED).foreach { readerType => |
There was a problem hiding this comment.
Could we add a V1 PERFILE case here and route that path through MissingFileErrorShim? On 400db173, GpuFileSourceScanExec sends PERFILE reads through Spark321PlusDBShims.getFileScanRDD, which returns the copied GpuFileScanRDD. That RDD still produces a plain Spark 3-style FileNotFoundException, so DBR 17.3 V1 PERFILE scans will not report FAILED_READ_FILE.FILE_NOT_EXIST.
| val runner = getBatchRunner(tc, file, outLocal, blocks, offset, batchContext) | ||
| runner.addFailureTransformer { | ||
| case error: FileNotFoundException => | ||
| GpuFileNotFoundException(file.toString, error) |
There was a problem hiding this comment.
file is a Hadoop Path here. For a local URI, Path.toString normalizes file:///tmp/x to file:/tmp/x, while Spark's structured error and the new test use the full URI. If the failure surfaces during the asynchronous block copy, the GPU path parameter can therefore differ from CPU. Could we carry the original URI or use file.toUri.toString, and cover this failure point?
| createReader: => PartitionReader[T]): PartitionReader[T] = { | ||
| val reader = withStructuredMissingFile(partition)(createReader) | ||
| new PartitionReader[T] { | ||
| override def next(): Boolean = withStructuredMissingFile(partition)(reader.next()) |
There was a problem hiding this comment.
This is now on the successful Spark 4/5 read path: every next() and get() goes through this wrapper and the call-by-name helper, rather than adding work only when an exception occurs. That makes the current “cold exception path / Performance: Not required” justification incomplete. Could we avoid the call-by-name helper on these hot calls and update the performance rationale with focused evidence, or provide focused performance validation?
Signed-off-by: Allen Xu <allxu@nvidia.com>
Resolve the GpuDataSourceRDD conflict by preserving the missing-file conversion while adopting the upstream reader lifecycle and custom metrics integration. Performance impact: the merge adds only exception-path conversion around hasNext/next and one input-partition reference update per reader; the successful row/batch path retains upstream ReaderIterator metric and close behavior. Signed-off-by: Allen Xu <allxu@nvidia.com>
Signed-off-by: Allen Xu <allxu@nvidia.com>
JaCoCo production line coverage: independently measured, not additive across incompatible shim bytecode tuples (
sql-plugin +56, Spark 3.3/Scala 2.12;sql-plugin +45, Spark 4.0/Scala 2.13; measured headad6d7a5a; current head844a13f7is a no-conflict main merge with the 19-file PR diff unchanged)Fixes #15511.
Description
GPU file scans can surface a raw
FileNotFoundExceptionafter a cached data file is removed. Before Spark 4, this omits Spark's recovery guidance. On Spark 4.x, it also loses structured-error parity with CPU execution: the GPU path should preserve Spark's error condition and the exact missingPATHparameter.This change handles both generations directly in the existing fix:
REFRESH TABLE/recreate guidance and the V2 recreate-only guidance.ExecutionException-wrapped missing-file failures are converted; unrelated exceptions and deeper wrapper chains are left unchanged.GpuDataSourceRDDand V2 reader factories.The three inherited Spark tests excluded by #15511 are re-enabled, and focused RAPIDS coverage exercises the relevant paths:
RapidsMetadataCacheV1Suite: SPARK-16336,SPARK-27961 Suggest fixing FileNotFoundExceptionmaps to SparkMetadataCacheSuite.scalalines 41-63.RapidsMetadataCacheV1Suite: SPARK-16337 temporary view refreshmaps to SparkMetadataCacheSuite.scalalines 72-99.RapidsMetadataCacheV2Suite: SPARK-16336,SPARK-27961 Suggest fixing FileNotFoundExceptionmaps to SparkMetadataCacheSuite.scalalines 41-63.missing ORC file includes recovery guidance - COALESCING/MULTITHREADEDverifies the V1/V2-specific guidance.MissingFileStructuredErrorSuitecovers V1 and V2 with bothCOALESCINGandMULTITHREADED, comparing the CPU/GPU error condition and missing-path parameter and verifying GPU scan-plan evidence.FileSystemBytesReadTrackerSuite: GPU datasource RDD enriches next() missing-file failures - direct V2/wrapped V1directly covers reader failures and the metricsfinallypath.Original Spark source: https://github.com/apache/spark/blob/f74867bddfbcdd4d08076db36851e88b15e66556/sql/core/src/test/scala/org/apache/spark/sql/MetadataCacheSuite.scala#L41-L99
Local validation:
Tests: succeeded 14, failed 0, canceled 0, ignored 0, pending 0;All tests passed;BUILD SUCCESS.Tests: succeeded 10, failed 0, canceled 0, ignored 0, pending 0;BUILD SUCCESS.No Origin.context leaks in shared source; shim signature coverage consistent across peers.sql-plugincompile:BUILD SUCCESS.sql-plugincompile:BUILD SUCCESS.spark-parent_2.13:4.0.0-databricks-173is unavailable locally.rapids-4-spark-private_2.13:spark500:26.10.0-SNAPSHOTis unavailable locally.sql-plugin +56of 140 added production lines on Spark 3.3/Scala 2.12, andsql-plugin +45of 140 on Spark 4.0/Scala 2.13. These measurements use incompatible classfiles and are intentionally reported independently rather than summed. The final copyright-only commit is bytecode-neutral.Performance impact: a local reader-wrapper microbenchmark measured a 0.705 ns/row baseline median and 0.711 ns/row wrapped median, a +0.006 ns/row (+0.81%) delta. The successful path adds one delegating wrapper per partition and
try/catchboundaries aroundnext/get, with no additional filesystem access, metadata reads, or GPU operations.AI assistance: The change and PR description were prepared with Codex assistance and reviewed by the author before submission.
Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance