Skip to content

Fix Delta v1 writer detection for spark 4.0+ - #15927

Open
jihoonson wants to merge 4 commits into
NVIDIA:mainfrom
jihoonson:fix-v1-writer-detect
Open

Fix Delta v1 writer detection for spark 4.0+#15927
jihoonson wants to merge 4 commits into
NVIDIA:mainfrom
jihoonson:fix-v1-writer-detect

Conversation

@jihoonson

@jihoonson jihoonson commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Fixes #15926.

Description

A Delta replaceWhere overwrite through DataFrameWriter.saveAsTable could lose the existing partition specification when running on GPU with Spark 4.0 or later. When .partitionBy(...) was not restated, rows outside the replacement predicate survived, but their partition-column values were read as NULL.

Spark 4.0 moved the classic writer implementation to org.apache.spark.sql.classic.DataFrameWriter. The existing stack-trace-based V1 writer detection still checked the pre-Spark-4 class, causing the operation to be treated as a V2 table replacement and allowing the existing partition metadata to be replaced. This change should introduce no performance change as the fixed v1 writer detection for Spark 4.0 is identical as it for older versions except for the writer class name. For Spark 4.1, the detection logic should be even cheaper as it checks only the DeltaOptions and SaveMode.

This change moves V1 saveAsTable overwrite detection behind the Delta runtime shim and uses the appropriate implementation for each supported Delta version:

  • Older Delta versions retain the existing V1 writer detection.
  • Delta 4.0 checks for Spark's classic DataFrameWriter.
  • Delta 4.1 uses Delta's CreateDeltaTableLikeShims.isV1WriterSaveAsTableOverwrite helper.

The detected writer type is computed once and used consistently when updating table metadata, selecting replace semantics, and handling domain metadata.

The new integration test creates equivalent CPU and GPU partitioned Delta tables and performs a replaceWhere overwrite through saveAsTable without restating .partitionBy(...). It verifies that:

  • The GPU overwrite executes GpuOverwriteByExpressionExecV1.
  • GPU and CPU partition counts match.
  • Rows outside the replacement predicate retain their partition values.
  • The table remains partitioned by region.

The change has been verified locally against Spark 3.5.5, 4.0.1, and 4.1.1 by running the new test added.

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: Jihoon Son <ghoonson@gmail.com>
@jihoonson
jihoonson force-pushed the fix-v1-writer-detect branch from 88d82bd to ab810ad Compare September 8, 2026 23:07
@greptile-apps

greptile-apps Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The implementation appears safe to merge; the remaining test-helper concern is non-blocking because the test now explicitly verifies the corrected GPU V1 path.

Findings

  1. P2 V1 path is unverified

Summary

  • Moves V1 writer classification behind the Delta runtime shim.
  • Uses Spark’s classic DataFrameWriter for Delta 4.0 and Delta’s native helper for Delta 4.1.
  • Computes the classification once and applies it consistently to metadata replacement, write semantics, domain metadata, and operation metadata.
  • Adds a regression test covering partition preservation, CPU/GPU result parity, and the version-specific GPU V1 execution node.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Delta saveAsTable overwrite] --> B[Build DeltaOptions and read SaveMode]
    B --> C{Delta runtime}
    C -->|Older versions| D[Check legacy DataFrameWriter stack frame]
    C -->|Delta 4.0| E[Check classic.DataFrameWriter stack frame]
    C -->|Delta 4.1| F[Call CreateDeltaTableLikeShims helper]
    D --> G{V1 overwrite?}
    E --> G
    F --> G
    G -->|Yes| H[Retain existing table metadata]
    G -->|No| I[Apply V2 replacement semantics]
Loading

Reviews (5) · Last reviewed commit: "Merge branch 'main' of https://github.co..."

@sameerz sameerz added the bug Something isn't working label Sep 8, 2026
@jihoonson
jihoonson requested a review from a team September 9, 2026 01:26
@jihoonson

jihoonson commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

@greptile-apps I have updated the PR description to include why the performance evaluation is not required for this PR. Please have a look.

Comment on lines +904 to +907
try:
with_gpu_session(lambda spark: replace_na_partition(spark, gpu_table), conf=confs)
plans = callback.getResultsWithTimeout(10000)
assert any(callback.contains(plan, "GpuAtomicReplaceTableAsSelectExec")

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.

P2 V1 path is unverified

The regression test now checks only GpuAtomicReplaceTableAsSelectExec, the V2 replacement node, instead of also verifying GpuOverwriteByExpressionExecV1, the V1 path corrected by this PR. A regression in V1 writer detection could therefore pass while the surrounding V2 command still executes. The test also compares CPU and GPU results manually rather than using assert_gpu_and_cpu_are_equal_collect or assert_gpu_fallback_collect, as required by the repository's GPU integration-test directive. This requirement must be satisfied before merging; please retain an explicit V1-node assertion while using one of the required comparison helpers.

Rule Used: Integration tests must verify GPU execution using ... (source)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

sdrp713
sdrp713 previously approved these changes Sep 9, 2026

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

@jihoonson

Copy link
Copy Markdown
Collaborator Author

build

gerashegalov
gerashegalov previously approved these changes Sep 10, 2026

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

@jihoonson
jihoonson dismissed stale reviews from gerashegalov and sdrp713 via 21829e8 September 10, 2026 06:34
@jihoonson

Copy link
Copy Markdown
Collaborator Author

build

* Detect a DataFrameWriter V1 mode("overwrite").saveAsTable operation so it retains the
* existing table metadata. Delta versions before 4.1 require stack-trace inspection.
*/
def isV1WriterSaveAsTableOverwrite(options: DeltaOptions, mode: SaveMode): Boolean = {

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.

Delta42xRuntimeShim extends DeltaRuntimeShimBase directly and does not override this method. That reintroduces the bug for Delta 4.2—its Spark 4.0 artifact checks classic.DataFrameWriter, while its Spark 4.1 artifact reads the explicit V1-overwrite option. Could Delta42xRuntimeShim delegate to CreateDeltaTableLikeShims just like Delta41xRuntimeShim?

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] GPU Delta replaceWhere via saveAsTable drops partition values for untouched rows

5 participants