Drop _tmp_metadata_row_index column from the output of Delta Scan on GPU by razajafri · Pull Request #13991 · NVIDIA/cudf-spark

razajafri · 2025-12-11T01:56:18Z

Description

When returning output from GpuFileSourceScanExec, we leave the temporary row_index column that is generated as part of the DV scan. The presence of this column breaks our assumption that partition columns follow immediately after the output column. This PR removes the _tmp_metadata_row_index column from the output.

Checklists

This PR has added documentation for new or modified features or behaviors.
This PR has added new tests or modified existing tests to cover new code paths.
(Please explain in the PR description how the new code paths are tested, such as names of the new/existing tests that cover them.)
Performance testing has been performed and its results are added in the PR description. Or, an issue has been filed with a link in the PR description.

jihoonson

My understanding of this bug is that the _tmp_metadata_row_index is still part of the output of GpuFileSourceScanExec even after DeltaProvider.pruneFileMetadata(). This is messing up with the the logic to find the partition column. My suggestion is to modify DeltaProviderBase.pruneFileMetadata() as below, so that we can have the metadata pruning logic in one place. I think this should be safe as we never use the _tmp_metadata_row_index column anyway. I tested this change manually and it seems to work.

diff --git a/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala b/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala
index 126de3544..96ff4e0e1 100644
--- a/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala
+++ b/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala
@@ -162,6 +162,7 @@ abstract class DeltaProviderBase extends DeltaIOProvider {
             dvFilterInput.copy(projectList = inputList.filterNot(_.name == "_metadata"))
               .withNewChildren(Seq(
                 fsse.copy(
+                  originalOutput = fsse.originalOutput.filterNot(_.name == "_tmp_metadata_row_index"),
                   requiredSchema = StructType(
                     fsse.requiredSchema.filterNot(_.name == "_tmp_metadata_row_index")
                   ))(fsse.rapidsConf)))))))

Also, please add some test to cover this bug.

razajafri · 2025-12-11T19:56:13Z

My understanding of this bug is that the _tmp_metadata_row_index is still part of the output of GpuFileSourceScanExec even after DeltaProvider.pruneFileMetadata(). This is messing up with the the logic to find the partition column. My suggestion is to modify DeltaProviderBase.pruneFileMetadata() as below, so that we can have the metadata pruning logic in one place. I think this should be safe as we never use the _tmp_metadata_row_index column anyway. I tested this change manually and it seems to work.
diff --git a/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala b/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala
index 126de3544..96ff4e0e1 100644
--- a/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala
+++ b/delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala
@@ -162,6 +162,7 @@ abstract class DeltaProviderBase extends DeltaIOProvider {
             dvFilterInput.copy(projectList = inputList.filterNot(_.name == "_metadata"))
               .withNewChildren(Seq(
                 fsse.copy(
+                  originalOutput = fsse.originalOutput.filterNot(_.name == "_tmp_metadata_row_index"),
                   requiredSchema = StructType(
                     fsse.requiredSchema.filterNot(_.name == "_tmp_metadata_row_index")
                   ))(fsse.rapidsConf)))))))
Also, please add some test to cover this bug.

Thanks, I think that is a much cleaner approach. I will update

Signed-off-by: Raza Jafri <raza.jafri@gmail.com>

razajafri · 2025-12-11T21:12:13Z

build

jihoonson · 2025-12-11T21:13:33Z

Also, please add some test to cover this bug.

You seem to have missed my previous comment. Please add some test.

greptile-apps · 2025-12-11T21:14:49Z

Greptile Overview

Greptile Summary

This PR fixes a bug in the GPU-accelerated Delta Lake scan with deletion vectors (DV). When GpuFileSourceScanExec returns output, the temporary _tmp_metadata_row_index column was remaining in originalOutput, breaking the assumption that partition columns immediately follow data columns (since output is split at requiredSchema.length). The fix filters _tmp_metadata_row_index from both requiredSchema (already done) and now originalOutput.

Added filtering of _tmp_metadata_row_index from fsse.originalOutput in pruneFileMetadata method
Added test test_delta_filter_out_metadata_col that creates a partitioned Delta table with deletion vectors and verifies GPU/CPU equality

Confidence Score: 5/5

This PR is safe to merge - it's a targeted bug fix with appropriate test coverage.
The change is minimal (2 lines of Scala) and follows an existing pattern. The fix is symmetric - filtering the column from originalOutput the same way it was already being filtered from requiredSchema. The new test validates the fix using the established GPU/CPU equality assertion pattern.
No files require special attention.

Important Files Changed

File Analysis

Filename	Score	Overview
delta-lake/common/src/main/delta-33x-40x/scala/com/nvidia/spark/rapids/delta/common/DeltaProviderBase.scala	5/5	Added filtering of `_tmp_metadata_row_index` from `originalOutput` in the `pruneFileMetadata` method, fixing partition column position assumptions when deletion vectors are used.
integration_tests/src/main/python/delta_lake_test.py	5/5	Added test `test_delta_filter_out_metadata_col` to verify Delta Lake DV scan correctly handles metadata column removal when partitioned tables are read.

Sequence Diagram

sequenceDiagram
    participant Delta as Delta Table (with DVs)
    participant Plan as Query Plan
    participant Prune as pruneFileMetadata
    participant FSSE as GpuFileSourceScanExec
    participant Output as Output Attributes

    Delta->>Plan: Read partitioned table
    Plan->>Plan: Add _tmp_metadata_row_index<br/>for DV filtering
    Plan->>Prune: Match DV scan pattern
    Prune->>FSSE: Filter _metadata from project list
    Prune->>FSSE: Filter _tmp_metadata_row_index<br/>from requiredSchema
    Prune->>FSSE: Filter _tmp_metadata_row_index<br/>from originalOutput (NEW)
    FSSE->>Output: Split at requiredSchema.length
    Output->>Output: Data attrs ++ Partition attrs<br/>(correctly positioned)

greptile-apps

_{1 file reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

Additional Comments (1)

integration_tests/src/main/python/delta_lake_test.py, line 170 (link)

style: Unused import - DeltaTable is imported but never used in this test.

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

razajafri · 2025-12-11T22:11:30Z

build

gerashegalov · 2025-12-11T22:12:11Z

+        USING DELTA
+        LOCATION '{data_path}'
+        PARTITIONED BY (region)
+        TBLPROPERTIES ('delta.enableDeletionVectors' = 'true')


This is an internal example that is prone to a single record per file where DVs are not necessary upon delete since the whole file can be dropped.

Please use the more robust example for regression test. You can rewrite in SQL if you like

from delta import DeltaTable spark.range(1000).withColumnRenamed('id', 'l_id').join(spark.range(10)).write.partitionBy('l_id').format('delta').option('delta.enableDeletionVectors', True).save('/tmp/range_dvp4') dt = DeltaTable.forPath(spark, '/tmp/range_dvp4') dt.delete('id = 1') dt.toDF().explain()

jihoonson · 2025-12-11T22:27:48Z

Can you also update the PR title and description to match with the updated change?

razajafri · 2025-12-12T00:37:25Z

@jihoonson @gerashegalov thank you for reviewing the PR. I have addressed all your concerns PTAL

greptile-apps

Additional Comments (1)

integration_tests/src/main/python/delta_lake_test.py, line 178-179 (link)

style: Trailing whitespace on line 178, and line 179 has 7 spaces instead of 8 for indentation.

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

jihoonson

LGTM

razajafri · 2025-12-12T00:54:22Z

build

gerashegalov · 2025-12-12T01:15:40Z

+    data_path = spark_tmp_path + "/DELTA_DATA"
+
+    def create_delta(spark):
+        two_col_df(spark, int_gen, int_gen).coalesce(1).write.format("delta") \


Is int_gen guaranteed to have a 0 that you are later deleting?

Is coalesce(1) the same as passing num_slices=1 to two_col_df ?

0 is guaranteed to be present as it's a special_case in the IntegerGen

gerashegalov · 2025-12-12T01:16:58Z

+            .option("delta.enableDeletionVectors", "true") \
+            .partitionBy("a").save(data_path)
+
+        spark.sql(f"DELETE FROM delta.`{data_path}` WHERE b = 0")


Should we assert that we deleted at least one row by looking at the result df returned by this statement?

good idea. I think we should

gerashegalov · 2025-12-12T01:21:25Z

+        spark.sql(f"DELETE FROM delta.`{data_path}` WHERE b = 0")
+
+    def read_table(spark): 
+       return spark.sql(f"SELECT * FROM delta.`{data_path}` ")


Alternatively to the above suggestions, can we assert the explain output includes DV-enabled scan with __delta_internal_is_row_deleted column?

greptile-apps

Additional Comments (1)

integration_tests/src/main/python/delta_lake_test.py, line 179-180 (link)

style: Trailing whitespace on line 179 and inconsistent indentation (7 spaces instead of standard 8).

_{2 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps

_{2 files reviewed, no comments}

_{Edit Code Review Agent Settings | Greptile}

razajafri · 2025-12-12T02:47:36Z

build

gerashegalov

LGTM

### Description A recently introduced test in #13991 is irrelevant for DBR as we currently don't support Deletion Vectors on DBR. This PR skips the test on DBR and for versions of Spark that don't support Deletion Vectors ### Checklists - [ ] This PR has added documentation for new or modified features or behaviors. - [x] This PR has added new tests or modified existing tests to cover new code paths. (Please explain in the PR description how the new code paths are tested, such as names of the new/existing tests that cover them.) - [ ] Performance testing has been performed and its results are added in the PR description. Or, an issue has been filed with a link in the PR description. --------- Signed-off-by: Raza Jafri <raza.jafri@gmail.com>

sameerz requested a review from a team December 11, 2025 15:17

razajafri changed the base branch from main to release/25.12 December 11, 2025 17:15

jihoonson requested changes Dec 11, 2025

View reviewed changes

drop _metadata_row_index column

0cf8bb5

razajafri force-pushed the nv-5729433-prunePartitionSchema branch from 425e311 to 0cf8bb5 Compare December 11, 2025 19:56

Signing off

31743d2

Signed-off-by: Raza Jafri <raza.jafri@gmail.com>

razajafri marked this pull request as ready for review December 11, 2025 21:10

razajafri requested a review from jihoonson December 11, 2025 21:11

greptile-apps Bot reviewed Dec 11, 2025

View reviewed changes

razajafri and others added 2 commits December 11, 2025 14:03

added tests

75bb8f5

Remove unnecessary commented line

baea0c2

greptile-apps Bot reviewed Dec 11, 2025

View reviewed changes

gerashegalov reviewed Dec 11, 2025

View reviewed changes

razajafri changed the title ~~Prune partition schema should drop _metadata columns~~ Drop _tmp_metadata_row_index column from the output of Delta Scan on GPU Dec 12, 2025

Updated the test to be more robust

1671116

greptile-apps Bot reviewed Dec 12, 2025

View reviewed changes

jihoonson previously approved these changes Dec 12, 2025

View reviewed changes

gerashegalov reviewed Dec 12, 2025

View reviewed changes

Addressed review comments

2af9c5d

razajafri dismissed jihoonson’s stale review via 2af9c5d December 12, 2025 02:15

greptile-apps Bot reviewed Dec 12, 2025

View reviewed changes

assert test has deletion vector scan

e67c990

greptile-apps Bot reviewed Dec 12, 2025

View reviewed changes

gerashegalov approved these changes Dec 12, 2025

View reviewed changes

sameerz added the bug Something isn't working label Dec 12, 2025

razajafri merged commit 01090d8 into NVIDIA:release/25.12 Dec 12, 2025
62 checks passed

razajafri deleted the nv-5729433-prunePartitionSchema branch December 12, 2025 06:23

razajafri mentioned this pull request Dec 12, 2025

Skip test_delta_filter_out_metadata_col on [databricks] #14002

Merged

3 tasks

revans2 mentioned this pull request Dec 15, 2025

[BUG] Delta Lake deletion vector tests failing on Spark != 3.5 due to version incompatibilities #14009

Closed

Uh oh!

Conversation

razajafri commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklists

Uh oh!

jihoonson left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

razajafri commented Dec 11, 2025

Uh oh!

razajafri commented Dec 11, 2025

Uh oh!

jihoonson commented Dec 11, 2025

Uh oh!

greptile-apps Bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Overview

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Sequence Diagram

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Additional Comments (1)

Uh oh!

razajafri commented Dec 11, 2025

Uh oh!

gerashegalov Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jihoonson commented Dec 11, 2025

Uh oh!

razajafri commented Dec 12, 2025

Uh oh!

greptile-apps Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Additional Comments (1)

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

razajafri commented Dec 12, 2025

Uh oh!

gerashegalov Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

razajafri Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

gerashegalov Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

razajafri Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

gerashegalov Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Additional Comments (1)

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

razajafri commented Dec 12, 2025

Uh oh!

gerashegalov left a comment

Choose a reason for hiding this comment

razajafri commented Dec 11, 2025 •

edited

Loading

jihoonson left a comment •

edited

Loading

greptile-apps Bot commented Dec 11, 2025 •

edited

Loading

greptile-apps Bot left a comment •

edited

Loading

gerashegalov Dec 11, 2025 •

edited

Loading

greptile-apps Bot left a comment •

edited

Loading

greptile-apps Bot left a comment •

edited

Loading