-
Notifications
You must be signed in to change notification settings - Fork 302
Support NOT MATCHED BY SOURCE in the GPU MERGE command on Databricks 17.3 [databricks] #15884
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jtwynne
wants to merge
20
commits into
NVIDIA:main
Choose a base branch
from
jtwynne:db173-merge-not-matched-by-source
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
20 commits
Select commit
Hold shift + click to select a range
b3698f5
Support NOT MATCHED BY SOURCE in the GPU MERGE command on Databricks …
jtwynne 6c0354c
Fix batch leak in the GPU merge processor when a clause projection th…
jtwynne e0df412
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne 620ba54
Compensate numTargetRowsMatchedDeleted for duplicate source matches […
jtwynne fbb6b2a
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne d39b613
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne 329f704
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne 7d95ec4
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne cfa6d77
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne eb8f690
Apply WHEN MATCHED conditions to multiple-match detection on Databric…
jtwynne ded963e
Treat a NULL clause condition as false in the GPU merge processor [da…
jtwynne 84c88fc
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne 108cf7c
Generate the helper column names for duplicate-match de-duplication […
jtwynne e23e07e
Merge branch 'db173-merge-not-matched-by-source' of https://github.co…
jtwynne bbfabf8
Attach every merge helper column under a generated name [databricks]
jtwynne 2a1669b
Preserve row tracking through the Databricks 17.3 GPU merge [databricks]
jtwynne 6be0e90
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne 3ebe117
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne 661ef6b
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne cf98af6
Merge branch 'main' into db173-merge-not-matched-by-source
jtwynne File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
processProjectionSeriespartitions rows usingpredicateandpredicate.not(). If a nullable NOT MATCHED BY SOURCE condition evaluates to NULL, both cuDF filter masks exclude the row becauseNOT NULLremains NULL. The row consequently reaches neither the clause output nornoopCopyOutput, silently removing an unmatched target row. Could we coalesce each condition to false before splitting and add a nullable-condition test? SQL MERGE should preserve the target row when no clause condition evaluates to true.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! And it looks like it isn't specific to the new clause. Every condition goes through the same split, so a
WHEN MATCHED AND s.flagwith a NULL flag was deleting the target row on the GPU where the CPU copies it. Fixed in ded963e, one level below these lines insplitBatchAndClose(the one place every clause type passes through) rather than just on the not-matched-by-source conditions: NULLs in the evaluated condition get replaced with false before the two filters (masks without NULLs are used as-is), so the row falls through to the next clause or the default the same way the CPU row processor does.Tests:
test_delta_merge_nullable_matched_conditions(all Delta versions; matched update and delete clauses plus an insert clause with NULL conditions, expected rows spelled out) andtest_delta_merge_nullable_not_matched_by_source_condition(OSS 4.1 and 17.3+; adds a NOT MATCHED BY SOURCE condition that's NULL for target rows with a NULL column, GPU processor asserted in the plan). Without the fix the first one loses the four matched rows with a NULL flag and the second also loses the two target-only rows with a NULL column.