Add row tracking preservation tests for MERGE, UPDATE and DELETE on OSS Delta [databricks] - #15907
Open
jtwynne wants to merge 7 commits into
Open
Add row tracking preservation tests for MERGE, UPDATE and DELETE on OSS Delta [databricks]#15907jtwynne wants to merge 7 commits into
jtwynne wants to merge 7 commits into
Conversation
…SS Delta The only tests that check the GPU DML commands preserve row tracking ran on Databricks 17.3. The OSS UPDATE and DELETE commands preserve it through UpdateCommand.preserveRowTrackingColumns and the OSS MERGE command inherits ClassicMergeExecutor.writeAllChanges, which does the same, but nothing exercised them. Add supports_delta_lake_row_tracking: Databricks 17.3 and later, or OSS Delta Lake 3.3, which the plugin pairs with Spark 3.5.x and later. Gate the UPDATE and DELETE row tracking tests on it instead of Databricks 17.3 and drop the _db173 suffix from their names. Add test_delta_merge_preserves_row_tracking: a matched update and an insert into a row-tracked target on the CPU and on the GPU; the rows that existed before keep their row id on both engines, copied rows keep their commit version, the updated row gets the new one, and the inserted row gets a fresh id. The ids are checked per row because the join-based merge lays out files differently from the CPU, which the commit-log comparison of the UPDATE and DELETE tests does not tolerate. The NOT MATCHED BY SOURCE clause is left out because it runs on the GPU only with Delta 4.1 and Databricks 17.3. Fixes NVIDIA#15906 Signed-off-by: Thomas Wynne <jtwynne3@gmail.com>
Contributor
Greptile SummaryThis PR broadens Delta row-tracking preservation coverage across supported OSS Delta Lake and Databricks environments.
Confidence Score: 5/5The PR appears safe to merge; the previously reported test-framework and Databricks coverage concerns are resolved, and no new actionable defects remain. The MERGE test now uses the standard GPU-verified Delta write helper, UPDATE and DELETE retain equivalent verification, and unsupported or known-broken runtime combinations are explicitly gated. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Detect row-tracking support] --> B{Supported runtime?}
B -- No --> C[Skip preservation tests]
B -- Yes --> D[Create matching CPU and GPU row-tracked tables]
D --> E[Run UPDATE or DELETE or MERGE]
E --> F[Verify GPU Delta execution]
F --> G[Read row metadata on CPU]
G --> H[Compare CPU and GPU results]
H --> I[Assert existing IDs persist and versions change correctly]
Reviews (7): Last reviewed commit: "Merge branch 'main' into oss-merge-row-t..." | Re-trigger Greptile |
The merge row tracking test ran the GPU merge through the plan-capture helper and compared the business columns between the engines by hand. Use assert_gpu_and_cpu_writes_are_equal_collect instead: it runs the merge on both engines and compares the rows that existed before, including their row id and commit version, which are identical on both engines because both tables start from the same single file. The per-engine checks stay for what cannot be compared across engines: the ids and versions against the before state, and the freshness of the inserted row's id, which the file layout decides. Signed-off-by: Thomas Wynne <jtwynne3@gmail.com>
Run the merge on both engines through assert_cpu_and_gpu_are_equal_collect_with_capture with GpuExecutedCommandExec as a required class, so a fallback to the CPU merge command fails the test instead of passing on identical results, and compare the merge's own result row between the engines. The rows that existed before are still compared engine to engine with their row id and commit version, and the per-engine checks against the before state and for the inserted row's fresh id are unchanged. Signed-off-by: Thomas Wynne <jtwynne3@gmail.com>
The previous commit asserted GpuExecutedCommandExec on the merge's own DataFrame plan. Since Spark 3.5 that plan is a CommandResult wrapper around the executed command, so the assertion could not see the GPU command and failed on Spark 3.5.6 although the GPU merge had run. Capture the plans of the GPU run with ExecutionPlanCaptureCallback, as the REORG tests do, and require GpuExecutedCommandExec and the GPU Delta write in them. The merge result row and the rows that existed before, with their row id and commit version, are still compared between the engines. Signed-off-by: Thomas Wynne <jtwynne3@gmail.com>
… skip it on Databricks test_delta_merge_preserves_row_tracking drove the CPU and GPU sessions by hand with its own plan capture. It now runs through assert_gpu_and_cpu_writes_are_equal_collect, the path the UPDATE and DELETE tests already take: for a Delta test the helper runs the GPU side under assert_rapids_delta_write, which asserts the GPU Delta write that only the GPU merge command produces, then reads both tables back on the CPU with the row tracking columns and compares them engine to engine (the inserted row's id masked, since the file layout decides it). The merge's result row and the per-engine row id and commit version checks stay. The capability predicate includes Databricks 17.3, but the 17.3 GPU merge on main regenerates row ids (NVIDIA#15884 fixes that and carries the 17.3 test), so the MERGE test is skipped on Databricks with that reason. The UPDATE and DELETE tests keep running on 17.3. Signed-off-by: Thomas Wynne <jtwynne3@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #15906.
Description
The only tests that check the GPU DML commands preserve row tracking ran on Databricks 17.3. The OSS UPDATE and DELETE commands preserve it through
UpdateCommand.preserveRowTrackingColumns, and the OSS MERGE command inheritsClassicMergeExecutor.writeAllChanges, which does the same, but nothing exercised them (#15906 first claimed a defect there; it was wrong, the coverage gap is what remains).supports_delta_lake_row_tracking()inspark_session.py: Databricks 17.3 and later, or OSS Delta Lake 3.3, which the plugin pairs with Spark 3.5.x and later.test_delta_update_preserves_row_trackingandtest_delta_delete_preserves_row_tracking(renamed from_db173) are gated on it instead of Databricks 17.3.test_delta_merge_preserves_row_tracking: a matched update and an insert into a row-tracked target, run on both engines throughassert_gpu_and_cpu_writes_are_equal_collect. For a Delta test the helper runs the GPU side underassert_rapids_delta_write, which asserts the GPU Delta write in the plans it captures, and only the GPU merge command writes that way, so a fallback to the CPU merge command fails the test. The helper then reads both tables back on the CPU with_metadata.row_idand_metadata.row_commit_versionand compares them engine to engine (both tables start from the same single file, so the rows that existed before carry the same ids and versions on both engines; the inserted row's id is masked because the file layout decides it and the join-based GPU merge lays out files differently from the CPU). Per engine, the rows that existed before keep their row id, copied rows keep their commit version, the updated row gets the new one, and the inserted row gets a fresh id. The merge's result row is compared between the engines; the commit logs are not, for the same layout reason. The NOT MATCHED BY SOURCE clause is left out because it runs on the GPU only with Delta 4.1 and Databricks 17.3; Support NOT MATCHED BY SOURCE in the GPU MERGE command on Databricks 17.3 [databricks] #15884 has the 17.3 test with that clause.The capability predicate includes Databricks 17.3, where
UPDATEandDELETEpreserve row tracking on the GPU and the two tests pass. The 17.3 GPUMERGEonmainregenerates row ids (#15884 fixes that and carries the 17.3 merge test), sotest_delta_merge_preserves_row_trackingis skipped on Databricks with that reason until #15884 lands.Tests
On an Azure
Standard_NC16as_T4_v3(one T4, driver 610.57.04), plugin built from this branch,run_pyspark_from_build.sh --delta_lake -m delta_lake -k preserves_row_trackingwith the Delta packages, extension and catalog confs fromjenkins/spark-tests.sh:test_delta_merge_preserves_row_tracking,test_delta_update_preserves_row_tracking,test_delta_delete_preserves_row_tracking), 0 failed, noWAS LEAKEDlines.WAS LEAKEDlines.WAS LEAKEDlines.WAS LEAKEDlines on each of Spark 4.0.0 / Delta 4.0.1, 3.5.6 / 3.3.0 and 4.1.1 / 4.1.0 (same T4 box).Standard_NC16as_T4_v3,run_pyspark_from_build.sh --runtime_env=databricks):test_delta_update_preserves_row_trackingandtest_delta_delete_preserves_row_trackingpassed,test_delta_merge_preserves_row_trackingskipped with the Support NOT MATCHED BY SOURCE in the GPU MERGE command on Databricks 17.3 [databricks] #15884 reason, noWAS LEAKEDlines.Checklists
Documentation
Testing
(Please provide the names of the existing tests in the PR description.)
Performance