Updating incremental merge WHERE condition to handle nullable fields …

[amardatar]

…with an appropriate null comparison.

resolves dbt-labs/dbt-adapters#159

Description

This is a proposed resolution to dbt-labs/dbt-adapters#159 which updates the check in the WHERE condition of the DELETE statement of an incremental merge, to use an additional {{ target }}.{{ key }} is null and {{ source }}.{{ key }} is null check to handle nulls.

Checklist

• I have read the contributing guide and understand what's expected of me
• I have signed the CLA
• I have run this code in development and it appears to resolve the stated issue
• This PR includes tests, or tests are not required/relevant for this PR
• I have opened an issue to add/update docs, or docs changes are not required/relevant for this PR
• I have run changie new to create a changelog entry

[dbeatty10]

Thank you for opening this @amardatar !

Do you think it's ready for review? Let us know if you need any assistance getting this across the line.

[amardatar]

Yep I think it's ready! Let me know if there's anything more that is required - but is functioning as expected (and seemed fine on the existing tests I ran).

[jtcohen6]

@cla-bot check

[cla-bot]

The cla-bot has been summoned, and re-checked this pull request!

[amardatar]

Hey @dbeatty10 / @jtcohen6 - just wanted to quickly follow-up on this in case it got lost! If it's sitting on a backlog already then don't mind me, just wanted to make sure it's going somewhere eventually :)

[dbeatty10]

Thanks for checking in @amardatar! Indeed it's on our backlog.

[github-actions]

This PR has been marked as Stale because it has been open with no activity as of late. If you would like the PR to remain open, please comment on the PR or else it will be closed in 7 days.

[nikita-sheremet-flocktory]

@dbeatty10
Any news for releasing this bug? May I help? Can I do something to help or speed up this PR to release?

Thanks in advance!

[dbeatty10]

@dbeatty10 Any news for releasing this bug? May I help? Can I do something to help or speed up this PR to release?

Thanks in advance!

@nikita-sheremet-flocktory

Since this PR was originally opened, we split the code within dbt-core into several repos:

• dbt-core
• dbt-common
• dbt-postgres
• dbt-adapters

So there's two things we'll need to do:

1. Open a new PR in https://github.com/dbt-labs/dbt-adapters that has the same contents as this one
2. Add an applicable test case

Both would be helpful, but adding the test case would be the most helpful.

[nikita-sheremet-flocktory]

@dbeatty10

Add an applicable test case

Could you please share a link to application test that can be used as example? For now I do not have ideas how it should be implemented.

Thanks in advance!

[amardatar]

Hey @dbeatty10 @nikita-sheremet-flocktory I created dbt-labs/dbt-adapters#110 for this.

@dbeatty10 as far as the test case goes, and from what I could see, it looked like the existing tests should work for this just with a change in the fixtures (rather than the tests themselves). I've made those edits instead, but I couldn't really work out how tests are currently meant to run (if they're even meant to run from here, or if they should be run from the adapters themselves).

[dbeatty10]

Thanks for taking the time to open this PR @amardatar! Since opening, we've decoupled dbt Adapters from dbt Core, and this code now lives in a separate repo: dbt-adapters.

A consequence of the decoupling is that PR can't be merged anymore as is, so we're closing it. For more context see #9171.

The linked issue has already been transferred. Please don't hesitate to re-open your proposed code changes within this PR in the dbt-adapters repo.